Skip to content

nixos/systemd-boot: Add extraEntries and extraFiles options#150408

Merged
pennae merged 5 commits intoNixOS:masterfrom
Enzime:systemd-boot-extra-entries
Jan 13, 2022
Merged

nixos/systemd-boot: Add extraEntries and extraFiles options#150408
pennae merged 5 commits intoNixOS:masterfrom
Enzime:systemd-boot-extra-entries

Conversation

@Enzime
Copy link
Copy Markdown
Member

@Enzime Enzime commented Dec 12, 2021

Motivation for this change

These options allow people to add their own EFI files and corresponding entries to systemd-boot.

I have also added boot.loader.systemd-boot.netbootxyz options to allow people to easily add netboot.xyz to their boot menu.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@Enzime Enzime requested a review from dasJ as a code owner December 12, 2021 11:52
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 12, 2021
@Enzime Enzime force-pushed the systemd-boot-extra-entries branch from 85dd512 to f0c1000 Compare December 12, 2021 12:06
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 12, 2021
pennae
pennae reviewed Dec 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@pennae pennae left a comment •
edited
Loading

Choose a reason for hiding this comment

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

is it feasible to add a nixos test to ensure the generated boot menu is functional and extras are cleaned up properly? looks pretty neat otherwise apart from a few nitpicks.

pennae
pennae reviewed Dec 18, 2021
View reviewed changes
@Enzime Enzime force-pushed the systemd-boot-extra-entries branch 2 times, most recently from ee82412 to f68cac0 Compare December 19, 2021 04:57
pennae
pennae reviewed Dec 19, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@pennae pennae left a comment

Choose a reason for hiding this comment

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

looks good now, just one thing we missed on the first go and one suggestion :)

@Enzime Enzime force-pushed the systemd-boot-extra-entries branch from f68cac0 to a10c456 Compare December 23, 2021 00:36
@Enzime Enzime force-pushed the systemd-boot-extra-entries branch from a10c456 to 2f0cfde Compare December 23, 2021 00:44
pennae
pennae approved these changes Dec 23, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@pennae pennae left a comment

Choose a reason for hiding this comment

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

tested locally, looks good!

@Enzime
nixos/nixos-enter: Don't passthru TMPDIR
1783cfd 
`mktemp` tries to use the `TMPDIR` from `nixos-install` outside of the
`chroot` instead of `/tmp` inside the `chroot` and fails. For some
reason the `TMPDIR` is being passed through the `chroot` call.

I haven't tested if other environment variables are being passed through
that shouldn't be.
@Enzime
Copy link
Copy Markdown
Member Author

Enzime commented Dec 30, 2021

@pennae you might be interested in taking a look at the newest commit I just made. Found this bug when doing a fresh install of NixOS using this branch 😄

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Jan 11, 2022

ping cc @pennae

pennae
pennae reviewed Jan 11, 2022
View reviewed changes
nixos/modules/system/boot/loader/systemd-boot/systemd-boot.nix
netbootxyz = if cfg.netbootxyz.enable then pkgs.netbootxyz-efi else "";

copyExtraFiles = pkgs.writeShellScript "copy-extra-files" ''
empty_file=$(mktemp)
Copy link
Copy Markdown
Contributor

@pennae pennae Jan 11, 2022

Choose a reason for hiding this comment

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

nitpick if you're up to it: we've since learned about the pkgs.emptyFile trivial builder. might be better to use that, and solve the TMPDIR problem with the installer?

apart from that, LGTM

Copy link
Copy Markdown
Member Author

@Enzime Enzime Jan 11, 2022

Choose a reason for hiding this comment

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

nixpkgs/nixos/modules/installer/tools/nixos-enter.sh

Lines 103 to 104 in 1783cfd

# Create /tmp
chroot "$mountPoint" systemd-tmpfiles --create --remove --exclude-prefix=/dev 1>&2 || true

I think the TMPDIR needs to be fixed irrespective of whether this PR uses it or not, as the nixos-enter script creates /tmp but then won't even use it if TMPDIR is set.

My current preference is to keep mktemp to dogfood the nixos-enter command :)

Copy link
Copy Markdown
Contributor

@pennae pennae Jan 12, 2022

Choose a reason for hiding this comment

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

how did you find that? just tested an installer run (on 21.11 though, not unstable) and found that TMPDIR wasn't set.

Copy link
Copy Markdown
Member Author

@Enzime Enzime Jan 12, 2022

Choose a reason for hiding this comment

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

I ran the installer off this PR

TMPDIR is set in nixos-install

nixpkgs/nixos/modules/installer/tools/nixos-install.sh

Lines 137 to 142 in 1783cfd

# A place to drop temporary stuff.
tmpdir="$(mktemp -d -p "$mountPoint")"
trap 'rm -rf $tmpdir' EXIT
# store temporary files on target filesystem by default
export TMPDIR=${TMPDIR:-$tmpdir}

Which then calls nixos-enter:

nixpkgs/nixos/modules/installer/tools/nixos-install.sh

Line 198 in 1783cfd

NIXOS_INSTALL_BOOTLOADER=1 nixos-enter --root "$mountPoint" -- /run/current-system/bin/switch-to-configuration boot

I'm guessing this bug wasn't found because nothing else was using TMPDIR

Copy link
Copy Markdown
Contributor

@pennae pennae Jan 13, 2022

Choose a reason for hiding this comment

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

interactions between different sides of the same chroot sure can be confusing. 👍

@pennae pennae merged commit 466cb74 into NixOS:master Jan 13, 2022
@Enzime Enzime deleted the systemd-boot-extra-entries branch January 13, 2022 07:15
@edrex edrex mentioned this pull request Feb 12, 2022
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pennae pennae pennae approved these changes

@dasJ dasJ Awaiting requested review from dasJ

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Enzime @Mic92 @pennae