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

copyDesktopItems: add new setup-hook #105151

Merged
merged 8 commits into from Nov 29, 2020
Merged

copyDesktopItems: add new setup-hook #105151

merged 8 commits into from Nov 29, 2020

Conversation

@B4dM4n
Copy link
Contributor

@B4dM4n B4dM4n commented Nov 27, 2020

Motivation for this change

A change made to the makeDesktopItem helper introduced by #105099 broke the build of some packages.

These packages used the desktopItem.buildCommand string to install their desktop files directly, instead of using the desktopItem output:

${desktopItem.buildCommand}



${desktopItems.planmaker.buildCommand}
${desktopItems.presentations.buildCommand}
${desktopItems.textmaker.buildCommand}

${desktopItem.buildCommand}

${desktopItem.buildCommand}

${desktopItem.buildCommand}

${(desktopItem "$out/bin/jd-gui").buildCommand}

This PR adds the copyDesktopItems setup hook, which simplifies the process of adding desktop files to a package. It is invoked as a postInstallHook and will install the files given to the desktopItems attribute into the out output.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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)
  • 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.

CC: @flokli @samueldr

postInstallHooks+=(copyDesktopItems)

copyDesktopItems() {
if [ "${dontCopyDesktopItems-}" = 1 ]; then return; fi
Copy link
Member

@SuperSandro2000 SuperSandro2000 Nov 27, 2020

Suggested change
if [ "${dontCopyDesktopItems-}" = 1 ]; then return; fi
[[ ${dontCopyDesktopItems-} == 1 ]] && return

Normally in bash I would write the following but this looks more like sh or dash but you told shellcheck it is bash. I am a bit confused. Would you mind clearing this up?

Copy link
Contributor Author

@B4dM4n B4dM4n Nov 27, 2020

I took this syntax directly from another hook. There are many which use the same style, so I copied it:

if [ "${dontMoveLib64-}" = 1 ]; then return; fi

if [ "${dontMoveSbin-}" = 1 ]; then return; fi

if [ -n "${dontUpdateAutotoolsGnuConfigScripts-}" ]; then return; fi

I'm currently unaware if there is any policy about the hook sh/bash syntax, so I tried to stick with the existing hook style.

Copy link
Member

@petabyteboy petabyteboy Nov 29, 2020

I'm pretty sure both are valid bash. Isn't bash a superset of sh anyways?

Copy link
Member

@Ericson2314 Ericson2314 Nov 29, 2020

Use [[ .. ]] it's better behaved. And when in doubt see what shellcheck says hehe (though I think it will instruct on both).

@petabyteboy
Copy link
Member

@petabyteboy petabyteboy commented Nov 29, 2020

I guess the alternative would be to change all derivations using the old pattern to just copy the files using install, ln, or cp as some other derivations already do. I do like this solution more.
Either way if there are no new concerns I would like to merge soon since many of the applications I use are broken on master and fixed by this change.

@petabyteboy petabyteboy force-pushed the copy-desktop-hook branch from 79b0aab to 1d5b0dd Nov 29, 2020
@petabyteboy petabyteboy force-pushed the copy-desktop-hook branch from 1d5b0dd to c96befc Nov 29, 2020
@petabyteboy petabyteboy force-pushed the copy-desktop-hook branch from c96befc to b65a1ab Nov 29, 2020
@petabyteboy
Copy link
Member

@petabyteboy petabyteboy commented Nov 29, 2020

Built minecraft, xonotic-sdl, thunderbird, jd-gui, softmaker-office, goattracker on x86_64-linux and confirmed the .desktop files were included in the output.

I rebased with these three changes:

  • fixed conflict with latest master (minecraft package was changed in master)
  • minecraft was missing a desktopItems = [ desktopItem ];
  • jd-gui was missing a desktopItems = [ desktopItem ];

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Nov 29, 2020

Thanks for cleaning this up! The old way has bugged me too.

Besides the comment above, can you perhaps find a way to share some code with the old makeDesktopItem? (Or explain why do that is no good?)

@FRidh
Copy link
Member

@FRidh FRidh commented Nov 29, 2020

Because #103071 broke the build of thunderbird, and this resolves it, I am going to merge this, even though some improvements have been suggested. It would be nice to see those in a follow-up commit.

@FRidh FRidh merged commit db29c15 into NixOS:master Nov 29, 2020
19 checks passed
@B4dM4n
Copy link
Contributor Author

@B4dM4n B4dM4n commented Nov 29, 2020

I rebased with these three changes:

  • fixed conflict with latest master (minecraft package was changed in master)
  • minecraft was missing a desktopItems = [ desktopItem ];
  • jd-gui was missing a desktopItems = [ desktopItem ];

@petabyteboy Thanks for fixing this.

Besides the comment above, can you perhaps find a way to share some code with the old makeDesktopItem? (Or explain why do that is no good?)

There is nothing wrong with makeDesktopItem and all packages, besides the ones mentioned above, use it correctly by copying or linking the produced output. It's the usage of the .buildCommand attribute, which is the input of the makeDesktopItem derivation, as a shell input of another derivation (which now lacks the required nativeBuildInputs).

This should have never been allowed in the first place. (Question to self: Why are the inputs to derivation(...) available on the output?)

As (hopefully) all the bad uses of makeDesktopItem are now removed from nixpkgs, I don't see a reason to document exactly this usage, since all other examples in nixpkgs that people can use yield a correct result.

Documenting the hook in general might be a good idea.

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Nov 29, 2020

There is nothing wrong with makeDesktopItem and all packages, besides the ones mentioned above, use it correctly by copying or linking the produced output. It's the usage of the .buildCommand attribute, which is the input of the makeDesktopItem derivation, as a shell input of another derivation (which now lacks the required nativeBuildInputs).

I agree with that. But I don't think the fact that makeDesktopItem isn't bad is a reason not share code?

Question to self: Why are the inputs to derivation(...) available on the output?

Nix choose to implement the builtins.derivation primop that way, but I agree it is dubious.

@B4dM4n
Copy link
Contributor Author

@B4dM4n B4dM4n commented Nov 29, 2020

Did you envision something tike this faee439?

runCommandLocal "${name}.desktop"
{
nativeBuildInputs = [ desktop-file-utils ];
# Provide a dev output that can be used to automatically copy this desktop file
outputs = [ "out" "dev" ];
propagatedNativeBuildInputs = [ copyDesktopItems ];
setupHook = ./setup-hook.sh;
}

This would allow outputs of makeDesktopItem to be used as buildInputs in other derivations. (example 5907605)

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Nov 29, 2020

@B4dM4n OK I guess there are less duplicated than I thought.

OK first a question. When would one use makeDesktopItem without copyDesktopItems? I can't think of any real use-case.

I suppose makeDesktopItem could just put the result in $out not $out/share/applications to hint that it's up to copyDesktopItems to install them in the right place.

@B4dM4n
Copy link
Contributor Author

@B4dM4n B4dM4n commented Nov 29, 2020

Nearly all of the uses of makeDesktopItem in nixpkgs copy or link the desktop files. The only other use is in documentation module:

desktopItem = pkgs.makeDesktopItem {
name = "nixos-manual";
desktopName = "NixOS Manual";
genericName = "View NixOS documentation in a web browser";
icon = "nix-snowflake";
exec = "nixos-help";
categories = "System";
};
in pkgs.symlinkJoin {
name = "nixos-help";
paths = [
helpScript
desktopItem
];
};

Using makeDesktopItem to create directly installable or symlinkable packages which doesn't use copyDesktopItems is a nice feature in that case.

I suppose makeDesktopItem could just put the result in $out not $out/share/applications to hint that it's up to copyDesktopItems to install them in the right place.

This sounds feasable. copyDesktopItems can use stripHash to get the target filename directly from $out. For uses of makeDesktopItem without copyDesktopItems, a attrbiute could be added to use $out/share/applications instead of $out.

Changing the default to $out would sadly break all uses of makeDesktopItem outside of nixpkgs.

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Nov 30, 2020

Ah symlinkJoin is a good enough reason to keep with $out/share/applications. OK, no more issues from me!

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

Successfully merging this pull request may close these issues.

None yet

5 participants