Mic92 / nixpkgs-review

Review pull-requests on https://github.com/NixOS/nixpkgs
MIT License
353 stars 59 forks source link

Add an option to disable unfree packages #318

Closed Atemu closed 8 months ago

Atemu commented 1 year ago

While trying to regression check a PR with a large amount of rebuilds, I wanted to not build all sorts of unfree packages but I was surprised to see that nixpkgs-review A. builds them by default despite explicit user config disabling unfree (neither ~/.config/nixpkgs/config.nix nor env var) B. doesn't have a flag to override this behaviour.

Please add a CLI option for unfree packages and respect the user's choice by default.

figsoda commented 1 year ago

https://github.com/Mic92/nixpkgs-review/pull/315 should allow that, using --extra-nixpkgs-config '{ allowUnfree = false; }' with --eval local. That being said, an option in --allow would probably be nice to have

SomeoneSerge commented 1 year ago

For that matter, I think it would be good manners to interactively ask the user consent to evaluating unfree code the first time they run nixpkgs-review. And to provide an optional batch-mode flag, disabling all interaction. And even then we might break somebody's scripting:)

Mic92 commented 1 year ago

For that matter, I think it would be good manners to interactively ask the user consent to evaluating unfree code the first time they run nixpkgs-review. And to provide an optional batch-mode flag, disabling all interaction. And even then we might break somebody's scripting:)

I don't like to have hidden settings in nixpkgs-review. It should be obvious from the command line flags how a user has run a review so it's clear to others what has been tested.

Mic92 commented 1 year ago

Please add a CLI option for unfree packages and respect the user's choice by default.

Cli option fine, configuration file not so much as it adds too much surprises why in once person builds some packages have been build and failed and on another machines it has one.

SomeoneSerge commented 1 year ago

I don't like to have hidden settings in nixpkgs-review. It should be obvious from the command line flags how a user has run a review so it's clear to others what has been tested.

$ nixpkgs-review pr 12345
...
You are about to evaluate nixpkgs with config.allowUnfree = true, would you like to proceed? [N/y/a]
n
Aborting. Use --only-free to run nixpkgs-review without unfree package
$ nixpkgs-review pr 12345 --only-free  # The only way to go without allowUnfree is explicit
...
nix build ...
...
$ nixpkgs-review pr 12345
...
You are about to evaluate nixpkgs with config.allowUnfree = true, would you like to proceed? [N/y/a]
y
...
nix build ...
...
$ nixpkgs-review pr 12345
...
You are about to evaluate nixpkgs with config.allowUnfree = true, would you like to proceed? [N/y/a]
a
Saving the preference to always allowUnfree
...
nix build ...
...
$ nixpkgs-review pr 12345
...
nix build ...
...

@Mic92 what do you think

Atemu commented 9 months ago

With #315 merged, this is sort of fixed. I'd personally still love to see a specific command line flag for unfree software that is off by default however. Reason being that blocking unfree is an ideology shared by many in this community.

I personally only run unfree if I have to and reviewing random proprietary software from nixpkgs is not a thing I want to have happen, especially not by default. Not only do I not want the code downloaded and executed on my machine, I don't want to have to care about unfree software. It's not worthy of my (or IMO most maintainers) attention.

This doesn't need to escalate into any current or future Nixpkgs config flags also having to be supported. allowUnfree has a bit more "weight" behind it than all the other nixpkgs options IMHO, so special treatment is warranted.

Perhaps there could also be a better flag to easily set config such as --config someSetting true which then gets translated into

import <nixpkgs> {
  allowUnfree = false;
} // {
  someSetting = true;
} ...
Mic92 commented 8 months ago

I would suggest to have a shell alias for your convenience.