-
Notifications
You must be signed in to change notification settings - Fork 124
build: introduce a new option to control library evolution #1307
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ add_library(_Testing_Foundation | |
ReexportTesting.swift) | ||
|
||
target_link_libraries(_Testing_Foundation PUBLIC | ||
_TestingInternals | ||
Testing) | ||
|
||
# Although this library links Foundation on all platforms, it only does so using | ||
|
@@ -35,6 +36,7 @@ endif() | |
# interface, because Foundation does not have Library Evolution enabled for all | ||
# platforms. | ||
target_compile_options(_Testing_Foundation PRIVATE | ||
-emit-module-interface -emit-module-interface-path $<TARGET_PROPERTY:_Testing_Foundation,Swift_MODULE_DIRECTORY>/_Testing_Foundation.swiftinterface) | ||
-emit-module-interface | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just a stylistic change, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. after the indentation for the conditional, yes. |
||
-emit-module-interface-path $<TARGET_PROPERTY:_Testing_Foundation,Swift_MODULE_DIRECTORY>/_Testing_Foundation.swiftinterface) | ||
|
||
_swift_testing_install_target(_Testing_Foundation) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -136,9 +136,13 @@ if(NOT BUILD_SHARED_LIBS) | |
endif() | ||
add_dependencies(Testing | ||
TestingMacros) | ||
target_compile_options(Testing PRIVATE | ||
-enable-library-evolution | ||
-emit-module-interface -emit-module-interface-path $<TARGET_PROPERTY:Testing,Swift_MODULE_DIRECTORY>/Testing.swiftinterface) | ||
|
||
if(SwiftTesting_ENABLE_LIBRARY_EVOLUTION) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty sure this module needs a .swiftinterface even without library evolution, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The swift interface is not generated without library evolution AFAIK. @etcwilde? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Swift interfaces are invalid without library evolution. Swift can't describe struct layouts and the interface doesn't include all properties resulting in ABI issues without library evolution. |
||
target_compile_options(Testing PRIVATE | ||
-enable-library-evolution | ||
-emit-module-interface | ||
-emit-module-interface-path $<TARGET_PROPERTY:Testing,Swift_MODULE_DIRECTORY>/Testing.swiftinterface) | ||
endif() | ||
|
||
_swift_testing_install_target(Testing) | ||
|
||
|
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.
The Foundation overlay shouldn't need to link to this module.
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 found that it did - there was a missing module error otherwise.
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.
The overlay doesn't import it and it's a static library, so if it's complaining, that probably aligns with what @stmontgomery noted in his comment.