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

Fix bug in create_sysimg_from_object_file for Xcode 15 CLT #935

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jayscook
Copy link

@jayscook jayscook commented Apr 2, 2024

Fix for issue #738, which conditionally adds the -Wl,-ld_classic flags to o_file_flags in create_sysimg_from_object_file based on Xcode CLT version.

All credit to @PhilReinhold for this code; I'm simply opening this PR based on his latest correspondence on the issue.

Tested locally on gcc 15.0.0 and Julia 1.8.5. @hbe72 confirmed in the issue correspondence that it's working on gcc 15.0.0 and Julia 1.10

Re: CI failures, I'm seeing other recent PRs (1, 2) that are also failing the nightly build tests and buildkite. Please let me know if you see anything in this change that should address these failures.

if Sys.isapple()
cltools_version_cmd = `pkgutil --pkg-info=com.apple.pkg.CLTools_Executables`
cltools_version = match(r"version: (.*)\n", readchomp(cltools_version_cmd))[1]
if startswith(cltools_version, "15")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering if we should make this >= 15 ?
I presume this issue will still be around in xcode 16 and we don't want to go back to the flags for <=14?

Copy link
Author

Choose a reason for hiding this comment

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

I'm mildly torn on this. As-is, it forces us to revisit this fix for xcode 16 and investigate whether the issue has been solved or if our mildly hacky solution is still needed. Ultimately, though, that comes at the expense of (most likely) breaking development for anyone who upgrades to Xcode 16. I see no reason to assume that this will not continue to be an issue in xcode 16+, so making it ≥ 15 seems like the best course of action. Thanks!

o_file_flags = Sys.isapple() ? `-Wl,-all_load $object_files` : `-Wl,--whole-archive $object_files -Wl,--no-whole-archive`
if Sys.isapple()
cltools_version_cmd = `pkgutil --pkg-info=com.apple.pkg.CLTools_Executables`
cltools_version = match(r"version: (.*)\n", readchomp(cltools_version_cmd))[1]
Copy link
Member

Choose a reason for hiding this comment

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

This should handle the case that pkgutil is unable to locate com.apple.pkg.CLTools_Executables

Copy link
Author

Choose a reason for hiding this comment

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

Good call. Do you have any thoughts on what version we should assume under this failure case? I don't have a good feel for whether it's more likely that a system has been upgraded to the latest version or not, but given how Apple ties Xcode version to macOS version (and professional dev envs are eventually going to force OS updates) it may be safest to default to 15+ (i.e. to apply the flags).

Copy link
Member

@topolarity topolarity Apr 4, 2024

Choose a reason for hiding this comment

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

Unfortunately it looks like there's a syntax ambiguity so that a lot of linkers interpret this flag as -ld_classic i.e. link against libd_classic.so, which will obviously fail.

I'm not sure how far back we have to go before that becomes an issue. It also sounds like we might have to worry about using a different compiler not from XCode CLT.

Could we execute the compiler binary and ask it for its version directly?

Copy link
Author

Choose a reason for hiding this comment

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

While I have a general understanding of the problem you're describing, this is admittedly over my head. Maybe something like the first answer on this SO post could work?

Copy link
Member

@topolarity topolarity Apr 5, 2024

Choose a reason for hiding this comment

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

My biggest concern is that this patch assumes that the user is using the XCode CLT to compile on macOS, which is not necessarily true

By "execute the compiler binary directly", I mean a combination of either:

  • Modifying get_compiler_cmd to inspect the path to the binary and see that it's provided by Xcode, and/or
  • Execute run_compiler(`` --version``) or similar to determine whether we're running the Xcode-provided compiler and which version.

Copy link
Author

Choose a reason for hiding this comment

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

I've tinkered with this a bit and am still having a bit of trouble.

I've tried getting the path to the compiler with

base_dir = dirname(sysimg_source_path)
compiler_source_path = joinpath(base_dir, "compiler", "compiler.jl")

Printing compiler_source_path results in /Users/jaycook/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/base/compiler/compiler.jl. I was hoping more for something like /Library/Developer/CommandLineTools/usr/bin, where I could check for a string like "xcode" or "CommandLineTools".

run_compiler()'s underlying call to run() does not appear to return the result of the command that's run. For example, print(run_compiler(`` --version``)) results in Process(``gcc --version``, ProcessExited(0)).

If you're so inclined, do you care to toss up a commit for the implementation that you have in mind?

if startswith(cltools_version, "15")
try
cltools_version_cmd = `pkgutil --pkg-info=com.apple.pkg.CLTools_Executables`
cltools_version = match(r"version: (.*)\n", readchomp(cltools_version_cmd))[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can check for nothing here, rather than try/catch.

Copy link
Author

Choose a reason for hiding this comment

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

Can you suggest a good way to test this? If I change the --pkg-info to force a failure I get the following error, but I imagine that the "real" failure mode here is simply returning nothing. Just not sure how to go about getting my system into a state that allows me to test this before shipping it.

Screenshot 2024-04-05 at 11 17 55 AM

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.

None yet

3 participants