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

[Auditor] Do not skip GCC libraries when updating linkage #1240

Merged
merged 3 commits into from
Oct 29, 2022

Conversation

giordano
Copy link
Member

@giordano giordano commented Oct 26, 2022

I feel like these libraries were skipped in the old days before CompilerSupportLibraries was a thing. Ref: JuliaPackaging/Yggdrasil#3703 (comment). CC: @vtjnash @apaz-cli

@giordano giordano requested a review from staticfloat October 26, 2022 22:13
@@ -313,31 +311,6 @@ end
# Determine whether a library is a "default" library or not, if it is we need
# to map it to `@rpath/$libname` on OSX or `\$ORIGIN/$libname` on Linux/FreeBSD
is_default_lib(lib, oh) = false
Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly don't understand what's the purpose of this function.

Copy link
Member

Choose a reason for hiding this comment

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

This method is for when oh is not a MachOHandle or an ELFHandle; e.g. when it's a COFFHandle, or some other handle type that we add in the future to ObjectFile

Copy link
Member Author

Choose a reason for hiding this comment

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

The method yes, I understand, but not the function as a whole. With this PR this is the only method of this function, so we can just remove it entirely

unpack(tarball_path, testdir)
# Make sure auditor set the rpath of `hello_world`, even if it links only to
# libgfortran.
@test Auditor._rpaths(joinpath(testdir, "bin", "hello_world_fortran")) == ["\$ORIGIN/../lib"]
Copy link
Member Author

Choose a reason for hiding this comment

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

Without the change, the RPATH would be empty because all required libraries would be skipped.

@vtjnash
Copy link
Member

vtjnash commented Oct 26, 2022

Thanks!

@@ -313,31 +311,6 @@ end
# Determine whether a library is a "default" library or not, if it is we need
# to map it to `@rpath/$libname` on OSX or `\$ORIGIN/$libname` on Linux/FreeBSD
is_default_lib(lib, oh) = false
Copy link
Member

Choose a reason for hiding this comment

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

This method is for when oh is not a MachOHandle or an ELFHandle; e.g. when it's a COFFHandle, or some other handle type that we add in the future to ObjectFile

@staticfloat
Copy link
Member

Looks like we need to update the locked gitshas in some other tests?

@giordano
Copy link
Member Author

Looks like we need to update the locked gitshas in some other tests?

The failing tests were gfortran builds on Linux which changed the SHA, but not the Windows ones, which is all consistent with the changes in this PR. Isn't it exciting? 😃

@giordano
Copy link
Member Author

This PR is highlighting that so far for the most part we were relying on some system GCC libraries (like libgcc_s) to be available, but if we expect that julia can run on a system without GCC that was a wrong assumption. I had to correct a few auditor tests that were relying on it. Instead I think we should always use CompilerSupportLibraries if stuff like libgcc_s is needed

@giordano giordano merged commit 6d0e293 into JuliaPackaging:master Oct 29, 2022
@giordano giordano deleted the mg/auditor-gcc-libs branch October 29, 2022 00:40
@vtjnash
Copy link
Member

vtjnash commented Oct 29, 2022

Thanks for hunting all that down

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

Successfully merging this pull request may close these issues.

3 participants