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

NixOS Integration Tests on macOS: Remove some obstacles (ulimit, requiredSystemFeatures) #261694

Closed
wants to merge 4 commits into from

Conversation

tfc
Copy link
Contributor

@tfc tfc commented Oct 17, 2023

Description of changes

I got a step towards running NixOS integration tests on macOS without modifications in the test, other than the ones on nixpkgs in this PR.

let
  pkgs = import ./. {};

  testConfig = { lib, hostPkgs, ... }: {
    name = "An awesome test.";

    nodes = {
      machine1 = { pkgs, ... }: { };
      machine2 = { pkgs, ... }: { };
    };

    testScript = ''
      machine1.wait_for_unit("network-online.target")
      machine2.wait_for_unit("network-online.target")

      machine1.succeed("ping -c 1 machine2")
      machine2.succeed("ping -c 1 machine1")
    '';
  };
in

pkgs.testers.runNixOSTest testConfig

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 Oct 17, 2023
@tfc tfc requested review from roberth and Enzime October 17, 2023 19:18
nixos/lib/testing/run.nix Outdated Show resolved Hide resolved
nixos/lib/testing/run.nix Outdated Show resolved Hide resolved
@tfc tfc force-pushed the macos-nixos-integration-tests branch from 532773f to f10148a Compare October 18, 2023 09:45
nixos/lib/testing/run.nix Outdated Show resolved Hide resolved
@tfc tfc force-pushed the macos-nixos-integration-tests branch 2 times, most recently from 343b5ce to 37d2650 Compare October 18, 2023 11:34
nixos/lib/testing/run.nix Outdated Show resolved Hide resolved
Comment on lines +102 to +105
guestPkgs =
if (!pkgs.stdenv.hostPlatform.isDarwin)
then pkgs
else import pkgs.path { system = toGuest pkgs.system; };
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have pkgsLinux, like we have pkgsStatic or pkgsCross.<x> (although this wouldn't be cross; just a related platform that's built natively)

Copy link
Member

Choose a reason for hiding this comment

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

Or pkgsNativePlatform.linux or something, with native referring both to not being cross and being able to run on the same hardware that runs pkgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Although this would blow up this PR... can we discuss this offline after this PR how to improve it?

pkgs/build-support/testers/default.nix Show resolved Hide resolved
{
config = lib.mkIf hostPkgs.stdenv.hostPlatform.isDarwin {
defaults = {
virtualisation.host.pkgs = hostPkgs;
Copy link
Member

Choose a reason for hiding this comment

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

This line doesn't seem darwin-specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so what do you think is the best way to do it? use !...isLinux ?

Copy link
Member

Choose a reason for hiding this comment

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

I think this could be set in this module, here

virtualisation.qemu.package = testModuleArgs.config.qemu.package;

I can't think of a situation where host.pkgs and hostPkgs should not have the same value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how you mean that... use hostPkgs instead of ...config.qemu.package? I guess you mean that the default value for that should be hostpkgs but then in the file where this is defined?

Copy link
Member

Choose a reason for hiding this comment

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

I meant it like

 virtualisation.qemu.package = testModuleArgs.config.qemu.package;
+virtualisation.host.pkgs = hostPkgs;

ie move the line away from here

Suggested change
virtualisation.host.pkgs = hostPkgs;

and into lib/testing/nodes.nix.

The virtualisation.qemu.package line is unchanged and unrelated.

nixos/lib/testing/macos-host.nix Outdated Show resolved Hide resolved
nixos/lib/testing/macos-host.nix Show resolved Hide resolved
@Enzime
Copy link
Member

Enzime commented Oct 19, 2023

The NixOS test shown in the example runs locally for me 👍

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/run-nixos-interactive-tests-on-aarch64-darwin/34534/2

@tfc tfc force-pushed the macos-nixos-integration-tests branch from 1566a37 to 65b5201 Compare November 24, 2023 17:25
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 24, 2023
Gabriella439 added a commit to Gabriella439/nixpkgs that referenced this pull request Feb 21, 2024
Based on my local testing these options seem unnecessary and the VM
runs fine without them being set.  I originally cargo-culted these
from NixOS#261694 but they don't seem necessary.
@tfc tfc closed this Mar 1, 2024
Gabriella439 added a commit that referenced this pull request Mar 2, 2024
Closes #193336
Closes #261694
Related to #108984

The goal here was to get the following flake to build and run on
`aarch64-darwin`:

```nix
{ inputs.nixpkgs.url = <this branch>;

  outputs = { nixpkgs, ... }: {
    checks.aarch64-darwin.default =
      nixpkgs.legacyPackages.aarch64-darwin.nixosTest {
        name = "test";

        nodes.machine = { };

        testScript = "";
      };
  };
}
```

… and after this change it does.  There's no longer a need for the
user to set `nodes.*.nixpkgs.pkgs` or
`nodes.*.virtualisation.host.pkgs` as the correct values are inferred
from the host system.
roberth pushed a commit to hercules-ci/nixpkgs that referenced this pull request Mar 5, 2024
Closes NixOS#193336
Closes NixOS#261694
Related to 108984

This pull request is a composite of the changes from NixOS#193336 and
also NixOS#261694, with some changes of my own that I made.

The goal here was to get the following flake to build and run on
`aarch64-darwin`:

```nix
{ inputs.nixpkgs.url = <this branch>;

  outputs = { nixpkgs, ... }: {
    checks.aarch64-darwin.default =
      nixpkgs.legacyPackages.aarch64-darwin.nixosTest {
        name = "test";

        nodes.machine = { };

        testScript = "";
      };
  };
}
```

… and after this change it does.  There's no longer a need for the
user to set `nodes.*.nixpkgs.pkgs` or
`nodes.*.virtualisation.host.pkgs` as the correct values are inferred
from the host system.

This change is spiritually much closer to the approach in NixOS#261694
than the approach in NixOS#193336.  However, I still made a few changes
compared to NixOS#261694:

- I didn't include the change to increase `ulimit`

  I think this change was questionable because I feel like a script
  provided by Nixpkgs shouldn't be tinkering with the user's ambient
  `ulimit` settings.

- I named the required system feature `hvf` instead of `apple-virt`

  I preferred the `hvf` system feature name chosen by NixOS#193336.

- I didn't use the `node.pkgs` setting to set `nodes.*.nixpkgs.pkgs`

  That does not work, based on my testing, but setting the same thing
  in `nixos/lib/testing/nodes.nix` does work.

- I created a `pkgsLocal` helper

  This is based on the feedback from @roberth here:

  https://github.com/NixOS/nixpkgs/pull/261694/files#r1365443955

  … and is similar in spirit to `pkgsCross`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: darwin Running or building packages on Darwin 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 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants