-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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/mopidy: test & cleanup #356021
base: master
Are you sure you want to change the base?
nixos/mopidy: test & cleanup #356021
Conversation
619a262
to
e931969
Compare
mopidyEnv = pkgs.buildEnv { | ||
name = "mopidy-with-extensions-${cfg.package.version}"; | ||
paths = lib.closePropagation cfg.extensionPackages; | ||
pathsToLink = [ "/${pkgs.mopidyPackages.python.sitePackages}" ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that this might allow mixing the extension packages and the mopidy
package in an unsafe way.
For instance, assume that this service is being used from a stable branch of NixOS.
If someone sets the package
option to use the unstable
Mopidy package, what happens when the branches target different versions of Python?
Which version of Python will be used, the one from stable or unstable? Or perhaps it will use some combination of versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we provide an additional option for the mopidyPackages
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was leaning toward something like that. It just makes me wonder if there still needs to be an option for mopidy
then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what i want to prevent is something like this (which could solve it): https://github.com/NixOS/nixpkgs/blob/nixos-24.05/nixos/modules/virtualisation/qemu-vm.nix#L668
can we assert that somehow?
i will ask around in the matrix-room for some suggestions.
29c87a2
to
0c0bcb5
Compare
@jwillikers maybe we first merge everything except the package option? |
That sounds great to me! |
0c0bcb5
to
9262fc4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the clean up!
adding test and cleanup
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.