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

make-iso9660-image: produce stable GPT disk GUID #74174

Merged
merged 1 commit into from Jul 20, 2020

Conversation

@raboof
Copy link
Contributor

raboof commented Nov 25, 2019

By generating a version-5 GUID based on $out (which contains
the derivation hash) and preventing isohybrid from overwriting
the GPT table (which already is populated correctly by xorriso).

Tested by booting from UEFI USB disk

Fixes #74047

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 nix-review --run "nix-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.
Notify maintainers

cc @

@raboof
Copy link
Contributor Author

raboof commented Nov 25, 2019

I guess the main thing to consider/test here is whether dropping --uefi from the isohybrid invocation is safe. It seems safe from my tests, but looking at the image with cfdisk it changed quite a lot (it is now 1 big partition instead of 2 separate ones). I'd love input on this!

@flokli flokli requested a review from samueldr Nov 26, 2019
@samueldr
Copy link
Member

samueldr commented Nov 26, 2019

From the Syslinux documentation (wiki)

Alternatively, the ISO 9660 production program "xorriso" can enhance its results by isohybrid, if an MBR template file from the local Syslinux installation is provided. The names of these files match the "isohdp[fp]x*.bin" pattern and are to be found in Syslinux under the "./[bios/]mbr" directory or installed as, for example, "/usr/lib/syslinux/isohdpfx.bin".
[...]
The resulting ISO image needs no further treatment by isohybrid tools.
https://wiki.syslinux.org/wiki/index.php?title=Isohybrid

It looks like the isohybrid from Syslinux can be removed entirely, as long as xorriso is given the appropriate files to embed.

There is also a section about UEFI documenting the proper invocation for xorriso.

@raboof
Copy link
Contributor Author

raboof commented Nov 26, 2019

It looks like the isohybrid from Syslinux can be removed entirely

Good find - looks like the needed xorriso flags were already being passed in. My laptop can indeed still boot from the iso-written-to-USBdisk with isohybrid skipped entirely. Updated the PR.

@raboof
Copy link
Contributor Author

raboof commented Nov 26, 2019

/cc @lostdj @bobvanderlinden who have worked on these parts of the script before, way back in 2014/2015 though ;)

@@ -117,15 +118,6 @@ xorriso="xorriso

$xorriso -output $out/iso/$isoName

if test -n "$usbBootable"; then

This comment has been minimized.

@bobvanderlinden

bobvanderlinden Nov 26, 2019 Contributor

I'm not sure whether we can remove isohybrid. It could very well be that the parameters passed to xorriso are already sufficient, but from what I remember there were quite a few systems we needed to support. Make sure to test this on a UEFI machine, a legacy bios machine and an OSX machine. Or is there some other way to verify the change doesn't affect these systems?

If we're indeed removing this, we might as well remove the usbBootable options from make-iso9660-image.nix. Might be better to do this in a separate PR.

This comment has been minimized.

@raboof

raboof Dec 30, 2019 Author Contributor

(I've since tested this both on UEFI, non-UEFI and OSX machines. agreed to do further refactoring in a separate PR)

@raboof
Copy link
Contributor Author

raboof commented Nov 26, 2019

Also successfully tested on an old HP Compaq gw687aw machine from USB.

So left to test:

  • OSX
  • Booting from an actual CD instead of USB?

For your testing convenience, https://we.tl/t-ON1G9XIYPG

@raboof
Copy link
Contributor Author

raboof commented Dec 3, 2019

I have burnt the above ISO to a DVD-R and verified I could boot from it both on an old non-UEFI system and on a recent UEFI system.

@raboof
Copy link
Contributor Author

raboof commented Dec 3, 2019

And I've tested booting from the ISO on an OSX machine as well, now, too. Held the 'C' button to boot from the CD. This put me into a grub shell (probably we held the 'C' too long?), but after giving the normal command on the grub prompt it successfully booted the installation.

@raboof raboof force-pushed the raboof:fix-74047-stable-gpt-disk-guid branch from 8072fc5 to 3827792 Dec 11, 2019
@raboof
Copy link
Contributor Author

raboof commented Dec 11, 2019

I have also tested that when applying this change together with #75484, nix-build ./nixos/release-combined.nix -A nixos.iso_minimal.x86_64-linux -I nixpkgs=~/nixpkgs-r13y --check now succeeds

@domenkozar
Copy link
Member

domenkozar commented Dec 11, 2019

@GrahamcOfBorg test installer

@raboof raboof force-pushed the raboof:fix-74047-stable-gpt-disk-guid branch from 3827792 to 59dff7d Jan 5, 2020
@raboof
Copy link
Contributor Author

raboof commented Jan 5, 2020

Since #75484 has now been merged I rebased & squashed this, and verified nix-build --check is still succeeding.

I think this should be good to go now?

@raboof
Copy link
Contributor Author

raboof commented Jan 5, 2020

@GrahamcOfBorg test installer

@nixos-discourse
Copy link

nixos-discourse commented Jan 8, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/105

@nixos-discourse
Copy link

nixos-discourse commented May 15, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/161

@raboof
Copy link
Contributor Author

raboof commented Jul 7, 2020

/marvin opt-in
/status awaiting_changes

@marvin-mk2
Copy link

marvin-mk2 bot commented Jul 7, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@raboof raboof force-pushed the raboof:fix-74047-stable-gpt-disk-guid branch from 59dff7d to 1bdc11b Jul 7, 2020
@nixos-discourse
Copy link

nixos-discourse commented Jul 15, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/what-are-your-goals-for-20-09/8035/7

@marvin-mk2 marvin-mk2 bot requested a review from timokau Jul 18, 2020
By generating a version-5 GUID based on $out (which contains
the derivation hash) and preventing isohybrid from overwriting
the GPT table (which already is populated correctly by xorriso).

Tested by:
* booting from USB disk on a UEFI system
* booting from USB disk on a non-UEFI system
* booting from CD on a UEFI system
* booting from CD on a non-UEFI system
* booting from CD on an OSX system

Also tested that "nix-build ./nixos/release-combined.nix -A
nixos.iso_minimal.x86_64-linux -I nixpkgs=~/nixpkgs-r13y --check"
now succeeds.

Fixes #74047
@raboof raboof force-pushed the raboof:fix-74047-stable-gpt-disk-guid branch from 1bdc11b to be006ea Jul 20, 2020
@raboof raboof requested a review from timokau Jul 20, 2020
Copy link
Member

timokau left a comment

Thanks!

@timokau timokau merged commit 830a8d6 into NixOS:master Jul 20, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="be006ea"; rev="be006eab1f3df27c405a02897d735768f180260f"; } ./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="be006ea"; rev="be006eab1f3df27c405a02897d735768f180260f"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="be006ea"; rev="be006eab1f3df27c405a02897d735768f180260f"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="be006ea"; rev="be006eab1f3df27c405a02897d735768f180260f"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="be006ea"; rev="be006eab1f3df27c405a02897d735768f180260f"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="be006ea"; rev="be006eab1f3df27c405a02897d735768f180260f"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="be006ea"; rev="be006eab1f3df27c405a02897d735768f180260f"; } ./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
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.

7 participants
You can’t perform that action at this time.