Skip to content

nixos/tests: drop LegacyStartCommand#291544

Merged
K900 merged 7 commits intoNixOS:masterfrom
K900:ovmf-oof
Feb 28, 2024
Merged

nixos/tests: drop LegacyStartCommand#291544
K900 merged 7 commits intoNixOS:masterfrom
K900:ovmf-oof

Conversation

@K900
Copy link
Contributor

@K900 K900 commented Feb 26, 2024 •
edited
Loading

Description of changes

Switches the remaining tests that use the LegacyStartCommand API to generate a start command in the test body and pass it to the driver.

All the affected tests are still working (at least on x86_64) after this change, except boot.ubootExtlinux, which hasn't worked before this change either.

The commits are roughly ordered as least objectionable to most objectionable, so if people really want to keep the existing API, I can move that to a separate change.

Also, gave all the installer/boot tests more RAM, because they are otherwise hitting swap heavily, which slows things down A LOT.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@K900 K900 requested a review from tfc as a code owner February 26, 2024 10:39
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Feb 26, 2024
@K900 K900 added the 1.severity: channel blocker Blocks a channel label Feb 26, 2024
roberth
roberth reviewed Feb 26, 2024
View reviewed changes
pkgs/applications/virtualization/xen/4.15.nix Outdated
Copy link
Member

@roberth roberth Feb 26, 2024

Choose a reason for hiding this comment

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

This is a different file.

nix-repl> OVMF.firmware
"/nix/store/jhnng8aqgw824a08ly9h878rzzgm5m92-OVMF-202402-fd/FV/OVMF_CODE.fd"

The other day I tried to boot a USB stick in qemu, but it didn't work with this attribute; I had to use OVMF.fd, not OVMF_CODE.fd.

Maybe this use case is different; I'm certainly no expert on this, but it looks wrong.

Copy link
Contributor Author

@K900 K900 Feb 26, 2024

Choose a reason for hiding this comment

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

You were probably using -bios OVMF.fd, which is not entirely correct, you actually need to use the OVMF_CODE.fd and OVMF_VARS.fd files separately. AFAICT Xen should do the right thing here, though I haven't tested it directly.

@ofborg ofborg bot requested a review from jnsgruk February 26, 2024 11:06
@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 Feb 26, 2024
@vcunat vcunat mentioned this pull request Feb 26, 2024
Merged
13 tasks
@adamcstephens
Copy link
Contributor

adamcstephens commented Feb 26, 2024

@K900 do you think the dropping of OVMF.fd is too much of a breaking change that we should roll all or part of #284874 back?

@K900
Copy link
Contributor Author

K900 commented Feb 26, 2024

@K900 do you think the dropping of OVMF.fd is too much of a breaking change that we should roll all or part of #284874 back?

The changes here are OK I think, there aren't that many direct dependencies for OVMF.

@shlevy
Copy link
Member

shlevy commented Feb 26, 2024

Sorry about the churn, I missed the leading comma in f6fc51d#diff-282053126a46a4b207a8b94587e595c7be8ded0fb79a5d8f42d4abe28bb66c13L160 🤦

I’ll review this, as I do agree that we should discourage -bios here anyway, but I’m also happy to just fix this patch if that’s preferred.

@K900
Copy link
Contributor Author

K900 commented Feb 26, 2024

I think we should apply your fix first then, and we can merge this later when there's no rush.

@shlevy
Copy link
Member

shlevy commented Feb 26, 2024

#291565

shlevy
shlevy reviewed Feb 26, 2024
View reviewed changes
@K900 K900 removed the 1.severity: channel blocker Blocks a channel label Feb 26, 2024
@K900 K900 changed the title treewide: fix tests (and a few others) after OVMF refactor, drop LegacyStartCommand nixos/tests: drop LegacyStartCommand Feb 26, 2024
@K900
Copy link
Contributor Author

K900 commented Feb 26, 2024

This PR is just scoped to tests now.

@K900
nixos/lib/test-driver: drop LegacyStartCommand, clean up create_machi…
423098c 
…ne API

We can finally do this now that it's no longer used.
@K900 K900 merged commit d53c203 into NixOS:master Feb 28, 2024
@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. and removed 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 28, 2024
@JulienMalka
Copy link
Member

JulienMalka commented Apr 1, 2024

Not exactly sure why but this change broke some of clevis tests (like installer.clevisBcachefs), the second VM is not booting correctly.

wolfgangwalther added a commit to wolfgangwalther/nixpkgs that referenced this pull request Nov 17, 2024
@wolfgangwalther
nixos/lib/test-driver: remove legacy args handling
71306e6 
Scheduled for removal in 24.11, so let's follow through.

Added in NixOS#291544.
github-actions bot pushed a commit that referenced this pull request Nov 22, 2024
@wolfgangwalther @github-actions
nixos/lib/test-driver: remove legacy args handling
8427b6f 
Scheduled for removal in 24.11, so let's follow through.

Added in #291544.

(cherry picked from commit 71306e6)
vcunat pushed a commit that referenced this pull request Nov 30, 2024
@wolfgangwalther @vcunat
nixos/lib/test-driver: remove legacy args handling
a92ea1f 
Scheduled for removal in 24.11, so let's follow through.

Added in #291544.

(cherry picked from commit 71306e6)
(cherry picked from commit 8427b6f)
hsjobeki pushed a commit to hsjobeki/nixpkgs that referenced this pull request Dec 3, 2024
@wolfgangwalther @hsjobeki
nixos/lib/test-driver: remove legacy args handling
797869c 
Scheduled for removal in 24.11, so let's follow through.

Added in NixOS#291544.

(cherry picked from commit 71306e6)
(cherry picked from commit 8427b6f)
@K900 K900 deleted the ovmf-oof branch July 27, 2025 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shlevy shlevy shlevy left review comments

@roberth roberth roberth left review comments

@tfc tfc Awaiting requested review from tfc

@Mic92 Mic92 Awaiting requested review from Mic92

@RaitoBezarius RaitoBezarius Awaiting requested review from RaitoBezarius

@JulienMalka JulienMalka Awaiting requested review from JulienMalka

@vcunat vcunat Awaiting requested review from vcunat

@jnsgruk jnsgruk Awaiting requested review from jnsgruk

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@K900 @adamcstephens @shlevy @JulienMalka @roberth