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

[5.3][CMake] fix runpath for ELF platforms #308

Merged
merged 1 commit into from Dec 18, 2020

Conversation

finagolfin
Copy link
Contributor

@finagolfin finagolfin commented Aug 29, 2020

  • Explanation

Remove the absolute path to the host toolchain's stdlib from libXCTest.so.

  • Scope

I previously submitted a pull to do this for the main branch, #303, but after discussing it with some Swift devs, I'm now submitting it for the 5.3 branch without the $ORIGIN portion which we couldn't agree on. @compnerd seemed to agree that only the part removing the build toolchain's stdlib rpath should be done on the 5.3 branch to get this removed for the upcoming release.

  • SR issue

Resolves point 2. in SR-1650

  • Risk

None, this should be completely safe for the 5.3 branch, as it's just removing the build toolchain's stdlib rpath, which should be unused by this library.

  • Testing

The other corelibs had similar pulls merged before the 5.3 release and have had no complaints.

  • Reviewer

I discussed these rpath issues in detail across the entire toolchain with @gottesmm and @compnerd.

Remove the absolute path to the host toolchain's stdlib from libXCTest.so.
@compnerd
Copy link
Collaborator

compnerd commented Sep 6, 2020

@swift-ci please test

@compnerd
Copy link
Collaborator

compnerd commented Sep 6, 2020

CC: @shahmishal - what's the current strategy for PRs to 5.3? Is it okay to merge once it passes testing?

@compnerd
Copy link
Collaborator

compnerd commented Sep 6, 2020

@swift-ci please test Linux platform

@finagolfin
Copy link
Contributor Author

Linux CI failure appears unrelated. Btw, since the similar SPM pull was held back for 5.3.1, I'm fine with waiting till then for this one too, as these are holes in the toolchain, not user executables.

@finagolfin
Copy link
Contributor Author

@compnerd, the similar SPM pull was just merged, let me know if you can get this one in too.

@finagolfin
Copy link
Contributor Author

@drexin, would be good to have this in for the next patch release, not sure what's up with the linux CI.

@finagolfin
Copy link
Contributor Author

@spevans, would you run the CI on this too?

@spevans
Copy link
Collaborator

spevans commented Oct 24, 2020

@swift-ci test

@gottesmm
Copy link
Member

@briancroom can I get a +1 here on this?

@briancroom
Copy link
Collaborator

I'm not very familiar with the CMake configuration in here, but this seems reasonable, thanks!

@finagolfin
Copy link
Contributor Author

@tomerd, just been waiting for a merge window to finally get this in.

@tomerd tomerd added the 5.3 label Dec 18, 2020
@briancroom
Copy link
Collaborator

It looks like we can land this now. Thanks for pushing on this @buttaface!

@briancroom briancroom merged commit 7d741ab into apple:release/5.3 Dec 18, 2020
@finagolfin finagolfin deleted the fix-rpath branch December 19, 2020 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants