Using fetchFromGitHub is preferred I think.

Is there a way to work around this? Otherwise we should provide a wrapper script that gives the user instructions. A comment in the .nix file is not easily discoverable by end users.

It could be worked around by changing the location it saves to, but then a decision would have to be made where that should be. A wrapper script that warns if the environment variable is not set is a good idea though.

Sounds like that should be in the home directory. Where does it regularly save it?

But either way, your call. I don't use the application. Just silently failing without giving the user a hint for the reason is sub-optimal.

It looks like it wants to save it to etc, which seems very odd to me, because then it can't be used by multiple users. I think that changing it only for nixpkgs would be confusing, so I'm going to leave it as-is, since I'm happy using the environment variable. That way if somebody else wants to use the file approach they can change it to be what they think makes sense.

Yes, that seems reasonable (unlike the upstream approach of saving user configuration to /etc). But then please add a wrapper that notifies the user and maybe add a comment explaining the alternative solution (in case somebody wants to improve it in the future).

Will do.

It is customary to add the version to the name: name = "slack-cli-${version}".

This shouldn't be necessary. The standard build phase uses make, it does not need to be specified as a dependency.

Where do you actually use make?

The makeWrapper dependency is for the installPhase where wrapProgram is called.

Ooh that is a confusing name. I assumed it was a wrapper for make 😀

It should be a nativeBuildInput then. The difference is that nativeBuildInputs are used exclusively during packaging. The distinction is important for cross-compilation.

Alternatively you could leave it out of any buildInputs and just use ${makeWrapper}/bin/wrapProgram in installPhase.

fwiw:

$ git grep -w 'nativeBuildInputs = .*makeWrapper' | wc -l
     444
$ git grep -w 'buildInputs = .*makeWrapper' | wc -l
     474

Alternatively you could leave it out of any buildInputs and just use ${makeWrapper}/bin/wrapProgram in installPhase.

This wouldn't work.

$ nix-shell -p makeWrapper --run 'type -t wrapProgram'
function

fwiw

Yes that is because the distinction is relatively recent, many contributors aren't aware of it and it is not enforced in regular compilation (yet). But the right way to do it is to always put makeWrapper in nativeBuildInputs. Maybe some point in the future we will enforce the distinction for regular compilation.

For example when you want to compile for a raspberry pi on your regular x86 desktop, you will need a x86 version of makeWrapper but aarch versions of all the runtime dependencies.

This wouldn't work.

Huh, I wonder how it works when it is in buildInputs. Anyways, that was a misunderstanding on my side. The way you do it currently is fine (except native vs. regular build inputs).

Cool, thanks for the explanation :)

Please quote shell variables, including $out.

Is there a reason to add all 3 of these to all-packages.nix?

Is there a reason to add all three of these to all-packages.nix?

A reason one might prefer a wrapper derivation is when the unwrapped one takes a long time to rebuild but the wrapper should have a quick turnaround time. Changing something in the wrapper doesn't affect the unwrapped one at all and can therefore be very quick.

It's not really needed here, because the unwrapped one is pretty much just a source, the code to wrap it can be put into it directly. So I think this should be changed.

@infinisil what do you think of #45055 (comment)?

Personally I'd just inline the wrapper into the main package. Any reason you chose this approach?

There is indeed! See #44102, but the tl;dr is that inlining the wrapper into the main package breaks overrideAttrs, so as a user of the package you can't eg add patches.

It doesn't need to be 2 packages at all: Just "insert" the wrapper into the original derivation like this: ln -s ${wrapper}/bin/slack-cli $out/bin/slack-cli, while moving the original slack-cli binary to $out/bin/.slack-cli-wrapped_

Oh, I see what you're saying! I'll do that.

You can use

let
  wrapper = writeShellScriptBin "slack" ''
    ...
    exec "${slack-cli}/bin/.slack-wrapped"
  '';

  slack-cli = stdenv.mkDerivation rec {
    ...
  };

in slack-cli

(or similar) to not have to use this dirname hack

I tried this! (Or something very similar.) It was infinitely recursive though. Was I doing something wrong? It makes sense to me I think – wrapper takes slack-cli's output path as an input, and slack-cli's output path depends on all of its inputs, one of which is wrapper.

Oh.. never mind then, I thought at some point that this should work (through some magic I don't understand myself), but apparently not

I guess what would work, and what a lot of packages do, is to use cat <<-EOF to write the script to $out/bin/foo in installPhase, such that you have access to the $out variable.

@infinisil that does work (just pushed), although it puts me in shell-escape hell…

Yeah that is a bit of a disadvantage..

What do you think then? I think that, having seen it, I actually prefer this version. Even if it was a bit annoying to write, it’s less hacky.

Agreed!