Mic92 / nixpkgs-review

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

add --extra-nixpkgs-config #315

Closed figsoda closed 1 year ago

figsoda commented 1 year ago

closes #314

SomeoneSerge commented 1 year ago

Is there anything to be done with list_packages? https://github.com/Mic92/nixpkgs-review/blob/6ffaffadeaafaf054fa5e76180789e387734fa6a/nixpkgs_review/review.py#L333-L347

Maybe it's easier to implement --nixpkgs-option config { allowUnfree = true; } than --extra-nixpkgs-config { config.allowUnfree = true; }, because it maps directly into the already familiar nix-env's --arg or --option

figsoda commented 1 year ago

I'm not sure. There are a few things we can do

SomeoneSerge commented 1 year ago

IIUC, we need either of the last two options, otherwise review.differences will miss the affected attributes. Either is good, and I don't know which is easier to maintain

Mic92 commented 1 year ago

I will skip reviewing this feature but I think we should have some way of passing nixpkgs configuration into nixpkgs-review. Just merge if you come to a consensus @SuperSandro2000 @figsoda

SomeoneSerge commented 1 year ago

Here's a preview of the current status: https://gist.github.com/SomeoneSerge/745681c4d577fac2b2e71b6349cc2503 with this quick solution to differences(): https://github.com/SomeoneSerge/nixpkgs-review/commit/9c3c9fe82f27cee5a9ff340068296b943a494e89

At a glance, I think the list of packages looks good

Mic92 commented 1 year ago

When this PR is merged I will make a new release.

figsoda commented 1 year ago

One issue I have currently is that nix-env doesn't do recursive updates. Would any config change the outputs of nix-env? I'm thinking we should just ignore nix-env and implement recursive updates for the regular eval I think we should go with --extra-nixpkgs-config instead so we don't have to handle multiple arguments

SomeoneSerge commented 1 year ago

I think (I hope) I'm observing some false-positive "updates": https://gist.github.com/SomeoneSerge/6cc00b41964e43f725fc12046778532d Sorry, it works alright, just accidentally opened a portal to hell. I'll run without global cudaSupport and post again when it's over

SomeoneSerge commented 1 year ago

Is it expect behaviour that nixpkgs-review pr doesn't always print the "X packages updated" part? E.g. here it didn't mention nix-env and just went ahead to nix build: https://gist.github.com/5c5ceccb1add211d551a9813ef8be960

figsoda commented 1 year ago

It is expected I believe: https://github.com/Mic92/nixpkgs-review/commit/e704d6a5b694f79881247f4f39aee12d63bd6575

figsoda commented 1 year ago

@SuperSandro2000

figsoda commented 1 year ago

bors r+

I messaged sandro privately. He is currently prioritizing on other things and is not against merging this

bors[bot] commented 1 year ago

Build succeeded:

nviets commented 1 year ago

Can I use this argument to cap resources with max-jobs and cores? By default nixpkgs-review tries to build with everything.

SuperSandro2000 commented 1 year ago

You want to set those settings in your nix.conf.