Skip to content
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

Re-Add Synchronization Import on non-Darwin #742

Merged
merged 3 commits into from
Jul 22, 2024

Conversation

jmschonfeld
Copy link
Contributor

Previously, we had issues with import Synchronization in the toolchain - this was due to the rpaths of the produced libraries. We previously applied -no-toolchain-stdlib-rpath so that the rpath of the libraries in the build folder had no rpaths set. When the libraries are installed into the toolchain, $ORIGIN was appended which is correct for the installed library. However, XCTest's tests link against these libraries directly in the build folder before they are installed. Loading the stdlib worked because XCTest's tests also link the stdlib and so the library was found in the cache, however since XCTest's tests do not link Synchronization it wasn't found in the cache and since the rpaths list was empty the library couldn't be found.

Instead, we now remove the -no-toolchain-stdlib-rpath argument so that the libraries in the build directory have rpaths pointing to the swift build folder. However, we also now set INSTALL_REMOVE_ENVIRONMENT_RPATH to ON so that when copying to the installation folder, this path is replaced by $ORIGIN instead of just appending the $ORIGIN path to the list so that the produced toolchain doesn't contain local rpaths.

This should also resolve a related warning in the macro due to the lack of rpaths while loading the macro from the build folder to build FoundationEssentials itself:

/home/build-user/swift-foundation/Sources/FoundationEssentials/Predicate/Expression.swift:32:14: warning: external macro implementation type 'FoundationMacros.ExpressionMacro' could not be found for macro 'Expression'; failed to load library plugin '/home/build-user/build/buildbot_linux/foundation-linux-x86_64/lib/libFoundationMacros.so' in plugin server '/home/build-user/build/buildbot_linux/swift-linux-x86_64/lib/swift/host/libSwiftInProcPluginServer.so'; loader error: libswift_StringProcessing.so: cannot open shared object file: No such file or directory

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test

@jmschonfeld
Copy link
Contributor Author

Full toolchain tests and a full toolchain build is running at swiftlang/swift#75321

Copy link
Contributor

@parkera parkera left a comment

Choose a reason for hiding this comment

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

Awesome, thanks so much for tracking this down.

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test

1 similar comment
@Azoy
Copy link
Contributor

Azoy commented Jul 18, 2024

@swift-ci please test

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test

@parkera
Copy link
Contributor

parkera commented Jul 19, 2024

For now I had to remove the file that used Synchronization, so this PR actually becomes somewhat of a no-op in the short term. However, I am sure we will want Synchronization back soon.

@jmschonfeld jmschonfeld force-pushed the import-synchronization branch from adafe0d to 911c5b8 Compare July 22, 2024 18:22
@jmschonfeld
Copy link
Contributor Author

For now I had to remove the file that used Synchronization, so this PR actually becomes somewhat of a no-op in the short term. However, I am sure we will want Synchronization back soon.

Sounds good - I've rebased on main so that we can still land this so that PredicateExpressions.Variable can still use Synchronization (and wherever else we need it in the future)

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test

@jmschonfeld jmschonfeld merged commit 37cdca9 into swiftlang:main Jul 22, 2024
3 checks passed
@jmschonfeld jmschonfeld deleted the import-synchronization branch July 22, 2024 20:26
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.

3 participants