Skip to content

nixpkgs-review: suffix PATH rather than prefix#370022

Open
Atemu wants to merge 1 commit intoNixOS:masterfrom
Atemu:nixpkgs-review-suffix-path
Open

nixpkgs-review: suffix PATH rather than prefix#370022
Atemu wants to merge 1 commit intoNixOS:masterfrom
Atemu:nixpkgs-review-suffix-path

Conversation

@Atemu
Copy link
Member

@Atemu Atemu commented Jan 1, 2025

This prefers using the required binaries from the user's env if present which should be preferred unless there are specific requirements on the binaries given at build time.

This allows using e.g. Lix without needing to overlay the nix package.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

This prefers using the required binaries from the user's env if present which
should be preferred unless there are specific requirements on the binaries given
at build time.

This allows using e.g. Lix without needing to overlay the nix package.
@Atemu Atemu requested a review from Mic92 January 1, 2025 19:26
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 1, 2025
@Atemu
Copy link
Member Author

Atemu commented Jan 1, 2025

This is a trivial change and I can already tell it works but I can't actually run it because 3.0.0 appears to be generally broken; with or without this change:

[atemu@THESEUS nixpkgs]$ ./result/bin/nixpkgs-review pr 369982
-> Attempting to fetch eval results from GitHub actions
$ git worktree remove -f /Users/atemu/.cache/nixpkgs-review/pr-369982-4/nixpkgs
fatal: '/Users/atemu/.cache/nixpkgs-review/pr-369982-4/nixpkgs' is not a working tree
Failed to remove worktree at /Users/atemu/.cache/nixpkgs-review/pr-369982-4/nixpkgs. Please remove it manually. Git failed with: 128
Traceback (most recent call last):
  File "/nix/store/3s85w6w6r9jahxpix52aysqqhr5n3m7z-nixpkgs-review-3.0.0/bin/.nixpkgs-review-wrapped", line 9, in <module>
    sys.exit(main())
             ^^^^^^
  File "/nix/store/3s85w6w6r9jahxpix52aysqqhr5n3m7z-nixpkgs-review-3.0.0/lib/python3.12/site-packages/nixpkgs_review/__init__.py", line 10, in main
    cli.main(command, args)
  File "/nix/store/3s85w6w6r9jahxpix52aysqqhr5n3m7z-nixpkgs-review-3.0.0/lib/python3.12/site-packages/nixpkgs_review/cli/__init__.py", line 348, in main
    return cast(str, args.func(args))
                     ^^^^^^^^^^^^^^^
  File "/nix/store/3s85w6w6r9jahxpix52aysqqhr5n3m7z-nixpkgs-review-3.0.0/lib/python3.12/site-packages/nixpkgs_review/cli/pr.py", line 97, in pr_command
    contexts.append((pr, builddir.path, review.build_pr(pr)))
                                        ^^^^^^^^^^^^^^^^^^^
  File "/nix/store/3s85w6w6r9jahxpix52aysqqhr5n3m7z-nixpkgs-review-3.0.0/lib/python3.12/site-packages/nixpkgs_review/review.py", line 300, in build_pr
    packages_per_system = self.github_client.get_github_action_eval_result(pr)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/3s85w6w6r9jahxpix52aysqqhr5n3m7z-nixpkgs-review-3.0.0/lib/python3.12/site-packages/nixpkgs_review/github.py", line 182, in get_github_action_eval_result
    changed_paths: Any = self.get_json_from_artifact(
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/3s85w6w6r9jahxpix52aysqqhr5n3m7z-nixpkgs-review-3.0.0/lib/python3.12/site-packages/nixpkgs_review/github.py", line 119, in get_json_from_artifact
    with no_redirect_opener.open(req) as resp:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/zv1kaq7f1q20x62kbjv6pfjygw5jmwl6-python3-3.12.7/lib/python3.12/urllib/request.py", line 521, in open
    response = meth(req, response)
               ^^^^^^^^^^^^^^^^^^^
  File "/nix/store/zv1kaq7f1q20x62kbjv6pfjygw5jmwl6-python3-3.12.7/lib/python3.12/urllib/request.py", line 630, in http_response
    response = self.parent.error(
               ^^^^^^^^^^^^^^^^^^
  File "/nix/store/zv1kaq7f1q20x62kbjv6pfjygw5jmwl6-python3-3.12.7/lib/python3.12/urllib/request.py", line 559, in error
    return self._call_chain(*args)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/zv1kaq7f1q20x62kbjv6pfjygw5jmwl6-python3-3.12.7/lib/python3.12/urllib/request.py", line 492, in _call_chain
    result = func(*args)
             ^^^^^^^^^^^
  File "/nix/store/zv1kaq7f1q20x62kbjv6pfjygw5jmwl6-python3-3.12.7/lib/python3.12/urllib/request.py", line 639, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 403: Forbidden

I can tell it's working though because it doesn't complain about unknown experimental features.

@Mic92
Copy link
Member

Mic92 commented Jan 1, 2025

I think I would prefer if user would just override the package because it's sneaky if the nix version changes without the user knowing and there might be subtle differences between versions. The re-build of this package is very fast.

@Mic92
Copy link
Member

Mic92 commented Jan 1, 2025

For future reference:

(nixpkgs-review.override { nix = pkgs.foo; })

@Atemu
Copy link
Member Author

Atemu commented Jan 1, 2025

it's sneaky if the nix version changes without the user knowing and there might be subtle differences between versions

Yes of course but I think that's an argument for doing this. Ideally, any nix-dependent process should use the same Nix as the nix-daemon which is the one in the environment.

@Mic92
Copy link
Member

Mic92 commented Jan 1, 2025

it's sneaky if the nix version changes without the user knowing and there might be subtle differences between versions

Yes of course but I think that's an argument for doing this. Ideally, any nix-dependent process should use the same Nix as the nix-daemon which is the one in the environment.

Some people might still have nix 2.3 installed for whatever reason and that will not be compatible with nixpkgs-review.

@nixpkgs-ci nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 1, 2025
@LordGrimmauld
Copy link
Contributor

LordGrimmauld commented Jul 5, 2025

Related: #420974
nix 2.3 might not have that long to live, at which point that consideration becomes obsolete.
It might also make sense to set PATH in an explicit (seperate) wrapper, that way the package rebuild doesn't compile anything (even if it is fast).

@Aleksanaa
Copy link
Member

I've merged #418318 which takes another approach.

@nixpkgs-ci nixpkgs-ci bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 5, 2025
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge-bot eligible This PR can be merged by commenting "@NixOS/nixpkgs-merge-bot merge". label Nov 2, 2025
@nixpkgs-ci nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge-bot eligible This PR can be merged by commenting "@NixOS/nixpkgs-merge-bot merge". 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants