linux: assorted build cleanups #187252
linux: assorted build cleanups #187252
Conversation
| "INSTALL_PATH=$(out)" | ||
| ] ++ (optional isModular "INSTALL_MOD_PATH=$(out)") | ||
| ++ optional installsFirmware "INSTALL_FW_PATH=$(out)/lib/firmware" | ||
| ++ optionals buildDTBs ["dtbs_install" "INSTALL_DTBS_PATH=$(out)/dtbs"]; | ||
|
|
||
| preInstall = '' | ||
| installFlagsArray+=("-j$NIX_BUILD_CORES") | ||
|
|
||
| # the install scripts expect to find installkernel in ~/bin/installkernel | ||
| export HOME=${installkernel} |
There was a problem hiding this comment.
If I understand correctly, this replaces INSTALLKERNEL=.... Is there a reason this is better? It feels like setting HOME because something else somewhere expects the path to be just right is very brittle. On the other hand INSTALLKERNEL=${installkernel} seems pretty explicit.
There was a problem hiding this comment.
The previous solution only worked by accident. The upstream install scripts look for ~/bin/${INSTALLKERNEL} and /sbin/${INSTALLKERNEL}, which after the mass-replacement turned into ${HOME}${INSTALLKERNEL} and ${INSTALLKERNEL} respectively, and the latter happened to work with what we were passing in. We could patch the path directly into the install scripts, but it's declared in different locations in different kernel releases, so this is easier.
There was a problem hiding this comment.
Could we make this a bit clearer by adding the reasoning in the comment, also linking to e.g. the current LTS's installkernel usage as an example of why this is needed? I'm betting in 1+ year the next person looking into this will be confused too :).
Note that because of legacy kernel requirements, I agree that this solution is not too bad, since it is all executing under the context of a single derivation build.
There was a problem hiding this comment.
I am very much hoping we will not have this in 1+ year, but sure, I'll add a comment.
There was a problem hiding this comment.
Removed. Better now than in 1+ year.
There was a problem hiding this comment.
Unremoved. The obvious thing doesn't work.
| @@ -114,10 +114,6 @@ let | |||
| ++ optional (lib.versionAtLeast version "5.2" && lib.versionOlder version "5.4") ./gen-kheaders-metadata.patch; | |||
|
|
|||
| prePatch = '' | |||
There was a problem hiding this comment.
This should be done in postPatch because it can break upstream patches.
There was a problem hiding this comment.
I think this is intentional, but I don't understand it enough to touch it right now.
There was a problem hiding this comment.
not sure about the rest but this bit needs to be done pre
There was a problem hiding this comment.
If a patch changes the shebang in scripts/ld-version.sh or a line below it, it will fail to apply. A better approach would be to conditionally patch the shebang or allow it to fail.
There was a problem hiding this comment.
Let's not change the established semantics of what is basically out of scope of the changes. This implementation has a lot of baggage from being really ancient.
If you believe it should be done, you're invited to open a new PR aimed at cleaning up the nits from the derivation.
|
Some more testing results: all the x86_64 kernels build, latest and LTS also build on aarch64 native and x86_64 to aarch64 cross. |
This whole thing is utterly broken, but let's start here. Should fix intermittent build failures with patchShebangs.
|
The device tree changes might be a bit out of scope for this as well, but they cause merge conflicts otherwise. If anyone wants to specifically merge the scripts stuff and not the DT stuff, I can split it out. |
K900
changed the title
| # by the makefile overlords for our own nefarious ends. | ||
| # | ||
| # Note that the makefiles specifically look in ~/bin/installkernel, and | ||
| # writeShellScriptBin writes the script to <store path>/bin/installkernel, |
There was a problem hiding this comment.
Now this is a good comment! I appreciate you writing it.
|
I want to merge this later tonight so I can catch any potential fallout tomorrow. Any objections? |
|
This broke build of some kernels, so I'd suggest prompt fix or reverting for now. Example: https://hydra.nixos.org/build/188038089 |
|
Investigating. |
This whole thing is utterly broken, but let's start here.
Should fix intermittent build failures with patchShebangs.
Split out from #187138.
Description of changes
Stop doing horrible things to makefiles. Manually audited changes to make sure they don't actually matter.
See also: torvalds/linux@master...K900:linux:insane for a 5.19 tree with two commits: one that applies the "fixup" as-is, and one that reverts everything that produces results that are objectively wrong. The final diff is the changes that we might want to actually make, audited again to make sure that code should not be hit under normal circumstances.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes