Skip to content

Fix bug in create_sysimg_from_object_file for Xcode 15 CLT #935

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

Closed
wants to merge 6 commits into from
+17 −1
Closed

Fix bug in create_sysimg_from_object_file for Xcode 15 CLT #935

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6d2cdd1
Fix bug in create_sysimg_from_object_file for Xcode 15 CLT
jayscook Apr 2, 2024
1d955aa
Bump version
jayscook Apr 2, 2024
7161e1a
Check for versions ≥ 15, handle pkgutil error
jayscook Apr 4, 2024
3d8f36b
Merge branch 'master' into xcode-flag-fix
KristofferC Aug 1, 2024
e17d3a8
Merge branch 'master' into xcode-flag-fix
DilumAluthge Oct 16, 2024
8c4fda1
Merge branch 'master' into xcode-flag-fix
DilumAluthge Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name = "PackageCompiler"
uuid = "9b87118b-4619-50d2-8e1e-99f35a4d4d9d"
version = "2.1.17"
version = "2.1.18"

[deps]
Artifacts = "56f22d72-fd6d-98f1-02f0-08ddc0907c33"
Expand Down
12 changes: 11 additions & 1 deletion src/PackageCompiler.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,17 @@ function create_sysimg_from_object_file(object_files::Vector{String},
end
mkpath(dirname(sysimage_path))
# Prevent compiler from stripping all symbols from the shared lib.
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

@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.

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

Copy link
Author

@jayscook jayscook Apr 4, 2024

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 •
edited
Loading

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

@jayscook jayscook Apr 5, 2024

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 •
edited
Loading

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

@jayscook jayscook Apr 8, 2024

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")
Copy link
Collaborator

@sjkelly sjkelly Apr 3, 2024

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

@jayscook jayscook Apr 4, 2024

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 = `-Wl,-all_load $object_files -Wl,-ld_classic`
else
o_file_flags = `-Wl,-all_load $object_files`
end
else
o_file_flags = `-Wl,--whole-archive $object_files -Wl,--no-whole-archive`
end
extra = get_extra_linker_flags(version, compat_level, soname)
cmd = `$(bitflag()) $(march()) -shared -L$(julia_libdir()) -L$(julia_private_libdir()) -o $sysimage_path $o_file_flags $(Base.shell_split(ldlibs())) $extra`
run_compiler(cmd; cplusplus=true)
Expand Down