Mic92 / nixpkgs-review

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

Eval fixes #254

Closed jonringer closed 2 years ago

jonringer commented 2 years ago

Keep running into these issues when reviewing staging PRs

jonringer commented 2 years ago

Testing this on some PRs. Takes a while for nix-env -qa to run.

jonringer commented 2 years ago

cc @Mic92 looks ready to go, was able to eval https://github.com/NixOS/nixpkgs/pull/142593 by adding --disable-aliases

jonringer commented 2 years ago

ofborg sometimes puts some aliases in the eval results. I am not sure why but it seems to always be gnome.*

unless you're reviewing a PR made against master, it will force the eval to include aliases

 --disable-aliases     Disable aliases while evaluating locally. They are automatically disabled when reviewing
                        NixOS/nixpkgs PRs against master.
SuperSandro2000 commented 2 years ago

Yeah, we put that in because PRs from stable could still contain aliases and it is not easy to remove them there.

Maybe those lines need to be updated https://github.com/Mic92/nixpkgs-review/blob/ab4a35df2c885f29e336aa3c3e30734283989b80/nixpkgs_review/cli/pr.py#L41-L44

The intention was, that only aliases are forbidden when reviewing PRs merged against master.

Edit: I think the conditions in https://github.com/Mic92/nixpkgs-review/commit/ab4a35df2c885f29e336aa3c3e30734283989b80 are wrong but I am not sure how and which yet.

jonringer commented 2 years ago

Yeah, we put that in because PRs from stable could still contain aliases and it is not easy to remove them there.

There's an ofborg check to prevent that situation. There should be no alias usage within nixpkgs.

example PR: https://github.com/NixOS/nixpkgs/pull/156318

 ofborg-eval-package-list-no-aliases — nix-env -qa --json --file . --arg config { allowAliases = false; }
jonringer commented 2 years ago

The intention was, that only aliases are forbidden when reviewing PRs merged against master.

255

SuperSandro2000 commented 2 years ago

I merged #255 and pushed 1ad19620ef898df35f80b9ee03d18bf80dfbe003 to master.