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

farge: init at 1.0.8 #93793

Closed
wants to merge 2 commits into from
Closed

Conversation

loafofpiecrust
Copy link

@loafofpiecrust loafofpiecrust commented Jul 25, 2020

Motivation for this change

I like using farge for picking colors, since there doesn't seem to be a solid equivalent on Wayland.
I'm also excited to make my first contribution to nixpkgs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux) (only tested in Wayland)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests) (N/A)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@laikq
Copy link
Contributor

laikq commented Jul 27, 2020

Hello :) I'm not a nixpkgs guru. But I would add two things:

  1. Adding a package to top-level/all-packages isn't worth a separate commit, this change should go into farge: init at 1.0.8. I think instead it would make more sense to have two commits, maintainers: add loafofpiecrust (where you only add yourself to the maintainers list) and farge: init at 1.0.8.
  2. This packages doesn't work for me on X11, it seems it needs colorpicker for that.

sha256 = "08z7dgc3wygl6s922s4gsi2fkrfmghzy82ivnivxr293b9mll7ar";
};

propagatedBuildInputs = with pkgs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is there a reason this isn't buildInputs? I don't see why these inputs should be propagated...

Copy link
Author

Choose a reason for hiding this comment

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

I added them as propagated because farge needs these programs on path at runtime, and my impression of the difference is that buildInputs doesn't do this. Maybe I misunderstand the difference?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main difference between propagatedBuildInputs and buildInputs is that: If a derivation A has a derivation B in its buildInputs, and derivation B has some derivations C, D, E in its propagatedBuildInputs, then A can see B, C, D and E in its PATH variable at build-time (and therefore A can, for example, save the path to C, D and E and execute them later at run-time). If C, D, and E are in the buildInputs (instead of propagatedBuildInputs) of B, then A only has B in its PATH at run-time. I hope someone corrects me if I'm wrong 🙃

But, I'm fairly certain that Nix doesn't "sandbox" any executables by default. What I mean is that the environment in which farge runs is the same as the environment from which it is executed, so PATH isn't changed by Nix, and farge can't find its buildInputs at run-time. But, this can be fixed easily, using the makeWrapper and wrapProgram shell functions from stdenv. I always use ripgrep to find out real-world examples of how to use NIx functions, and it seems you can just do something like this: wrapProgram $out/bin/farge --prefix PATH : ${stdenv.lib.makeBinPath buildInputs} (in postInstall for example) to make all buildInputs visible to farge via the PATH variable at run-time.

Now, I wonder how you got the packages available at run-time, though. Could it be that you already have wl-clipboard, slurp and grim installed in your user environment, maybe? Or ran farge from nix-shell? If not, I have probably misunderstood something.

Copy link
Member

Choose a reason for hiding this comment

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

@laikq You're very much on point with your explanation, nice!

@loafofpiecrust
Copy link
Author

loafofpiecrust commented Jul 28, 2020

@laikq Thank you for the feedback! I suspected it wouldn't work on X11 as it does require colorpicker there. I've only been using this on Wayland. It seems like colorpicker is not already in nixpkgs, and I don't have motivation to write that up too. Is that a deal breaker for initial inclusion here?
EDIT: Looks like there's a PR for colorpicker at #85259

wrapProgram "$out/bin/farge" --prefix PATH : "${lib.makeBinPath buildInputs}"
'';

meta = with stdenv.lib; {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = with stdenv.lib; {
meta = with lib; {

Comment on lines +6 to +7
version = "1.0.8";
src = fetchFromGitHub {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version = "1.0.8";
src = fetchFromGitHub {
version = "1.0.8";
src = fetchFromGitHub {

@stale
Copy link

stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@SuperSandro2000
Copy link
Member

Closing due to inactivity from author.

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.

None yet

6 participants