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

makeModulesClosure: handle builtin modules better #92081

Merged
merged 1 commit into from Aug 13, 2020

Conversation

@CrystalGamma
Copy link
Contributor

CrystalGamma commented Jul 2, 2020

Rewrite the closure computation and copying.

Motivation for this change

The previous code discarded entire dependency trees if the first entry in the dependency list compiled by modprobe --show-depends is a builtin and otherwise handled its output in a rather hackish way.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@xaverdh
Copy link
Contributor

xaverdh commented Jul 2, 2020

While at it, can we also handle module arguments properly (in the spirit of https://github.com/NixOS/nixpkgs/pull/66235/files)?
Currently they are detected as separate modules. It should amount to throwing away any additional input from the first whitespace in a module name (I hope kernel modules are not allowed to contain whitespace..).

@CrystalGamma
Copy link
Contributor Author

CrystalGamma commented Jul 2, 2020

@xaverdh should be just another dummy variable in the read command, so yes … I don't know how to make it emit arguments though, because I'm not aware of a module name that produces arguments …

@xaverdh
Copy link
Contributor

xaverdh commented Jul 3, 2020

@xaverdh should be just another dummy variable in the read command, so yes … I don't know how to make it emit arguments though, because I'm not aware of a module name that produces arguments …

Well if you set a module parameter in the kernel command line (e.g. via boot.kernelParams) that shows up in the modprobe output. E.g. something like boot.kernelParams = [ "i915.enable_psr=1" ] should reproduce the issue if you have intel hardware.

update: I can confirm that with your pr and an additional parameter to read my issue is fixed.
update2: To be more specific, the error to look for is something like modinfo: ERROR: Module alias enable_psr=1 not found.

The previous code discarded entire dependency trees if the first entry in the dependency list compiled by `modprobe --show-depends` is a builtin and otherwise handled its output in a rather hackish way.
@CrystalGamma CrystalGamma force-pushed the CrystalGamma:modules-closure branch from 89620df to b155b1d Jul 16, 2020
@xaverdh
Copy link
Contributor

xaverdh commented Jul 17, 2020

Thanks! Works on my system = )
Can we get this reviewed somehow? Maybe abbradar wants to have a look at it (since he originally wrote those lines), or we could try marvin for setting the status of this pr.

@lheckemann
Copy link
Member

lheckemann commented Aug 11, 2020

@ofborg test boot

Copy link
Member

lheckemann left a comment

Overall LGTM, I'd consider my array suggestion optional. Since the tests have passed, I'm all for merging once the array thing has been addressed (even if it's with a "no" ;) )

@@ -19,36 +19,55 @@ version=$(cd $kernel/lib/modules && ls -d *)
echo "kernel version is $version"

# Determine the dependencies of each root module.
closure=
mkdir -p $out/lib/modules/"$version"
touch closure

This comment has been minimized.

@lheckemann

lheckemann Aug 11, 2020 Member

Using an array could be nicer here, since it avoids word-splitting "fun":

closure=()
[…]
closure+=("$module")
[…]
for module in "${closure[@]}"; do […]

This comment has been minimized.

@CrystalGamma

CrystalGamma Aug 17, 2020 Author Contributor

The reason I used a file here was because the while read loop seems to run in a subshell, so code afterwards can't use any variables that are set in the loop. I personally don't have experience with arrays in Bourne-based shells (I use fish where I can) but I would assume the same holds for them. Anyway, thanks for merging. :)

This comment has been minimized.

@lheckemann

lheckemann Aug 17, 2020 Member

Aah, I see. Yeah, you can work around that by making what's piped into while run in the subshell instead, as in…

while read cmd module args ; do
  […]
done < <(modprobe --config no-config -d $kernel --set-version "$version" --show-depends "$module")

but that does make reading it a little more confusing. Alternatively:

exec 5< <(modprobe --config no-config -d $kernel --set-version "$version" --show-depends "$module")
while read -u 5 cmd module args ; do
  […]
done

but yeah I'd say leave it as is now :)

@lheckemann lheckemann merged commit efc739f into NixOS:master Aug 13, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="b155b1d"; rev="b155b1dafb52b4b9e352da2da9bf914be29a325a"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="b155b1d"; rev="b155b1dafb52b4b9e352da2da9bf914be29a325a"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="b155b1d"; rev="b155b1dafb52b4b9e352da2da9bf914be29a325a"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="b155b1d"; rev="b155b1dafb52b4b9e352da2da9bf914be29a325a"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="b155b1d"; rev="b155b1dafb52b4b9e352da2da9bf914be29a325a"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="b155b1d"; rev="b155b1dafb52b4b9e352da2da9bf914be29a325a"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="b155b1d"; rev="b155b1dafb52b4b9e352da2da9bf914be29a325a"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
tests.boot on aarch64-linux Success
Details
tests.boot on x86_64-linux Success
Details
@xaverdh
Copy link
Contributor

xaverdh commented Aug 13, 2020

Thanks for finally making some progress on this!

@baloo baloo mentioned this pull request Aug 22, 2020
1 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.