Skip to content

nixos/qemu-vm: use cfg.host.pkgs#238596

Merged
Atemu merged 1 commit intoNixOS:masterfrom
nikstur:qemu-vm-use-host-pkgs
Jun 26, 2023
Merged

nixos/qemu-vm: use cfg.host.pkgs#238596
Atemu merged 1 commit intoNixOS:masterfrom
nikstur:qemu-vm-use-host-pkgs

Conversation

@nikstur
Copy link
Contributor

@nikstur nikstur commented Jun 19, 2023

Description of changes

Hopefully fixes at least part of the issue described here: #236656 (comment)

I'm not sure if all these instances need to be exchanged for cfg.host.pkgs but it seems right to me.

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.11 Release Notes (or backporting 23.05 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
  • 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: module (update) This PR changes an existing module in `nixos/` labels Jun 19, 2023
@nikstur nikstur requested a review from RaitoBezarius June 19, 2023 15:01
Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Thank you! This looks right (dunno about the closureInfo but I'll defer to you). I'll look into e2fsprogs myself soon.

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.

This looks good indeed! Thank you

@figsoda figsoda added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Jun 19, 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 Jun 19, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/darwin-builder-on-an-m1-mac-problems-with-mkfs-ext4/29523/3

@RaitoBezarius
Copy link
Member

We need a Darwin person to confirm this is fixing darwin.builder.

@Gabriella439
Copy link
Contributor

I confirmed locally that this PR fixes darwin.builder on aarch64-darwin. Specifically, I first tested that darwin.builder failed locally on master for the reason described in #236656 (comment) and then worked when I switched to this branch.

HOWEVER, I had to also wipe the old nixos.qcow2 image before it worked again.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/darwin-builder-on-an-m1-mac-problems-with-mkfs-ext4/29523/5

@nikstur
Copy link
Contributor Author

nikstur commented Jun 23, 2023

@Gabriella439 thank you for testing

HOWEVER, I had to also wipe the old nixos.qcow2 image before it worked again.

Sadly, I don't think there is anything we can do about that.

@emilazy
Copy link
Member

emilazy commented Jun 23, 2023

We need a Darwin person to confirm this is fixing darwin.builder.

It's still broken on x86_64 because of CoreFoundation rpath hook problems breaking e2fsprogs. I have a workaround for that but I'm waiting for the Darwin stdenv rework to see if it's necessary to PR it.

In other words, LGTM; this is more correct even if it doesn't fix the package on all platforms immediately.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

I was not able to run the created machine because it prints this error but creation works again which is what this PR's purpose is.

qemu-system-aarch64: -netdev user,id=user.0,hostfwd=tcp::22-:22,: Could not set up host forwarding rule 'tcp::22-:22'

@lilyball
Copy link
Member

HOWEVER, I had to also wipe the old nixos.qcow2 image before it worked again.

I noticed recently that you have to do this when changing the disk space option too. I really wish the qemu-vm stuff would recognize when something changes that requires wiping the image, but that seems orthogonal to this particular issue.

@emilazy
Copy link
Member

emilazy commented Jun 23, 2023

With #239473 maybe we can eliminate the persistent image altogether? But agreed that this PR should land as-is.

@nikstur nikstur force-pushed the qemu-vm-use-host-pkgs branch from ed51497 to f6b37dd Compare June 23, 2023 22:45
@nikstur
Copy link
Contributor Author

nikstur commented Jun 23, 2023

With #239473 maybe we can eliminate the persistent image altogether?

It's a little more complicated than this. There are multiple and different kinds of persistent images used in the QEMU module. The erofs PR would only lay the groundwork to get rid of one of them (the Nix Store image built with make-disk-image.nix).

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

LGTM.

Nit: The commit message could explain itself a bit better.

@mweinelt mweinelt added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Jun 25, 2023
@Atemu Atemu merged commit a04b45f into NixOS:master Jun 26, 2023
@Atemu
Copy link
Member

Atemu commented Jun 26, 2023

Should this be backported?

@emilazy
Copy link
Member

emilazy commented Jun 26, 2023

I think the changes that broke darwin.builder aren't in 23.05 so it's probably fine?

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: module (update) This PR changes an existing module in `nixos/` 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.approvals: 3+ This PR was reviewed and approved by three or more persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants