Skip to content

make-disk-image: documentation, UEFI variables recording, improved determinism#207038

Merged
Mic92 merged 2 commits intomasterfrom
make-disk-image-for-uefi
Dec 26, 2022
Merged

make-disk-image: documentation, UEFI variables recording, improved determinism#207038
Mic92 merged 2 commits intomasterfrom
make-disk-image-for-uefi

Conversation

@RaitoBezarius
Copy link
Member

@RaitoBezarius RaitoBezarius commented Dec 20, 2022

Description of changes

This is a split of #203641 to isolate make-disk-image changes.

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/)
  • 23.05 Release Notes (or backporting 22.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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation labels Dec 20, 2022
@RaitoBezarius RaitoBezarius force-pushed the make-disk-image-for-uefi branch 2 times, most recently from b07ba3c to 28c1494 Compare December 20, 2022 20:15
@ofborg ofborg bot added 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 20, 2022
@RaitoBezarius RaitoBezarius mentioned this pull request Dec 20, 2022
Merged
13 tasks
Mic92
Mic92 reviewed Dec 20, 2022
View reviewed changes
Mic92
Mic92 approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

Overall nice change. Documentation is well written. I have not tested the code yet but from eye balling it looks good to me.

@RaitoBezarius RaitoBezarius force-pushed the make-disk-image-for-uefi branch from 28c1494 to 16b60be Compare December 20, 2022 23:02
@github-actions github-actions bot added the 8.has: changelog This PR adds or changes release notes label Dec 20, 2022
@RaitoBezarius RaitoBezarius marked this pull request as ready for review December 20, 2022 23:15
@RaitoBezarius RaitoBezarius mentioned this pull request Dec 21, 2022
Closed
20 tasks
nikstur
nikstur approved these changes Dec 23, 2022
View reviewed changes
Copy link
Contributor

@nikstur nikstur left a comment

Choose a reason for hiding this comment

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

LGTM

blitz
blitz suggested changes Dec 23, 2022
View reviewed changes
Copy link
Contributor

@blitz blitz left a comment

Choose a reason for hiding this comment

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

The added documentation is really nice! I left some language nits. Maybe you can find a native speaker to make a pass over it. I would also propose to move the description of the implementation to the .nix file, because it is not immediately helpful for people that just want to use this function.

I can help with the documentation part but I'm only back at the keyboard at 28th or so.

nikstur
nikstur reviewed Dec 24, 2022
View reviewed changes
Copy link
Contributor

@nikstur nikstur left a comment

Choose a reason for hiding this comment

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

Here are my invited docs nitpicks. Feel free to ignore them. We can always easily fix such things later.

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 24, 2022
@RaitoBezarius RaitoBezarius force-pushed the make-disk-image-for-uefi branch from 277e38f to b497110 Compare December 24, 2022 18:12
@github-actions github-actions bot added the 6.topic: policy discussion Discuss policies to work in and around Nixpkgs label Dec 24, 2022
@RaitoBezarius RaitoBezarius force-pushed the make-disk-image-for-uefi branch from b497110 to cc87bbd Compare December 24, 2022 18:13
@RaitoBezarius
nixos/lib/make-disk-image: docs, UEFI vars recording, more determinism
22adcaa 
- Extensive documentation in NixOS manual
- Deterministic mode that fixes various identifiers relative to disk
  partitions and filesystems in ext4 case
- UEFI variable recording
@RaitoBezarius RaitoBezarius force-pushed the make-disk-image-for-uefi branch from cc87bbd to 22adcaa Compare December 24, 2022 18:15
@ofborg ofborg bot added 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. and removed 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Dec 24, 2022
Lassulus
Lassulus reviewed Dec 24, 2022
View reviewed changes
nixos/lib/make-disk-image.nix
- creates an FAT32 ESP partition from 8MiB to specified `bootSize` parameter (256MiB by default), set it bootable ;
- creates a primary ext4 partition starting after the boot one and extending to the full disk image

This partition could be booted by a BIOS able to understand GPT layouts and recognizing the MBR at the start.
Copy link
Member

@Lassulus Lassulus Dec 24, 2022

Choose a reason for hiding this comment

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

hybrid should also be bootable via efi. it basically supports bios and efi (thats why it's hybrid) if your bootloader supports both (I guess only grub does both)

Copy link
Member Author

@RaitoBezarius RaitoBezarius Dec 24, 2022

Choose a reason for hiding this comment

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

I don't think I wrote something contrary to this, but good to write explicitly!

nikstur
nikstur approved these changes Dec 25, 2022
View reviewed changes
Copy link
Contributor

@nikstur nikstur left a comment

Choose a reason for hiding this comment

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

This is in very good shape now.

@Mic92
Copy link
Member

Mic92 commented Dec 26, 2022

Installer tests before:

⚠︎ vm-test-run-installer-bcache failed with exit code 1 after ⏱︎ 3m7s
✔︎ vm-test-run-installer-luksroot-format1 ⏱︎ 3m6s
✔︎ vm-test-run-installer-simple ⏱︎ 3m6s
✔︎ vm-test-run-installer-swraid ⏱︎ 3m5s
✔︎ vm-test-run-installer-btrfsSubvolEscape ⏱︎ 3m6s
✔︎ vm-test-run-installer-separateBoot ⏱︎ 3m6s
✔︎ vm-test-run-installer-zfs-root ⏱︎ 3m5s
✔︎ vm-test-run-installer-simpleUefiGrub ⏱︎ 3m5s
✔︎ vm-test-run-installer-bcachefs-simple ⏱︎ 3m7s
✔︎ vm-test-run-installer-encryptedFSWithKeyfile ⏱︎ 3m6s
✔︎ vm-test-run-installer-simpleProvided ⏱︎ 3m5s
✔︎ vm-test-run-installer-luksroot-format1 ⏱︎ 3m6s
✔︎ vm-test-run-installer-bcachefs-encrypted ⏱︎ 3m7s
✔︎ vm-test-run-installer-luksroot-format2 ⏱︎ 3m6s
✔︎ vm-test-run-installer-simpleUefiSystemdBoot ⏱︎ 3m5s
✔︎ vm-test-run-installer-separateBootFat ⏱︎ 3m6s
✔︎ vm-test-run-installer-simpleSpecialised ⏱︎ 3m5s
✔︎ vm-test-run-installer-simpleLabels ⏱︎ 3m6s
✔︎ vm-test-run-installer-grub1 ⏱︎ 3m6s
✔︎ vm-test-run-installer-btrfsSubvols ⏱︎ 3m6s
✔︎ vm-test-run-installer-simpleUefiGrubSpecialisation ⏱︎ 3m5s
✔︎ vm-test-run-installer-lvm ⏱︎ 3m6s
✔︎ vm-test-run-installer-btrfsSubvolDefault ⏱︎ 3m6s
✔︎ vm-test-run-installer-btrfsSimple ⏱︎ 3m6s
✔︎ vm-test-run-installer-bcachefs-multi ⏱︎ 3m7s

Installer tests after:

┃ ✔︎ vm-test-run-installer-simpleUefiSystemdBoot ⏱︎ 6m5s
┃ ✔︎ vm-test-run-installer-zfs-root ⏱︎ 6m8s
┃ ✔︎ vm-test-run-installer-swraid ⏱︎ 6m10s
┃ ✔︎ vm-test-run-installer-simpleUefiGrubSpecialisation ⏱︎ 6m10s
┃ ✔︎ vm-test-run-installer-simple ⏱︎ 6m10s
┃ ✔︎ vm-test-run-installer-simpleLabels ⏱︎ 6m10s
┃ ✔︎ vm-test-run-installer-separateBoot ⏱︎ 6m10s
┃ ✔︎ vm-test-run-installer-lvm ⏱︎ 6m10s
┃ ✔︎ vm-test-run-installer-simpleSpecialised ⏱︎ 6m10s
┃ ✔︎ vm-test-run-installer-simpleUefiGrub ⏱︎ 6m10s
┃ ✔︎ vm-test-run-installer-simpleProvided ⏱︎ 6m10s
┃ ✔︎ vm-test-run-installer-separateBootFat ⏱︎ 6m11s
┃ ✔︎ vm-test-run-installer-luksroot-format2 ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-luksroot-format1 ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-luksroot-format1 ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-grub1 ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-encryptedFSWithKeyfile ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-btrfsSubvols ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-btrfsSubvolEscape ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-btrfsSubvolDefault ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-btrfsSimple ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-bcachefs-simple ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-bcachefs-multi ⏱︎ 6m25s
┃ ⚠︎ vm-test-run-installer-bcachefs-encrypted failed with exit code 1 after ⏱︎ 6m25s
┃ ✔︎ vm-test-run-installer-bcache ⏱︎ 6m25s

No regressions.

@Mic92
Copy link
Member

Mic92 commented Dec 26, 2022

I also built a bunch of nixos-generator targets without issues. This is good to go.

@Mic92 Mic92 merged commit ea415d1 into master Dec 26, 2022
@Mic92 Mic92 deleted the make-disk-image-for-uefi branch December 26, 2022 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lassulus Lassulus Lassulus left review comments

@nikstur nikstur nikstur approved these changes

@copumpkin copumpkin Awaiting requested review from copumpkin

@edolstra edolstra Awaiting requested review from edolstra

@blitz blitz Awaiting requested review from blitz

@Mic92 Mic92 Awaiting requested review from Mic92

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@RaitoBezarius @Mic92 @blitz @Lassulus @nikstur