Skip to content

Remove extra paths added to LD_LIBRARY_PATH on Linux when running tests #6684

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 10, 2023
Merged

Remove extra paths added to LD_LIBRARY_PATH on Linux when running tests #6684

merged 1 commit into from
Jul 10, 2023

Conversation

bnbarham
Copy link
Contributor

@bnbarham bnbarham commented Jul 8, 2023

These break running tests when there's already a swift on PATH that isn't the swift currently being run.

They were originally added as a workaround to (#5952), but the RUNPATH looks correct (includes the library of the compiler that built the test and "$ORIGIN") and tests are working without it.

@bnbarham
Remove extra paths added to LD_LIBRARY_PATH on Linux when running tests
5c7db58 
These break running tests when there's already a swift on PATH that
isn't the swift currently being run. There shouldn't be a need for these
as the run path of the compiled binary is already the correct path (ie.
the library path of the swift that built it and `$ORIGIN`).
@bnbarham bnbarham requested a review from DougGregor July 8, 2023 01:45
@bnbarham
Copy link
Contributor Author

bnbarham commented Jul 8, 2023

@swift-ci please test

@DougGregor
Copy link
Member

@swift-ci smoke test

@DougGregor
Copy link
Member

@swift-ci please bootstrap

@DougGregor
Copy link
Member

@swift-ci please test Windows

@DougGregor
Copy link
Member

@swift-ci please smoke test Windows

@DougGregor
Copy link
Member

@swift-ci test Windows

@bnbarham
Copy link
Contributor Author

bnbarham commented Jul 8, 2023

@swift-ci please test Windows platform

1 similar comment
@bnbarham
Copy link
Contributor Author

bnbarham commented Jul 8, 2023

@swift-ci please test Windows platform

@tomerd
Copy link
Contributor

tomerd commented Jul 8, 2023

removing workarounds is great 👍🏼. let’s make sure @neonichu is comfortable as he has the most background on why that was even needed in the first place

@bnbarham
Copy link
Contributor Author

bnbarham commented Jul 8, 2023 •
edited
Loading

Looking through the history, it looks like this was actually about libXCTest.so failing to find dependencies. There's some talk of that being caused by it having no RPATH set and that is indeed still the case. But the binary does, so its dependencies are still found. Maybe that broke at some point and is now fixed (not sure where/when that would have been though).

Setting $ORIGIN for libXCTest.so's RPATH makes sense to me, as its dependencies are expected to be found next to it. If it was loaded directly (say via a dlopen to an absolute path, where the binary itself has no RPATH set) all its dependencies would fail to be found.

@bnbarham bnbarham merged commit ada0e31 into swiftlang:main Jul 10, 2023
@bnbarham bnbarham deleted the remove-linux-test-ld-path branch July 10, 2023 20:53
This was referenced Jul 10, 2023
Merged
Merged
@bnbarham bnbarham mentioned this pull request Aug 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neonichu neonichu neonichu approved these changes

@DougGregor DougGregor DougGregor approved these changes

@abertelrud abertelrud Awaiting requested review from abertelrud

@tomerd tomerd Awaiting requested review from tomerd

@MaxDesiatov MaxDesiatov Awaiting requested review from MaxDesiatov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bnbarham @DougGregor @tomerd @neonichu