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.

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

Introduce LLD_jll as stdlib #46455

Merged
merged 4 commits into from
Sep 30, 2022
Merged

Introduce LLD_jll as stdlib #46455

merged 4 commits into from
Sep 30, 2022

Conversation

petvana
Copy link
Member

@petvana petvana commented Aug 23, 2022

This PR adds LLD_jll to stdlib. The motivation is to enable bundling binary code into package or system images. Currently, there are at least two approaches that would benefit from it:

Cc: @vchuravy, as he asked me to prepare this PR.

It seems to work fine but is not registered in Pkg and cannot be tested. (not sure if this is correct)

julia> using LLD_jll

julia> LLD_jll.lld_path
"[USER-DIR]/julia/usr/tools/ld.lld"

julia> run(`$(LLD_jll.lld()) --version`);
LLD 14.0.5 (compatible with GNU linkers)

(@v1.9) pkg> test LLD_jll
ERROR: The following package names could not be resolved:
 * LLD_jll (not found in project or manifest)
   Suggestions: Libglvnd_jll

@petvana petvana added the stdlib Julia's standard library label Aug 23, 2022
@petvana
Copy link
Member Author

petvana commented Aug 23, 2022 •
edited
Loading

Edit: Should be fixed by 5dbdec0

The problem detected by CI is that we have only lld in libexec directory when running make binary-dist. However, we need to create system-dependent symlinks, such as the lld states:

lld is a generic driver.
Invoke ld.lld (Unix), ld64.lld (macOS), lld-link (Windows), wasm-ld (WebAssembly) instead

Any idea on how and where exaclty to fixed that?

staticfloat
staticfloat approved these changes Aug 23, 2022
View reviewed changes
Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

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

This looks good so far, but it's unclear to me if LLD will be naturally installed when using a from-source build, or even worse, a build with USE_SYSTEM_LLVM=1.

stdlib/LLD_jll/src/LLD_jll.jl
Comment on lines +42 to +59
function adjust_ENV!(env::Dict, PATH::String, LIBPATH::String, adjust_PATH::Bool, adjust_LIBPATH::Bool)
if adjust_LIBPATH
LIBPATH_base = get(env, LIBPATH_env, expanduser(LIBPATH_default))
if !isempty(LIBPATH_base)
env[LIBPATH_env] = string(LIBPATH, pathsep, LIBPATH_base)
else
env[LIBPATH_env] = LIBPATH
end
end
if adjust_PATH && (LIBPATH_env != "PATH" || !adjust_LIBPATH)
if adjust_PATH
if !isempty(get(env, "PATH", ""))
env["PATH"] = string(PATH, pathsep, env["PATH"])
else
env["PATH"] = PATH
end
end
end
return env
end
Copy link
Member

Choose a reason for hiding this comment

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

I hate that we have to duplicate this, but I understand why you're doing it.

@vchuravy
Copy link
Member

The problem detected by CI is that we have only lld in libexec directory when running make binary-dist. However, we need to create system-dependent symlinks, such as the lld states:

lld is a generic driver.
Invoke ld.lld (Unix), ld64.lld (macOS), lld-link (Windows), wasm-ld (WebAssembly) instead

Any idea on how and where exaclty to fixed that?

There is a command line flag -flavor that you can use instead of the silly symlinks https://github.com/JuliaLang/julia/pull/44527/files#diff-1ebe28ad2baac46aa0ecec530ed7b8795690e81fd6e809372f74b165779d567fR1354

@petvana
Copy link
Member Author

petvana commented Aug 23, 2022

There is a command line flag -flavor

Yes; however, this is not recommended according to the source and would make the function lld more complex.
https://github.com/llvm/llvm-project/blob/35a56e5ddc0354d0d317ef981a0a5790a145f2e0/lld/tools/lld/lld.cpp#L23-L24

@petvana
Copy link
Member Author

petvana commented Aug 23, 2022

This looks good so far, but it's unclear to me if LLD will be naturally installed when using a from-source build, or even worse, a build with USE_SYSTEM_LLVM=1.

@staticfloat You are absolutely right, lld is not yet installed when building from source. First, target install-lld is not available, but even if the target is moved outside the USE_BINARYBUILDER_LLVM condition in
https://github.com/petvana/julia/blob/d66ab76df34312b8e7d52301a709bf2b6687c71e/deps/llvm.mk#L312
lld is not insalled into usr/tools.

Btw, testing by

$ export USE_BINARYBUILDER=0
$ make

@vchuravy
Copy link
Member

There is a command line flag -flavor

Yes; however, this is not recommended according to the source and would make the function lld more complex. https://github.com/llvm/llvm-project/blob/35a56e5ddc0354d0d317ef981a0a5790a145f2e0/lld/tools/lld/lld.cpp#L23-L24

Symlink management is really annoying to do reliably, so I prefer the flavor variant, and current users of LLD_jll also use it.

@petvana
Copy link
Member Author

petvana commented Aug 24, 2022

Symlink management is really annoying to do reliably, so I prefer the flavor variant, and current users of LLD_jll also use it.

And what about renaming, instead of a symlink?

@vchuravy
Copy link
Member

I really think the flavor flag is the right way to go

@petvana
Copy link
Member Author

petvana commented Aug 24, 2022 •
edited
Loading

I really think the flavor flag is the right way to go

Updated: Ok, no problem. Now, we have only lld, or lld.exe for Windows. User need to specify -flavor every time, as in the original package.

@DilumAluthge
Copy link
Member

Tests are failing on Windows.

@petvana
Copy link
Member Author

petvana commented Aug 27, 2022

Tests are failing on Windows.

Thanks. It should be fixed by 76b3553, but it seems a little fishy that dll libraries are not in Base.LIBDIR on Windows.

@jpsamaroo
Copy link
Member

The convention on Windows is for libraries to also go in bin/

@vchuravy vchuravy requested a review from staticfloat August 28, 2022 12:40
staticfloat
staticfloat approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

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

This looks beautiful to me!

@vchuravy vchuravy added the merge me PR is reviewed. Merge when all tests are passing label Sep 29, 2022
@vchuravy vchuravy merged commit 02574e3 into JuliaLang:master Sep 30, 2022
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Oct 1, 2022
@ararslan ararslan mentioned this pull request Nov 15, 2022
Closed
@petvana petvana deleted the pv/LLD_jll-v2 branch December 1, 2022 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staticfloat staticfloat staticfloat approved these changes

Assignees
No one assigned
Labels
stdlib Julia's standard library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@petvana @vchuravy @DilumAluthge @jpsamaroo @staticfloat