-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cxx-interop] Disable c++ execution header with libstdcxx versions >= 11 #75662
[cxx-interop] Disable c++ execution header with libstdcxx versions >= 11 #75662
Conversation
Ideally this change could make it into release/6.x, but that's assuming the maintainers think it's a worthwhile workaround before someone looks into how to get libstdc++ 11+ and tbb to behave in the module-map realm. |
@@ -1,7 +1,9 @@ | |||
#include "version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ADKaster if possible, (if this is a header visible on Microsoft Windows) I believe this include will break on that platform, since the vcruntime.modulemap
marks that version
header as C++20 language standard REQUIRED. And setting the C++ Language Standard anything past C++17 is an unreasonable expectation at the time of writing for many existing C++ libraries.
With an error that looks something like the following, I posted about it on the forums here:
// SomeApp.swift
import SomeCXXTarget
^~~~~~~~~~~~~
vcruntime.modulemap:696:10: error: module 'std.version' requires feature 'cplusplus20'
Perhaps this whole file can be guarded via #if !defined(_WIN32)
, if this header is even visible on windows in the first place.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't realize the libstdc++ overlay might be built on all platforms. I suppose features.h with a __has_include should be good enough to get the libstdc++ version check I'm trying to use.
Fwiw, when I was testing some swift things on Linux with 5.x last year when I first saw this, downgrading to libstdc++-10-dev did "workaround" this issue assuming you don't need STL features from 11+.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't realize the libstdc++ overlay might be built on all platforms. I suppose features.h with a __has_include should be good enough to get the libstdc++ version check I'm trying to use.
That should work out nicely, though I'm not entirely sure if its relevant or not (consider this Windows <version>
header PTSD 😅), just trying to limit this issue from propping up over on Windows if it can be helped (I've yet to explore the relationship between much of the Swift source code and the respective platforms in which they are built on).
Fwiw, when I was testing some swift things on Linux with 5.x last year when I first saw this, downgrading to libstdc++-10-dev did "workaround" this issue assuming you don't need STL features from 11+.
Good to note! Thank you for your findings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This header isn't used on Windows. We currently only support the platform-default C++ stdlib on each platform (there is an effort to change that in #75589). On Windows that would be the Microsoft STL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks @egorzhdan for the clarification!
@swift-ci please smoke test |
@swift-ci please build toolchain UBI9 |
@swift-ci please build toolchain CentOS 7 |
Looks like this is failing on CentOS 7:
|
Lovely. I pushed a fix that should not trip up on that version, or a libstdc++ version before _GLIBCXX_RELEASE started being defined. I'm not sure if there's any overlap between "execution header exists" and "_GLIBCXX_RELEASE not defined", but i'd rather not find out in a CI run or bug report. |
@swift-ci please smoke test |
@swift-ci please build toolchain CentOS 7 |
@swift-ci please build toolchain UBI9 |
It looks like the CI error here is due to some compiler flags issues, rather than my change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Great work! 🎉 |
Workaround for #75661