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

extlinux-conf-builder: expose and use base builder command, allow a custom FDT to be specified #91195

Merged
merged 5 commits into from Jun 23, 2020

Conversation

@flokli
Copy link
Contributor

flokli commented Jun 20, 2020

Some bootloaders might not properly detect the model.
If the specific model is known by configuration, provide a way to
explicitly point to a specific dtb in the extlinux.conf.

This adds hardware.deviceTree.name, uses it from the generic-extlinux-compatible module, and exposes the builder command generated there as a read-only NixOS option, which is now used by the sd-image-*.nix files - allowing us to get rid of all the additional imports there, too.

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.

cc @NixOS/exotic-platform-maintainers

@flokli flokli force-pushed the flokli:extlinux-conf-builder-dtbname branch from 8a751f9 to 8c14ada Jun 20, 2020
@flokli flokli force-pushed the flokli:extlinux-conf-builder-dtbname branch 2 times, most recently from d45a30f to ad3725e Jun 20, 2020
Copy link
Member

samueldr left a comment

I haven't run the code, but the author verifiably did, successfully, and it looks all right.

hardware.deviceTree.base points to a path, not a package (and also if of
types.path)

It defaults to ${config.boot.kernelPackages.kernel}/dtbs.
@flokli
Copy link
Contributor Author

flokli commented Jun 21, 2020

I realized this currently only fixes half of the problem - while one would now be able to pass this to the exlinux builder, the generic-extlinux-compatible is in charge of invoking the builder on the next configuration switch - and it doesn't know about the custom fdt specified, so it'd remove that option again, which is most likely not what we want.

I added the necessary option as hardware.deviceTree.name, use it from the generic-extlinux-compatible module, and expose the builder command generated there as a read-only NixOS option, which is now used by the sd-image-*.nix files - allowing us to get rid of all the additional imports there, too.

@flokli flokli force-pushed the flokli:extlinux-conf-builder-dtbname branch from ad3725e to 7dfb841 Jun 21, 2020
@flokli
Copy link
Contributor Author

flokli commented Jun 21, 2020

I pushed an updated version, containing some drive-by fixes, the refactor, and the addition of the fdt option, please take another look :-)

@flokli flokli changed the title extlinux-conf-builder: allow a custom FDT to be specified extlinux-conf-builder: expose and use base builder command, allow a custom FDT to be specified Jun 21, 2020
@@ -8,7 +8,7 @@ let

timeoutStr = if blCfg.timeout == null then "-1" else toString blCfg.timeout;

builder = import ./extlinux-conf-builder.nix { inherit pkgs; };
builder = import ./extlinux-conf-builder.nix { pkgs = pkgs.buildPackages; };

This comment has been minimized.

Copy link
@flokli

flokli Jun 21, 2020

Author Contributor

This is still not correct. While this now succeeds in assembling the image, switching at runtime won't work.

I feel like the one we expose for image generation should set pkgs = pkgs.buildPackages) (and should probably be named to reflect that), but we still need to have a builderusing the native packages inside this module (as that generates the line in the activation script, which runs on the board architecture).

This comment has been minimized.

Copy link
@flokli

flokli Jun 21, 2020

Author Contributor

I renamed the option to populateCmd, so it's usecase is clearer, and explicitly distinguished between the builder used during activation and the builder used to populate a (possibly cross) image.

I prefer this over just exposing the arguments - then every sd-image-* would then need to still import the builder.

flokli added 4 commits Jun 21, 2020
…inux-compatible.populateCmd

This option exposes the builder command used to populate an image,
honoring all options except the -c <path-to-default-configuration>
argument.

Useful to have for sdImage.populateRootCommands.

Special care needs to be taken w.r.t cross - the populate command runs
on the host platform, the activation script on the build platform (so
the builders differ)
…eCmd

While getting rid of the separate extlinux-conf-builder import, this now
also honors boot.loader.timeout in the initial sd card image if
specified.
Some bootloaders might not properly detect the model.
If the specific model is known by configuration, provide a way to
explicitly point to a specific dtb in the extlinux.conf.
This can be used to explicitly specify a specific dtb file, relative to
the dtb base.

Update the generic-extlinux-compatible module to make use of this option.
@flokli flokli force-pushed the flokli:extlinux-conf-builder-dtbname branch from 7dfb841 to 387f3b5 Jun 21, 2020
@flokli
Copy link
Contributor Author

flokli commented Jun 21, 2020

This now also works - I successfully switched configurations and built cross sd images with this module. PTAL.

@flokli flokli requested a review from samueldr Jun 22, 2020
Copy link
Member

samueldr left a comment

I haven't run the latest changes, but it all looks fine and I trust @flokli as having ran this correctly.

The newly added option (hardware.deviceTree.name) may be useful in other situations too.

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 22, 2020

I....totally don't know about this stuff. But maybe the other @NixOS/exotic-platform-maintainers do and can review instead (I see you already CCed).

@Ericson2314 Ericson2314 removed their request for review Jun 22, 2020
@flokli
Copy link
Contributor Author

flokli commented Jun 23, 2020

Okay, let's merge this :-)

@flokli flokli merged commit d227d81 into NixOS:master Jun 23, 2020
14 checks passed
14 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="387f3b5"; rev="387f3b58d2cfe131700a3c40541665c1dfde1bbb"; } ./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="387f3b5"; rev="387f3b58d2cfe131700a3c40541665c1dfde1bbb"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="387f3b5"; rev="387f3b58d2cfe131700a3c40541665c1dfde1bbb"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="387f3b5"; rev="387f3b58d2cfe131700a3c40541665c1dfde1bbb"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="387f3b5"; rev="387f3b58d2cfe131700a3c40541665c1dfde1bbb"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="387f3b5"; rev="387f3b58d2cfe131700a3c40541665c1dfde1bbb"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="387f3b5"; rev="387f3b58d2cfe131700a3c40541665c1dfde1bbb"; } ./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
@flokli flokli deleted the flokli:extlinux-conf-builder-dtbname branch Jun 23, 2020
@B4dM4n B4dM4n mentioned this pull request Jun 23, 2020
3 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.