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: 1.14.0 -> 1.15.0 #363626

Merged
merged 3 commits into from
Dec 11, 2024
Merged

multipass: 1.14.0 -> 1.15.0 #363626

merged 3 commits into from
Dec 11, 2024

Conversation

jnsgruk
Copy link
Member

@jnsgruk jnsgruk commented Dec 9, 2024

This commit is a fairly big one:

  • Package up the new 1.15.0-rc4 release
  • Include the Flutter desktop application
  • Move to pkgs/by-name

I've pushed this a little early while we're still in the late rc phase of the Multipass release to get feedback on the restructuring. I noted the following in the commit that introduces the GUI element:

This commit breaks down multipass into two intermediate packages,
`multipassd` and `multipass-gui`. These are then packaged up with
`symlinkJoin` to work around some intricacies in the
`buildFlutterApplication` process which would otherwise be difficult
to patch.

The multipass repo is setup upstream such that the flutter app would
be built as part of the regular cmake process, but that can't work
here due to the fetching of the flutter dependencies. The upstream
multipass project builds `libdart_ffi.so`, which is needed in the
LD_LIBRARY_PATH of the GUI. Building the two derivations seperately
enables that to be done relatively simply.

Hopefully that makes sense! Tested locally on my machine and all works as expected.

Perhaps @RossComputerGuy could give some feedback on this approach, seems they have lots of involvement with the Flutter tooling in nixpkgs.

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 9, 2024
@jnsgruk

This comment was marked as outdated.

@jnsgruk jnsgruk force-pushed the multipass-1.15.0 branch 3 times, most recently from 575022a to 42f31ed Compare December 9, 2024 21:27
Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

Also is there a reason to be updating to a release candidate instead of waiting for a stable release?

pkgs/by-name/mu/multipass/gui.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/multipassd.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/package.nix Show resolved Hide resolved
pkgs/by-name/mu/multipass/gui.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/gui.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/gui.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/package.nix Outdated Show resolved Hide resolved
@jnsgruk
Copy link
Member Author

jnsgruk commented Dec 10, 2024

Also is there a reason to be updating to a release candidate instead of waiting for a stable release?

Thanks for the feedback, I'll make those changes today - and nice tip on the desktop file, i'd not spotted that before.

Indeed my intention was to wait until the 1.15.0 release, I meant to mark this as draft while i got feedback on the approach 🤦‍♂️

@jnsgruk jnsgruk marked this pull request as draft December 10, 2024 08:51
@jnsgruk jnsgruk force-pushed the multipass-1.15.0 branch 3 times, most recently from 99e7f94 to 6a45cda Compare December 10, 2024 10:50
@jnsgruk jnsgruk requested a review from getchoo December 10, 2024 11:02
pkgs/by-name/mu/multipass/update.sh Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/update.sh Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/multipassd.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/multipassd.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/gui.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/gui.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/gui.nix Outdated Show resolved Hide resolved
pkgs/by-name/mu/multipass/gui.nix Outdated Show resolved Hide resolved
@jnsgruk

This comment was marked as outdated.

@jnsgruk jnsgruk changed the title multipass: 1.14.0 -> 1.15.0-rc4 multipass: 1.14.0 -> 1.15.0 Dec 11, 2024
@jnsgruk
Copy link
Member Author

jnsgruk commented Dec 11, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 363626


x86_64-linux

✅ 1 package built:
  • multipass

@jnsgruk jnsgruk marked this pull request as ready for review December 11, 2024 13:02
@jnsgruk
Copy link
Member Author

jnsgruk commented Dec 11, 2024

@SuperSandro2000 @getchoo feedback addressed, and also now bumped to the actual release, not the RC. Hopefully good to go now! Thanks for the help.

@jnsgruk
Copy link
Member Author

jnsgruk commented Dec 11, 2024

And some screenshots 😄

image

image

image

This commit breaks down multipass into two intermediate packages,
`multipassd` and `multipass-gui`. These are then packaged up with
`symlinkJoin` to work around some intricacies in the
`buildFlutterApplication` process which would otherwise be difficult
to patch.

The multipass repo is setup upstream such that the flutter app would
be built as part of the regular cmake process, but that can't work
here due to the fetching of the flutter dependencies. The upstream
multipass project builds `libdart_ffi.so`, which is needed in the
LD_LIBRARY_PATH of the GUI. Building the two derivations seperately
enables that to be done relatively simply.
@jnsgruk
Copy link
Member Author

jnsgruk commented Dec 11, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 363626


x86_64-linux

✅ 1 package built:
  • multipass

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

Diff LGTM, but I'm not all that familiar with flutter

@getchoo getchoo added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 11, 2024
@jnsgruk jnsgruk merged commit e659d91 into NixOS:master Dec 11, 2024
20 of 21 checks passed

# Generate the Dart gRPC code for the Multipass GUI.
protoc \
--plugin=protoc-gen-dart=${lib.getExe protoc-gen-dart} \
Copy link
Contributor

Choose a reason for hiding this comment

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

I think as it is, this is broken for cross-compilation? And needs to use either:

  • $(which protoc-gen-dart) in the shell to look up the nativeBuildInputs.
  • ${lib.getExe buildPackages.protoc-gen-dart} in Nix to correctly use a build package, not a host package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants