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

multipass: init at 1.11.0 #214193

Merged
merged 4 commits into from
Feb 4, 2023
Merged

multipass: init at 1.11.0 #214193

merged 4 commits into from
Feb 4, 2023

Conversation

jnsgruk
Copy link
Member

@jnsgruk jnsgruk commented Feb 2, 2023

Description of changes

Adds a multipass package and NixOS module.

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.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@AndersonTorres AndersonTorres changed the title maintainers: add jnsgruk multipass: init at 1.11.0 Feb 2, 2023
Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

General hammering.

nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
pkgs/tools/virtualization/multipass/default.nix Outdated Show resolved Hide resolved
pkgs/tools/virtualization/multipass/default.nix Outdated Show resolved Hide resolved
pkgs/tools/virtualization/multipass/default.nix Outdated Show resolved Hide resolved
pkgs/tools/virtualization/multipass/default.nix Outdated Show resolved Hide resolved
pkgs/tools/virtualization/multipass/default.nix Outdated Show resolved Hide resolved
@jnsgruk jnsgruk marked this pull request as ready for review February 2, 2023 16:46
@jnsgruk
Copy link
Member Author

jnsgruk commented Feb 2, 2023

Thanks for the rapid review @AndersonTorres! I think I've addressed all your feedback.

@jnsgruk
Copy link
Member Author

jnsgruk commented Feb 2, 2023

I've added some simple tests here; I initially added a test that actually started a VM and tested it, but since Multipass fetches the VM images from the internet, the tests don't pass, so I've removed them for now.

@RaitoBezarius
Copy link
Member

I've added some simple tests here; I initially added a test that actually started a VM and tested it, but since Multipass fetches the VM images from the internet, the tests don't pass, so I've removed them for now.

Thank you a lot for this! Can you rebase your branch into three commits:

  • maintainer introduction commit
  • multipass: init at 1.11.0
  • nixos/multipass: init

separating package and module if possible :). (you can keep tests in the module commit, if it's too annoying, make it two commits with multipass: init at 1.11.0

@jnsgruk
Copy link
Member Author

jnsgruk commented Feb 2, 2023

I've added some simple tests here; I initially added a test that actually started a VM and tested it, but since Multipass fetches the VM images from the internet, the tests don't pass, so I've removed them for now.

Thank you a lot for this! Can you rebase your branch into three commits:

  • maintainer introduction commit
  • multipass: init at 1.11.0
  • nixos/multipass: init

separating package and module if possible :). (you can keep tests in the module commit, if it's too annoying, make it two commits with multipass: init at 1.11.0

Will address your feedback, then rebase. Thanks!

nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/multipass.nix Outdated Show resolved Hide resolved
@AndersonTorres
Copy link
Member

you can keep tests in the module commit

It is recommended to put the tests in its own separated commit.

nixos/tests/multipass.nix Outdated Show resolved Hide resolved
@jnsgruk
Copy link
Member Author

jnsgruk commented Feb 3, 2023

I've restricted this to x86_64-linux for now, as I don't have a way of testing others. I know for sure there will be tweaks needed in the build for aarch64.

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

Besides LGTM

nixos/tests/multipass.nix Outdated Show resolved Hide resolved
Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

Besides LGTM.

pkgs/tools/virtualization/multipass/default.nix Outdated Show resolved Hide resolved
@AndersonTorres
Copy link
Member

And, before I forget: welcome to Nixpkgs!

@AndersonTorres AndersonTorres merged commit bc0944c into NixOS:master Feb 4, 2023
@jnsgruk
Copy link
Member Author

jnsgruk commented Feb 4, 2023

Thanks for all the help! 😄

@jnsgruk jnsgruk deleted the add-multipass-pkg branch February 6, 2023 17:49
repo = "multipass";
rev = "refs/tags/v${version}";
sha256 = "sha256-2d8piIIecoSI3BfOgAVlXl5P2UYDaNlxUgHXWbnSdkg=";
fetchSubmodules = true;
Copy link
Member

Choose a reason for hiding this comment

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

Using submodules is problematic because eg we are using a two year old fork of libssh https://github.com/canonical/multipass/blob/main/.gitmodules#L9-L11

Copy link
Member

Choose a reason for hiding this comment

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

Please, can you open an issue documenting your remarks post-merge?

Copy link
Member

Choose a reason for hiding this comment

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

As this is directly related to this PR, I think that is unnecessary.


nativeBuildInputs = [
cmake
git
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need git?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants