Skip to content

[Build] Update SWIFT_TARGET_LIBRARY_NAME for swiftRuntime in new build. #78977

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

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

al45tair
Copy link
Contributor

@al45tair al45tair commented Jan 28, 2025

We need to change SWIFT_TARGET_LIBRARY_NAME for the swiftRuntime target in the new build because of changes made to the old build system.

@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

@al45tair
Copy link
Contributor Author

See #78516, which changed the name so that we can use Runtime as a Swift module name.

@al45tair al45tair enabled auto-merge January 28, 2025 15:44
PRIVATE
-DSWIFT_RUNTIME
-DSWIFT_TARGET_LIBRARY_NAME=swiftRuntime
-DSWIFT_TARGET_LIBRARY_NAME=swiftRuntimeCore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the only part that actually needs changing here so that the compatibility header picks things up correctly. This is an object library that only contains C/C++, so there shouldn't be any conflict with parts from the old build. Given that the Runtime module depends on the overlay, it will have to be a separate "supplemental" library anyway.

@al45tair al45tair disabled auto-merge January 28, 2025 17:02
We need to change `SWIFT_TARGET_LIBRARY_NAME` for the `swiftRuntime`
target in the new build because of changes made to the old build system.
@al45tair al45tair force-pushed the rename-swift-runtime-target branch from c8f882d to 72d3c22 Compare January 28, 2025 17:06
@al45tair al45tair requested a review from etcwilde January 28, 2025 17:07
@al45tair al45tair changed the title [Build] Rename the swiftRuntime target in the new build system. [Build] Update SWIFT_TARGET_LIBRARY_NAME for swiftRuntime in new build. Jan 28, 2025
@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

@al45tair al45tair enabled auto-merge January 28, 2025 17:07
@Catfish-Man Catfish-Man disabled auto-merge January 28, 2025 17:12
@Catfish-Man Catfish-Man merged commit 96f23e3 into swiftlang:main Jan 28, 2025
0 of 3 checks passed
@@ -86,7 +86,7 @@ endif()
target_compile_definitions(swiftRuntime
PRIVATE
-DSWIFT_RUNTIME
-DSWIFT_TARGET_LIBRARY_NAME=swiftRuntime
-DSWIFT_TARGET_LIBRARY_NAME=swiftRuntimeCore
Copy link
Member

@compnerd compnerd Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect, this should be swiftCore. The way that this macro is used is with the CPP macro programming in Visibility.h, and the only valid options are swiftCore, swift_Concurrency, swiftDistributed, swift_Differentiation`. The value is used to select if the symbol is in the current module's ABI or not. See https://github.com/swiftlang/swift/blob/main/stdlib/public/SwiftShims/swift/shims/Visibility.h#L190-L214

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the only place it gets used, and it was being used with swiftRuntime in the past (which is not part of the list you're referring to). The macro we're tripping up over isn't actually one of the ones from Visibility.h either; it's CompatibilityOverride.h that's the problem in this case, and in particular COMPATIBILITY_OVERRIDE_INCLUDE_PATH_swiftRuntime[Core] that was the issue.

All I'm trying to fix here is the build breakage, and only by updating the spelling that I've changed, rather than doing something entirely new.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants