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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from
Dec 26, 2022
Merged

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

merged 2 commits into from
Dec 26, 2022

Conversation

RaitoBezarius
Copy link
Member

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.

@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
@RaitoBezarius RaitoBezarius mentioned this pull request Dec 20, 2022
Merged
13 tasks
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 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.

doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
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.

doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
doc/builders/images/makediskimage.section.md Outdated Show resolved Hide resolved
@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
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

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

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 6.topic: nixos 6.topic: policy discussion 8.has: changelog 8.has: documentation 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@RaitoBezarius @Mic92 @blitz @Lassulus @nikstur