Skip to content

Conversation

@tejing1
Copy link
Contributor

@tejing1 tejing1 commented Jun 24, 2025

  • Added binlore entry to ease use in resholved scripts.
  • Converted to finalAttrs since buildNpmPackage supports that now.
  • Using finalAttrs.finalPackage ensures that the binlore passthru corresponds correctly even when the package is overridden outside of an overlay, which otherwise wouldn't be possible. (tests, too... though that's not all that important)

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/)
  • Nixpkgs 25.11 Release Notes (or backporting 24.11 and 25.05 Nixpkgs Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • NixOS 25.11 Release Notes (or backporting 24.11 and 25.05 NixOS Release notes)
    • (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, pkgs/README.md, maintainers/README.md and other contributing documentation in corresponding paths.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jun 24, 2025
@tejing1
Copy link
Contributor Author

tejing1 commented Jun 27, 2025

I suppose I should clarify a bit, for reviewers not familiar with resholve and binlore. There are 3 things that need to be verified about this PR:

  • That the conversion to finalAttrs was done correctly without some kind of mistake.
  • That the binlore passthru is properly formed and is picked up by resholve. You can test this by trying to use hred in a resholve-packaged shell script.
    Try building something like this: pkgs.resholve.writeScript "hred-test" { interpreter = lib.getExe pkgs.bash; inputs = [ pkgs.hred ]; } "hred --version"
    The build will fail with a complaint about missing lore on current nixpkgs, but succeed with this PR.
  • That the assertion is actually correct. That is, that hred never runs any of its arguments as commands, and thus resholve doesn't need to worry about translating any of them to absolute paths. This is quite easy to see from hred's --help output. It has a very simple interface, with no commands in sight.

@nixpkgs-ci nixpkgs-ci bot added the 9.needs: reviewer This PR currently has no reviewers requested and needs attention. label Jul 4, 2025
@pbsds pbsds merged commit f8d1386 into NixOS:master Jul 10, 2025
31 of 32 checks passed
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented Jul 10, 2025

Successfully created backport PR for release-25.05:

@github-actions github-actions bot added the 8.has: port to stable This PR already has a backport to the stable release. label Jul 10, 2025
@tejing1 tejing1 deleted the hred-add-binlore branch July 10, 2025 01:18
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/5655

@tejing1
Copy link
Contributor Author

tejing1 commented Jul 15, 2025

6 days late, there, bot. Thanks anyway?

@pbsds
Copy link
Member

pbsds commented Jul 15, 2025

good bot, at least you tried

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: port to stable This PR already has a backport to the stable release. 9.needs: reviewer This PR currently has no reviewers requested and needs attention. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants