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

llvmPackages/libllvm: do not include static archives when shared is r… #162607

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

waldheinz
Copy link
Contributor

Reduces closure size by ~240MiB for LLVM 13, I assume the
others are similar.

@waldheinz
Copy link
Contributor Author

I was a bit too optimistic when opening this PR because the stripped down version of libllvm is sufficient for mesa, but there are other derivations whicht break. E.g. nix-build -A clang_12 bails out with

CMake Error at //nix/store/z3im54p2im6n9qfv93xfx06sgipyvr8f-llvm-12.0.1-dev/lib/cmake/llvm/LLVMExports.cmake:1358 (message):
  The imported target "LLVMDemangle" references the file

     "/nix/store/8m5pqkjdidzv6hhjr8gansaiq24f7rhl-llvm-12.0.1-lib/lib/libLLVMDemangle.a"

Would it be acceptable to move *.a to the llvm-dev package and patching the CMake file(s) accordingly? I still think that the archives do not belong to the lib output as they are not a runtime dependency.

@waldheinz waldheinz force-pushed the libllvm-drop-static branch 2 times, most recently from 7d2c5fa to 8f8b8a4 Compare March 4, 2022 10:37
@waldheinz waldheinz marked this pull request as ready for review March 4, 2022 10:49
@waldheinz
Copy link
Contributor Author

So I just did what I said yesterday and moved the archives to the dev output, which seems to work. I updated the commit message explaning why I think this is the way to go (C++ API is only exposed via the archives for static linking, not the dynlib).

I also manually confirmed that the nixos tests "login" and "sway" succeed, which already exercises those changes quite a bit. Currently my machine is evaluating the "xfce" test. Anything else I should do?

Reduces closure size by ~240MiB (down to ~100MiB) for
LLVM 13, the others are similar.

Having those archives in the lib output makes no sense
as they are no runtime dependencies. Removing them
alltogether is also not an option because the dynamic
libraries offer only the C API while many users of
libllvm require the C++ API. Those users must have an
dependency on libllvm.dev anyway and will find those
files for linking.
@waldheinz
Copy link
Contributor Author

waldheinz commented Mar 4, 2022

I'm not happy with duplicating the exact same change 11 times with this commit, but I fear that's the style the different LLVM versions are handled in nixpkgs. So I think getting rid of the duplication would be out of scope here, if it is desirable at all.

@SuperSandro2000
Copy link
Member

I'm not happy with duplicating the exact same change 11 times with this commit, but I fear that's the style the different LLVM versions are handled in nixpkgs. So I think getting rid of the duplication would be out of scope here, if it is desirable at all.

Out of scope but I wouldn't mind if some of this stuff gets deduplicated.

@waldheinz
Copy link
Contributor Author

I promise I'll come back to this when I have a beefier machine under my desk, the compile times ain't funny. ;-)

@7c6f434c
Copy link
Member

7c6f434c commented Mar 4, 2022

This should probably target staging not master.

@waldheinz
Copy link
Contributor Author

I see, this is a bit exciting. So if there is a consensus that this might be a good idea I'll work on this a bit more to reduce code duplication in nixpkgs and address the same issue in clang (and maybe other derivations under llvmPackages; I'll have a look where it applies).

But before I invest my time on this I'd like to know if this is "the way to go" in general.

I stumbled upon this because llvm.lib is a big dependency of mesa-drivers, so reducing it to a third of it's original size may be of benefit for quite some people. Reducing the size of clang benefits much fewer installations, but it's something.

@7c6f434c
Copy link
Member

7c6f434c commented Mar 5, 2022

I think deduplication would need a larger discussion in terms of whether people who do LLVM updates expect significant changes in build procedures between versions in the future.

@vcunat vcunat changed the base branch from master to staging March 5, 2022 13:01
@Lassulus Lassulus merged commit a1780e4 into NixOS:staging Apr 8, 2022
@waldheinz waldheinz deleted the libllvm-drop-static branch June 1, 2022 14:13
@sternenseemann
Copy link
Member

sternenseemann commented Jun 17, 2022

@waldheinz This change caused llvm-config{,-native} to stop finding the static libraries when looking for them. Can you either adapt llvm-config.patch (I think that should be all that's necessary) or create a revert of this PR?

Edit: I forgot how this changed, patching llvm-config won't really work (or shouldn't) anymore, we need to either make LLVM install into different directories using it's CMake flags and nothing else.

sternenseemann added a commit that referenced this pull request Jun 17, 2022
Reverts #162607 / 1748887.

Reason for revert: This change caused llvm-config{,-native} to be unable
to find static archives bundled with LLVM, as has been [reported]. Ever
since #152944 using moveToOutput in LLVM is _evil_ because llvm-config
obtains it knowledge about the installation locations from the CMake
configure step.

Consequently a change like #162607 will need to be implemented by making
LLVM itself install the static archives to the correct location or by
adding yet another patch which updates llvm-config's knowledge of the
location. The latter is not desireable in my opinion, though, since it
is just asking for this sort of trouble: Before #152944 we had an
outputs.patch that did this sort of things which broke spectacularly in
edge cases.

Fixes #148117.

[reported]: #148117 (comment)
@sternenseemann
Copy link
Member

Proposed in #178007.

sternenseemann added a commit that referenced this pull request Jul 5, 2022
Reverts #162607 / 1748887.

Reason for revert: This change caused llvm-config{,-native} to be unable
to find static archives bundled with LLVM, as has been [reported]. Ever
since #152944 using moveToOutput in LLVM is _evil_ because llvm-config
obtains it knowledge about the installation locations from the CMake
configure step.

Consequently a change like #162607 will need to be implemented by making
LLVM itself install the static archives to the correct location or by
adding yet another patch which updates llvm-config's knowledge of the
location. The latter is not desireable in my opinion, though, since it
is just asking for this sort of trouble: Before #152944 we had an
outputs.patch that did this sort of things which broke spectacularly in
edge cases.

Fixes #148117.

[reported]: #148117 (comment)
github-actions bot pushed a commit that referenced this pull request Jul 5, 2022
Reverts #162607 / 1748887.

Reason for revert: This change caused llvm-config{,-native} to be unable
to find static archives bundled with LLVM, as has been [reported]. Ever
since #152944 using moveToOutput in LLVM is _evil_ because llvm-config
obtains it knowledge about the installation locations from the CMake
configure step.

Consequently a change like #162607 will need to be implemented by making
LLVM itself install the static archives to the correct location or by
adding yet another patch which updates llvm-config's knowledge of the
location. The latter is not desireable in my opinion, though, since it
is just asking for this sort of trouble: Before #152944 we had an
outputs.patch that did this sort of things which broke spectacularly in
edge cases.

Fixes #148117.

[reported]: #148117 (comment)

(cherry picked from commit a2ed5b2)
sternenseemann added a commit that referenced this pull request Jul 5, 2022
Reverts #162607 / 1748887.

Reason for revert: This change caused llvm-config{,-native} to be unable
to find static archives bundled with LLVM, as has been [reported]. Ever
since #152944 using moveToOutput in LLVM is _evil_ because llvm-config
obtains it knowledge about the installation locations from the CMake
configure step.

Consequently a change like #162607 will need to be implemented by making
LLVM itself install the static archives to the correct location or by
adding yet another patch which updates llvm-config's knowledge of the
location. The latter is not desireable in my opinion, though, since it
is just asking for this sort of trouble: Before #152944 we had an
outputs.patch that did this sort of things which broke spectacularly in
edge cases.

Fixes #148117.

[reported]: #148117 (comment)

(cherry picked from commit a2ed5b2)
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
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.

None yet

7 participants