Skip to content

pass: use resholvePackage#157641

Draft
abathur wants to merge 1 commit intoNixOS:masterfrom
abathur:resholve_pass
Draft

pass: use resholvePackage#157641
abathur wants to merge 1 commit intoNixOS:masterfrom
abathur:resholve_pass

Conversation

@abathur
Copy link
Copy Markdown
Member

@abathur abathur commented Feb 1, 2022

Opening this as a draft to start a conversation (I'm hopefully over-@ing--feel free to opt out):

This PR repackages pass with resholve, a tool I develop to improve Shell packaging in Nixpkgs. resholve identifies unspecified dependencies, and rewrites external invocations to absolute paths so that end-users of Shell projects don't need to manually add dependencies to their system/user packages.

Aside: I also want to thank @cole-h, who did much of the work here back in August/September. (Fair warning that I'm not a pass user; I asked Cole about this and he said he's not using pass anymore--so I'm trying to see if I can pick up that patch to keep the work from going to waste...)

I'm hoping to (eventually) get to a simple yes/no on the change (and confirmation that it isn't breaking any of your own uses), so I'm happy to field any questions you have to help you decide.

(If we move forward, I'm happy to pick at any style/format/organization complaints you have :)

For reference

  • I've created a gist w/ some of the resholved linux and macOS outputs (I haven't tried to replicate all option combinations)
  • nixpkgs-review reports:
    • Result of nixpkgs-review run on x86_64-darwin 1

      1 package failed to build:
      • qtpass
      3 packages built:
      • pass (pass-nodmenu)
      • passExtensions.pass-import
      • passff-host

      (Note: qtpass failure is unrelated. It fails on segfaults in localization when sandbox is enabled. Here's my log and hydra's. It sounds like this is the same impurity issue as gnuplot_qt: fix on darwin #82815 (review))

    • Result of nixpkgs-review run on x86_64-linux 1

      9 packages built:
      • krunner-pass
      • pass
      • pass-nodmenu
      • pass-wayland
      • passExtensions.pass-audit
      • passExtensions.pass-import
      • passff-host
      • qtpass
      • rofi-pass

Motivation for this change

Convert pass to use resholvePackage.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.05 Release Notes (or backporting 21.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested review from Ma27, fpletz, globin, lovek323 and tadfisher February 1, 2022 18:15
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Feb 1, 2022
@ofborg ofborg bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. and removed 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Feb 3, 2022
@abathur
Copy link
Copy Markdown
Member Author

abathur commented Feb 3, 2022

  • Force-push is just a rebase on master after the merge of yallback: 0.1.0 -> 0.2.0 #157638

  • Also established that the darwin qtpass failure seen in nixpkgs-review predates this PR and updated first post to reflect this.

@DamienCassou
Copy link
Copy Markdown
Contributor

I suggest splitting the commit into 2 to facilitate review and understanding later on:

  • 1 commit to cleanup the file (I can see you are changing spaces in many places, I also see addition of lib.)
  • 1 commit to introduce the semantic change you want to introduce

++ optionals (waylandSupport && dmenuSupport) [ ydotool dmenu-wayland ]
++ optionals (x11Support && dmenuSupport) [ xdotool dmenu ]
);
solutions = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My impression is that this is very complex. The original file was 168 lines long which seems quite large already for a small tool such as pass. The new version contains 306 lines. I agree many added lines are just comments but these comments increase the impression of complexity: there is no reason to comment something simple in my opinion.

I would prefer an upstream patch grouping all external tool references into functions or variables. Then nixpkgs can just patch these lines. The result would be simpler and upstream would be better too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no reason to comment something simple in my opinion.

As in outer, this is a draft of a change that uses an API that most of the maintainers/mentioned are likely not familiar with yet. The comments are for this audience, and I expect to pare them back to whatever the maintainers find helpful if the work moves forward.

My impression is that this is very complex.

This is, of course, the discussion the draft is here to start. Is a device with with all of its screws more complex than a device missing a few?

@abathur
Copy link
Copy Markdown
Member Author

abathur commented Feb 4, 2022

I suggest splitting the commit into 2 to facilitate review and understanding later on:

  • 1 commit to cleanup the file (I can see you are changing spaces in many places, I also see addition of lib.)
  • 1 commit to introduce the semantic change you want to introduce

I guess I didn't make it explicit, but this draft picks from an existing patch with overlapping style/semantic changes. I expect to cross this bridge if the change gets a thumbs-up.

@DamienCassou
Copy link
Copy Markdown
Contributor

I've tried this during the previous days and everything seems to work fine for me.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 12, 2022
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 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.

3 participants