Skip to content

nixos/make-disk-image: fix contents dir paths#226214

Merged
RaitoBezarius merged 2 commits intoNixOS:masterfrom
leon-barrett:leon-make-disk-image-dir
Apr 16, 2023
Merged

nixos/make-disk-image: fix contents dir paths#226214
RaitoBezarius merged 2 commits intoNixOS:masterfrom
leon-barrett:leon-make-disk-image-dir

Conversation

@leon-barrett
Copy link

Description of changes

make-disk-image is a tool for creating VM images. It takes an argument contents that allows one to specify files and directories that should be copied into the VM image. However, directories do not end up at the specified target, but instead at a subdirectory of the target, with a nix-store-like path, e.g. /target/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-source. See issue #226203 .

This change adds a test for make-disk-image's contents directory handling and adds a fix (appending / to rsync input directory names).

This closes issue #226203 .

Things done

I added some checks in nixosTests.image-contents and ran it on an x86_64 machine. I also ran nixosTests.qemu-vm-restrictnetwork.

I did not run these relevant-looking tests, however, because they had issues on master:

  • nixosTests.ec2-config (not run because it logged about something broken on master)
  • nixosTests.ec2-nixops (not run because it failed on master)
  • nixosTests.sourcehut (not tested because it did not complete in 10 minutes on master)
  • nixosTests.systemd-repart (not tested because it failed on master)
  • 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
    • Added a release notes entry for this library function fix. (I'm not sure if this is useful.)
  • 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: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Apr 14, 2023
@ofborg ofborg bot added 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. labels Apr 14, 2023
Copy link
Member

Choose a reason for hiding this comment

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

Modification of this file should go in a separate commit to keep clean history

Copy link
Author

Choose a reason for hiding this comment

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

I moved these changes to another commit in the same PR. Should I move them to another PR?

Copy link
Member

Choose a reason for hiding this comment

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

No, but the commit should be formatted using a path name: nixos/tests/ec2 is better

Copy link
Author

Choose a reason for hiding this comment

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

Okay, thanks, I wasn't sure quite how to format it.

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

Minor changes are necessary, but LGTM other than that, thank you!

@leon-barrett leon-barrett force-pushed the leon-make-disk-image-dir branch from 6dde3e6 to 972e102 Compare April 16, 2023 00:32
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

Minor change on the commit formatting for EC2 and we are good to go!

This change fixes two problems with the qemu testing code:
1. Previously, the qemu-img command was missing a disk image format
   argument.
2. Previously, if a test assertion failed, the test hung because the VM
   was not torn down.
`make-disk-image` is a tool for creating VM images. It takes an argument
`contents` that allows one to specify files and directories that should
be copied into the VM image. However, directories end up not at the
specified target, but instead at a subdirectory of the target, with a
nix-store-like path, e.g.
`/target/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-source`. See issue
NixOS#226203 .

This change adds a test for make-disk-image's contents directory
handling and adds a fix (appending `/` to rsync input directory names).

This closes issue NixOS#226203 .
@leon-barrett leon-barrett force-pushed the leon-make-disk-image-dir branch from 972e102 to 15c760d Compare April 16, 2023 16:55
@leon-barrett
Copy link
Author

Thanks!

@leon-barrett leon-barrett deleted the leon-make-disk-image-dir branch April 16, 2023 22:27
@Janik-Haag Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 13, 2023
@ghpzin ghpzin mentioned this pull request Aug 29, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 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. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants