Mic92 / nixpkgs-review

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

feature: add `--nom` flag to enable nix-output-monitor #303

Closed gador closed 1 year ago

gador commented 1 year ago

nix-output-monitor provides a nice real time build graph, which gives more information about what builds where, which is especially nice for larger rebuilds.

fixes #302

gador commented 1 year ago

There are a few more uses of nix tools that should be changed, you can find all the instances by running rg '"nix(-\w+)?"'

I disagree. nom only makes sense when build is used. E.g. https://github.com/Mic92/nixpkgs-review/blob/d7a07874b4651a0945583b5b73faaf912d6b4947/nixpkgs_review/utils.py#L48-L59 doesn't make any sense to use nom

figsoda commented 1 year ago

turns out most things that pop up from rg command wouldn't work with nom, but the nix-shell in nix.py could be changed to nom-shell

gador commented 1 year ago

I'm not too familiar with the code that uses nix-shell. But it was my understanding, that the shell gets executed after everything is build. So, in my understanding, changing this to nom-shell doesn't yield any improvements, right?

figsoda commented 1 year ago

nom-shell doesn't do much more than doing a mkShell and buildEnv, but I still think it would be nice to have a consistent interface for commands that could be replaced by nom

gador commented 1 year ago

@figsoda Shall we change it to autodiscover nom and enable it by default? Or leave it as is?

figsoda commented 1 year ago

that sounds like a cool idea, I'll leave the decision to @Mic92

gador commented 1 year ago

I took the liberty to adjust the PR as per @Mic92 suggestion. We can revert the commit, if we decide to not use it.

Otherwise it now uses nom by default, if it is found. Also I added some test cases.

figsoda commented 1 year ago

instead of specifying the path, what if we have something like --nix-flavor, empty string will default to nom and fall back to nix, nom will force nom when possible, and nix will force nix, this way we can also allow nom-shell to be used

gador commented 1 year ago

I like the nix-flavor better. See the new commit

gador commented 1 year ago

shall I squash the commits and rebase?

figsoda commented 1 year ago

you can rebase and force push if you want, or this can also be squash merged

gador commented 1 year ago

Ok, then you can squash merge if it seems all alright to you :+1:

Mic92 commented 1 year ago

bors merge

bors[bot] commented 1 year ago

Build succeeded: