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

Jump to bottom

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
Merged

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

merged 5 commits into from Jun 23, 2020

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Jun 20, 2020 •
edited

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 extlinux-conf-builder-dtbname branch from 8a751f9 to 8c14ada Compare June 20, 2020 21:56
@flokli flokli force-pushed the extlinux-conf-builder-dtbname branch 2 times, most recently from d45a30f to ad3725e Compare June 20, 2020 23:08
samueldr
samueldr approved these changes Jun 21, 2020
View reviewed changes
Copy link
Member

@samueldr samueldr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@flokli
nixos/deviceTree: fix description
4342468 
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 extlinux-conf-builder-dtbname branch from ad3725e to 7dfb841 Compare June 21, 2020 09:32
@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
flokli
flokli commented Jun 21, 2020
View reviewed changes
nixos/modules/system/boot/loader/generic-extlinux-compatible/default.nix Outdated
@@ -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; };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
nixos/generic-extlinux-compatible: introduce boot.loader.generic-extl…
54129e7 
…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)
@flokli
nixos/sd-image-*: use boot.loader.generic-extlinux-compatible.populat…
afa6277 
…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.
@flokli
extlinux-conf-builder.sh: allow a custom FDT to be specified
bd8137a 
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.
@flokli
hardware.deviceTree: add name
387f3b5 
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 extlinux-conf-builder-dtbname branch from 7dfb841 to 387f3b5 Compare June 21, 2020 11:49
@flokli
Copy link
Contributor Author

flokli commented Jun 21, 2020 •
edited

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

@flokli flokli requested a review from samueldr June 22, 2020 21:32
samueldr
samueldr approved these changes Jun 22, 2020
View reviewed changes
Copy link
Member

@samueldr samueldr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 June 22, 2020 22:42
@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
@flokli flokli deleted the extlinux-conf-builder-dtbname branch June 23, 2020 16:07
@B4dM4n B4dM4n mentioned this pull request Jun 23, 2020
Merged
10 tasks
flokli added a commit to flokli/pynq that referenced this pull request Dec 5, 2022
@flokli
nix/sources.json: bump nixpkgs
7ba762e 
This contains two more PRs currently staged for nixpkgs:

 - nixos/make-ext4-fs: increase fudge factor from 1.03 to 1.10
   NixOS/nixpkgs#91214
 - extlinux-conf-builder: allow a custom FDT to be specified
   NixOS/nixpkgs#91195

The former one is needed to have the initial sd image be big enough to
be able to resize itself, the latter to circumvent u-boot not properly
detecting our board, and trying to load the wrong FDT.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samueldr samueldr samueldr approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@flokli @Ericson2314 @samueldr