-
Notifications
You must be signed in to change notification settings - Fork 10.5k
CMake: Specify modern Cxx interop flag when building SwiftCompilerSources #72033
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
CMake: Specify modern Cxx interop flag when building SwiftCompilerSources #72033
Conversation
The linux CI still uses Swift 5.8, swiftlang/swift-docker#368. |
acbe1eb
to
83fe59b
Compare
Darn, looks like I'll need to conditionalize it then. It appears we've already handled this in the ASTGen module. |
…rces. When building with a Swift 5.9 toolchain, we can specify the `-cxx-interoperability-mode` flag instead of the deprecated `-enable-experimental-cxx-interop` flag.
83fe59b
to
066b6d1
Compare
@swift-ci please test |
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, it indeed makes sense to use the new flag whenever we can.
One question about the version check.
@@ -98,10 +98,17 @@ function(add_swift_compiler_modules_library name) | |||
"DEPENDS" | |||
${ARGN}) | |||
|
|||
# Prior to 5.9, we have to use the experimental flag for C++ interop. | |||
if (CMAKE_Swift_COMPILER_VERSION VERSION_LESS 5.9) |
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.
I don't think CMAKE_Swift_COMPILER_VERSION
is valid unless we're building with hosttools. Last time I checked it was empty when bootstrapping is enabled since we aren't using CMake's built-in Swift support in that case. @eeckstein knows more about this.
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.
Perhaps this can be further refined to check something that indicates whether we are bootstrapping and specify -cxx-interoperability-mode=default
when we are. Lmk if that sounds reasonable.
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.
That would make sense to me!
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.
I wouldn't bother with it. Bootstrapping is on life support and not even checked on the official CI: hosttools are de facto required at this point, with bootstrapping soon to be removed.
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.
Ok, given that assessment of bootstrapping, the fact that CI passed, and that this approach matches ASTGen I'm inclined to land it as-is. I think the failure case is just that we pass the deprecated flag when bootstrapping?
When building with a Swift 5.9 toolchain, we can specify the
-cxx-interoperability-mode
flag instead of the deprecated-enable-experimental-cxx-interop
flag.