llvmPackages_{12,13,14,15,16,17,18,git}: Simplify argument passthrough #307211
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ofborg
bot
added
10.rebuild-darwin: 0
pwaller
force-pushed
the
llvm-simplify-arg-passthrough
branch
from
7d2f737
to
7819777
Compare
|
This refactor found a bug in LLVM 13, that monorepoSrc wasn't being consistently passed through. (Search for the term 'bug' in the patch). I preserved the bug during this refactor since we're in/approaching a stablisation phase I don't want to cause any rebuilds. In order to test this I generated the complete set of drvPaths for all attributes of Click to expand resulting attrset for `nixpkgs.llvmPkgs` @ 8753c7c (unchanged by this patch) |
|
/cc llvm maintainers @dtzWill @Ericson2314 @lovek323 @alyssais @RossComputerGuy @rrbutani @sternenseemann |
RossComputerGuy
approved these changes
Apr 29, 2024
wegank
added
the
12.approvals: 1
|
Thanks for the approval @RossComputerGuy. I don't have merge permissions, can someone please merge for me? |
wegank
added
the
2.status: merge conflict
This patch is not intended to introduce any functional change. drvPaths
should remain unchanged; if they do change, it's a bug.
Prior to this patch, a set of packages gets passed through from the
llvmPackages top level function to the individual packages via
callPackages, which is a newScope constructed with some specific
arguments.
As it stands this makes it harder to override dependencies of LLVM; for
example take ncurses. If you want to override it, it is an argument to
libllvm, however, if you override libllvm you then have to write a lot
of code to have correctly overridden clang, given how llvmPackages is
previously composed (out of tools and libraries).
Instead, I propose to make sure that all the dependencies of all
llvmPackages are listed as an inputs to the top leve llvmPackages,
and then the resulting newScope will contain all of them. This in
turn will make `llvmPackages.override` work as expected for any
input to each of the llvm packages.
We'll achieve this by first simplifying the code a bit and ensuring that
all arguments to llvmPackage get forwarded to all packages (via
`{}@args`).
This represents a chance to simplify things a bit so I propose doing it
in two steps:
1. This patch: Simplify argument pass through.
2. (Later): Ensure all arguments to each llvm package are listed in the
top level `llvm/X/default.nix`.
Once the second patch lands, this means that `(llvmPackages.override {
ncurses = myncurses; }).clang` would consist of a clang whose libllvm
had the ncurses overridden. This is not the case prior to this patch.
Signed-off-by: Peter Waller <[email protected]>
pwaller
force-pushed
the
llvm-simplify-arg-passthrough
branch
from
7819777
to
8164fd6
Compare
|
Rebased. I don't have merge permissions, please can someone merge? |
ofborg
bot
removed
the
2.status: merge conflict
rrbutani
added
the
6.topic: llvm/clang
wegank
removed
the
12.approvals: 1
RossComputerGuy
approved these changes
Jun 16, 2024
RossComputerGuy
added
the
12.approvals: 1
|
Successfully created backport PR for |
1 task
13 tasks
pwaller
added a commit
to pwaller/nixpkgs
that referenced
this pull request
Jun 16, 2024
... consistently. Further to NixOS#307211, allow overriding arguments through llvmPackages.override. This makes it possible to override any dependency of LLVM or clang by overriding it on `llvmPackages.override { <dependency> = ...; }`. This is useful in development or customization where sometimes it is desirable to turn features on or off. Without this patch the only way to for example override ncurses was to do `overriddenLLVM = llvmPackages.llvm.override { ncurses }`, but then you would have to thread `overriddenLLVM` as dependencies into clang and other packages, which results in quite a difficult expression to write correctly in cross compilation scenarios. Signed-off-by: Peter Waller <[email protected]>
github-actions bot
pushed a commit
that referenced
this pull request
Jun 20, 2024
... consistently. Further to #307211, allow overriding arguments through llvmPackages.override. This makes it possible to override any dependency of LLVM or clang by overriding it on `llvmPackages.override { <dependency> = ...; }`. This is useful in development or customization where sometimes it is desirable to turn features on or off. Without this patch the only way to for example override ncurses was to do `overriddenLLVM = llvmPackages.llvm.override { ncurses }`, but then you would have to thread `overriddenLLVM` as dependencies into clang and other packages, which results in quite a difficult expression to write correctly in cross compilation scenarios. Signed-off-by: Peter Waller <[email protected]> (cherry picked from commit 40a7f21)
13 tasks
sternenseemann
added a commit
to sternenseemann/nixpkgs
that referenced
this pull request
Oct 11, 2024
This change implements a leftover task from NixOS#307211, namely passing monorepoSrc to the different llvmPackages_13 package expressions. Before this change, all packages llvmPackages_13 would be built from a subdirectory of the full LLVM monorepo tree. After this change only the relevant directories are made available at build time. This - reduces the size of the source that needs to be made available to the builder. - prevents LLVM from sidestepping our instructions and including extra sources from other directories it shouldn't. Since LLVM 12 and 13 don't have the `cmake` directory at the top level, the runCommand expressions filtering the source need to be adjusted, but this causes no rebuild for any other LLVM version (ofborg should confirm this). The only problem encountered was in lld: 1. We need to make the patch to the inclusion of libunwind headers unconditional now. lld needs this on non-darwin as well. In the full monorepo, LLVM_MAIN_SRC_DIR would be set, so the patch wasn't necessary. 2. For some reason, the component is built from a different directory in LLVM 12 and 13 so we can't unify postPatch between 12 and 13, unfortunately. Change was tested by building the following expression on x86_64-linux. with import ./. {}; builtins.removeAttrs llvmPackages_13 [ "lldb" "lldbPlugins" ]' lld was also tested on aarch64-darwin.
sternenseemann
added a commit
to sternenseemann/nixpkgs
that referenced
this pull request
Oct 12, 2024
This change implements a leftover task from NixOS#307211, namely passing monorepoSrc to the different llvmPackages_13 package expressions. Before this change, all packages llvmPackages_13 would be built from a subdirectory of the full LLVM monorepo tree. After this change only the relevant directories are made available at build time. This - reduces the size of the source that needs to be made available to the builder. - prevents LLVM from sidestepping our instructions and including extra sources from other directories it shouldn't. Since LLVM 12 and 13 don't have the `cmake` directory at the top level, the runCommand expressions filtering the source need to be adjusted, but this causes no rebuild for any other LLVM version (ofborg should confirm this). The only problem encountered was in lld: - We need to make the patch to the inclusion of libunwind headers unconditional now. lld needs this on non-darwin as well. In the full monorepo, LLVM_MAIN_SRC_DIR would be set correctly, so the patch wasn't necessary. - The substitute mechanism for LLVM 12 and 13 can't be unified yet since LLVM 12 still uses a non monorepo build, so we come up with a different LLVM_MAIN_SRC_DIR. Change was tested by building the following expression on x86_64-linux. with import ./. {}; builtins.removeAttrs llvmPackages_13 [ "lldb" "lldbPlugins" ]' lld was also tested on aarch64-darwin.
sternenseemann
added a commit
that referenced
this pull request
Oct 14, 2024
This change implements a leftover task from #307211, namely passing monorepoSrc to the different llvmPackages_13 package expressions. Before this change, all packages llvmPackages_13 would be built from a subdirectory of the full LLVM monorepo tree. After this change only the relevant directories are made available at build time. This - reduces the size of the source that needs to be made available to the builder. - prevents LLVM from sidestepping our instructions and including extra sources from other directories it shouldn't. Since LLVM 12 and 13 don't have the `cmake` directory at the top level, the runCommand expressions filtering the source need to be adjusted, but this causes no rebuild for any other LLVM version (ofborg should confirm this). The only problem encountered was in lld: - We need to make the patch to the inclusion of libunwind headers unconditional now. lld needs this on non-darwin as well. In the full monorepo, LLVM_MAIN_SRC_DIR would be set correctly, so the patch wasn't necessary. - The substitute mechanism for LLVM 12 and 13 can't be unified yet since LLVM 12 still uses a non monorepo build, so we come up with a different LLVM_MAIN_SRC_DIR. Change was tested by building the following expression on x86_64-linux. with import ./. {}; builtins.removeAttrs llvmPackages_13 [ "lldb" "lldbPlugins" ]' lld was also tested on aarch64-darwin.
yuanwang-wf
pushed a commit
to yuanwang-wf/nixpkgs
that referenced
this pull request
Oct 17, 2024
This change implements a leftover task from NixOS#307211, namely passing monorepoSrc to the different llvmPackages_13 package expressions. Before this change, all packages llvmPackages_13 would be built from a subdirectory of the full LLVM monorepo tree. After this change only the relevant directories are made available at build time. This - reduces the size of the source that needs to be made available to the builder. - prevents LLVM from sidestepping our instructions and including extra sources from other directories it shouldn't. Since LLVM 12 and 13 don't have the `cmake` directory at the top level, the runCommand expressions filtering the source need to be adjusted, but this causes no rebuild for any other LLVM version (ofborg should confirm this). The only problem encountered was in lld: - We need to make the patch to the inclusion of libunwind headers unconditional now. lld needs this on non-darwin as well. In the full monorepo, LLVM_MAIN_SRC_DIR would be set correctly, so the patch wasn't necessary. - The substitute mechanism for LLVM 12 and 13 can't be unified yet since LLVM 12 still uses a non monorepo build, so we come up with a different LLVM_MAIN_SRC_DIR. Change was tested by building the following expression on x86_64-linux. with import ./. {}; builtins.removeAttrs llvmPackages_13 [ "lldb" "lldbPlugins" ]' lld was also tested on aarch64-darwin.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
This patch is not intended to introduce any functional change. drvPaths
should remain unchanged; if they do change, it's a bug (note: this revealed
a bug in the derivation's source paths for LLVM 13 which I've preserved
to avoid a rebuild).
Prior to this patch, a set of packages gets passed through from the
llvmPackages top level function to the individual packages via
callPackages, which is a newScope constructed with some specific
arguments.
As it stands this makes it harder to override dependencies of LLVM; for
example take ncurses. If you want to override it, it is an argument to
libllvm, however, if you override libllvm you then have to write a lot
of code to have correctly overridden clang.
Instead, I propose to make sure that all the dependencies of all
llvmPackages are listed as an inputs to the top level llvmPackages,
and then the resulting newScope will contain all of them. This in
turn will make
llvmPackages.overridework as expected for anyinput to each of the llvm packages.
We'll achieve this by simplifying the code a bit and ensuring that all
arguments to llvmPackage get forwarded to all packages (via
{}@args).This represents a chance to simplify things a bit so I propose doing it
in two steps:
top level
llvm/X/default.nix.Once 2 lands, this means that
(llvmPackages.override { ncurses = myncurses; }).clangwould consist of a clang whose libllvm had thencurses overridden. This is not the case prior to this patch.
For v1 draft of this patch I have only fixed llvmPackages 18 to keep thefirst pass review simple. If others agree this is a good way forward, I
will fix up all the other llvmPackages revision.
(No longer draft after getting some preliminary private approval from a maintainer)
Signed-off-by: Peter Waller [email protected]
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.