search

Mic92 / nixpkgs-review

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

Insight on why shell-review attempts to parse a hosts file as a script #407

Closed Frontear closed 3 months ago

Frontear commented 3 months ago

Hi!

Got a PR on nixpkgs here with a variety of improvements for the stevenblack-blocklist package + nixos modules, and I'm running into a very odd error when running the standard review from HEAD.

┃ error: builder for '/nix/store/dmr7w0cqxdj0xbz1l5fm5wm124c2jc09-review-shell.drv' failed with exit code 127;
┃        last 3 log lines:
┃        > /nix/store/1jik86ikk79rlkhrnwmyksfzcnhgaa6l-stevenblack-blocklist-3.14.84-ads: line 15: 127.0.0.1: command not found
┃        > /nix/store/d3dzfy4amjl826fb8j00qp1d9887h7hm-stdenv-linux/setup: line 692: pop_var_context: head of shell_variables not a function context
┃        > /nix/store/d3dzfy4amjl826fb8j00qp1d9887h7hm-stdenv-linux/setup: line 699: pop_var_context: head of shell_variables not a function context
┃        For full logs, run 'nix log /nix/store/dmr7w0cqxdj0xbz1l5fm5wm124c2jc09-review-shell.drv'.

I was hoping for some insight regarding why this error seems to happen. It looks like its trying to process the hosts file as a shell script, and failing on the first line that declares localhost bindings.

Artturin commented 3 months ago

https://github.com/NixOS/nixpkgs/blob/4cccdeaffb6f676b44e58ff7ef37415239ec863b/pkgs/stdenv/generic/setup.sh#L647

the line was added in https://github.com/nixos/nixpkgs/commit/b23dbb1a5dffbfa3abb47fcd0f1579ac2e6f29fc

Frontear commented 3 months ago

What should I consider this, a bug in nixpkgs-review or a mistake in my approach? No idea where this sort of logic would propagate and if its a fundamentally incorrect way to expose outputs.

Artturin commented 3 months ago

The root cause is the sourcing in nixpkgs Then the cause is that the package is added to nativeBuildInputs by packages in mkShell

You can avoid the error by creating a directory and moving the file inside it

Frontear commented 3 months ago

Oh yes my mistake, nixpkgs not this. I think I'll end up changing my outputs and then subsequently maybe raising this as a separate issue upstream.