Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tmux-thumbs: init at 0.7.1 #162121

Merged
merged 3 commits into from
Apr 8, 2022
Merged

Conversation

ghostbuster91
Copy link
Member

Motivation for this change

Add https://github.com/fcsonline/tmux-thumbs

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.

pkgs/misc/tmux-plugins/tmux-thumbs/default.nix Outdated Show resolved Hide resolved
pkgs/misc/tmux-plugins/tmux-thumbs/default.nix Outdated Show resolved Hide resolved
pkgs/misc/tmux-plugins/tmux-thumbs/fix.patch Outdated Show resolved Hide resolved
pkgs/misc/tmux-plugins/tmux-thumbs/fix.patch Outdated Show resolved Hide resolved
pkgs/tools/misc/thumbs/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/thumbs/fix.patch Outdated Show resolved Hide resolved
@ghostbuster91 ghostbuster91 force-pushed the init/tmux-thumbs branch 3 times, most recently from ea6a94d to dfd7745 Compare February 28, 2022 20:09
Copy link
Contributor

@legendofmiracles legendofmiracles left a comment

Choose a reason for hiding this comment

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

Both diffs also apply to your other derivation

Thanks!

pkgs/tools/misc/thumbs/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/thumbs/default.nix Outdated Show resolved Hide resolved
@ghostbuster91 ghostbuster91 force-pushed the init/tmux-thumbs branch 4 times, most recently from 30f5460 to efc7b2a Compare March 2, 2022 19:58
Copy link
Contributor

@hqurve hqurve left a comment

Choose a reason for hiding this comment

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

You forgot to apply the changes suggested by legendofmiracles to this file too.

After this, its good. Thanks

PS: I havent actually tested it as I dont have the means, but it looks good and builds. So, im assuming that it works for you

pkgs/misc/tmux-plugins/tmux-thumbs/default.nix Outdated Show resolved Hide resolved
pkgs/misc/tmux-plugins/tmux-thumbs/default.nix Outdated Show resolved Hide resolved
@ghostbuster91
Copy link
Member Author

You forgot to apply the changes suggested by legendofmiracles to this file too.

After this, its good. Thanks

PS: I havent actually tested it as I dont have the means, but it looks good and builds. So, im assuming that it works for you

Oh I see, sorry got lost a little bit. I will fix the in a minute. Many thanks for the review!

@ghostbuster91
Copy link
Member Author

PS: I havent actually tested it as I dont have the means, but it looks good and builds. So, im assuming that it works for you

Yes, I use it all the time, and my setup is based on this particular changes.

@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-already-reviewed/2617/447

Comment on lines 15 to 19
patches = [ ./fix.patch ];
postPatch = ''
substituteInPlace src/swapper.rs --replace '@@thumbs-bin-dir@@' $out/bin
'';
Copy link
Member

Choose a reason for hiding this comment

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

Please combine this like

patches = [
(substituteAll {
src = ./mpv.patch;
inherit mpv;
})
];

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if it is possible, at last I haven't found a way to do this. The substitution uses out but I cannot use it as a nix variable because the derivation hasn't been created yet.

Following code correctly complains about out being undefined:

 patches = [
    (substituteAll {
      src = ./fix.patch;
      thumbsBinDir = "${out}/bin";
    })
  ];

Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

I think one of the following should work:

 patches = [
    (substituteAll {
      src = ./fix.patch;
      thumbsBinDir = "$out/bin";
    })
  ];

or

 patches = [
    (substituteAll {
      src = ./fix.patch;
      thumbsBinDir = "${placeholder "out"}/bin";
    })
  ];

Copy link
Member Author

@ghostbuster91 ghostbuster91 Mar 11, 2022

Choose a reason for hiding this comment

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

Unfortunately, neither of them works. My debugging capabilities of that are very limited but I was about to observe following errors:
With first solution:

 patches = [
    (substituteAll {
      src = ./fix.patch;
      thumbsBinDir = "$out/bin";
    })
  ];

Not a directory : $out/bin/thumbs

With second solution:

 patches = [
    (substituteAll {
      src = ./fix.patch;
      thumbsBinDir = "${placeholder "out"}/bin";
    })
  ];

Not a directory: /nix/store/some-hash-fix.patch/bin/thumbs

Could it be possible that in the second case we somehow refer to the output of the substituteAll function?

Copy link
Member Author

Choose a reason for hiding this comment

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

@SuperSandro2000 I managed to get rid completely of this one substitute :)

@ghostbuster91 ghostbuster91 force-pushed the init/tmux-thumbs branch 3 times, most recently from d190dac to 44e5145 Compare March 11, 2022 08:26
@ghostbuster91 ghostbuster91 force-pushed the init/tmux-thumbs branch 2 times, most recently from e92d08f to c486aca Compare March 17, 2022 22:31
@ghostbuster91 ghostbuster91 force-pushed the init/tmux-thumbs branch 3 times, most recently from 0cee6b8 to 47ed76c Compare April 3, 2022 10:45
@auroraanna
Copy link
Contributor

Come on ;_; why didn't you mention this PR in #161792? I've made a duplicate PR now. Closing it.

@ghostbuster91
Copy link
Member Author

@papojari Sorry, my bad. I've just simply forgot to do that. Thanks.

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

Successfully merging this pull request may close these issues.

7 participants