copyDesktopItems: add new setup-hook #105151

B4dM4n · 2020-11-27T14:29:13Z

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:

nixpkgs/pkgs/applications/audio/goattracker/default.nix

Line 56 in 05d408e

${desktopItem.buildCommand}

nixpkgs/pkgs/applications/networking/mailreaders/thunderbird/68.nix

Line 291 in 05d408e

${desktopItem.buildCommand}

nixpkgs/pkgs/applications/networking/mailreaders/thunderbird/default.nix

Line 286 in 05d408e

${desktopItem.buildCommand}

nixpkgs/pkgs/applications/office/softmaker/generic.nix

Lines 114 to 116 in 05d408e

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

nixpkgs/pkgs/games/minecraft/default.nix

Line 114 in 05d408e

${desktopItem.buildCommand}

nixpkgs/pkgs/games/xonotic/default.nix

Line 154 in 05d408e

${desktopItem.buildCommand}

nixpkgs/pkgs/tools/misc/jdiskreport/default.nix

Line 41 in 05d408e

${desktopItem.buildCommand}

nixpkgs/pkgs/tools/security/jd-gui/default.nix

Line 95 in 05d408e

${(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

CC: @flokli @samueldr

SuperSandro2000 · 2020-11-27T15:01:16Z

pkgs/build-support/setup-hooks/copy-desktop-items.sh

+postInstallHooks+=(copyDesktopItems)
+
+copyDesktopItems() {
+    if [ "${dontCopyDesktopItems-}" = 1 ]; then return; fi


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?

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

nixpkgs/pkgs/build-support/setup-hooks/move-lib64.sh

Line 11 in 05d408e

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

nixpkgs/pkgs/build-support/setup-hooks/move-sbin.sh

Line 8 in 05d408e

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

nixpkgs/pkgs/build-support/setup-hooks/update-autotools-gnu-config-scripts.sh

Line 4 in 05d408e

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.

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

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

ghost · 2020-11-29T03:09:36Z

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.

ghost · 2020-11-29T03:30:05Z

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 · 2020-11-29T06:23:30Z

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 · 2020-11-29T12:26:14Z

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.

B4dM4n · 2020-11-29T13:23:00Z

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 · 2020-11-29T15:29:02Z

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 · 2020-11-29T16:49:44Z

Did you envision something tike this faee4397a10cf53fbe5f5741c91724623a23b10c?

https://github.com/NixOS/nixpkgs/blob/faee4397a10cf53fbe5f5741c91724623a23b10c/pkgs%2Fbuild-support%2Fmake-desktopitem%2Fdefault.nix#L51-L58

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

Ericson2314 · 2020-11-29T18:01:36Z

@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 · 2020-11-29T21:53:43Z

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

nixpkgs/nixos/modules/misc/documentation.nix

Lines 62 to 77 in 9a01e97

    
           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 · 2020-11-30T15:31:57Z

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

B4dM4n requested a review from Ericson2314 as a code owner November 27, 2020 14:29

B4dM4n force-pushed the copy-desktop-hook branch from db7db09 to 40072bd Compare November 27, 2020 14:41

ofborg bot added 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Nov 27, 2020

SuperSandro2000 reviewed Nov 27, 2020

View reviewed changes

B4dM4n added 4 commits November 29, 2020 04:03

copyDesktopItems: add new setup-hook

50f54c5

goattracker: use copyDesktopItems hook

65f8183

thunderbird: use copyDesktopItems hook

c3598cc

softmaker-office: use copyDesktopItems hook

ab345be

ghost force-pushed the copy-desktop-hook branch from 79b0aab to 1d5b0dd Compare November 29, 2020 03:12

ghost approved these changes Nov 29, 2020

View reviewed changes

B4dM4n added 3 commits November 29, 2020 04:17

minecraft: use copyDesktopItems hook

81e2df3

xonotic: use copyDesktopItems hook

74088ba

jdiskreport: use copyDesktopItems hook

5fbffec

ghost force-pushed the copy-desktop-hook branch from 1d5b0dd to c96befc Compare November 29, 2020 03:17

jd-gui: use copyDesktopItems hook

b65a1ab

ghost force-pushed the copy-desktop-hook branch from c96befc to b65a1ab Compare November 29, 2020 03:27

This was referenced Nov 29, 2020

thunderbird fails to build, blocks unstable #103071

Closed

make-desktopitem: desktop-file-utils is a nativeBuildInput #105099

Merged

FRidh merged commit db29c15 into NixOS:master Nov 29, 2020

ghost mentioned this pull request Dec 19, 2020

xonotic .desktop file is broken #107219

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

copyDesktopItems: add new setup-hook #105151

copyDesktopItems: add new setup-hook #105151

B4dM4n commented Nov 27, 2020

SuperSandro2000 Nov 27, 2020

B4dM4n Nov 27, 2020

ghost Nov 29, 2020

Ericson2314 Nov 29, 2020

ghost commented Nov 29, 2020

ghost commented Nov 29, 2020 •

edited by ghost

Ericson2314 commented Nov 29, 2020 •

edited

FRidh commented Nov 29, 2020 •

edited

B4dM4n commented Nov 29, 2020

Ericson2314 commented Nov 29, 2020

B4dM4n commented Nov 29, 2020

Ericson2314 commented Nov 29, 2020 •

edited

B4dM4n commented Nov 29, 2020

Ericson2314 commented Nov 30, 2020

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

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

copyDesktopItems: add new setup-hook #105151

copyDesktopItems: add new setup-hook #105151

Conversation

B4dM4n commented Nov 27, 2020

Motivation for this change

Things done

SuperSandro2000 Nov 27, 2020

Choose a reason for hiding this comment

B4dM4n Nov 27, 2020

Choose a reason for hiding this comment

ghost Nov 29, 2020

Choose a reason for hiding this comment

Ericson2314 Nov 29, 2020

Choose a reason for hiding this comment

ghost commented Nov 29, 2020

ghost commented Nov 29, 2020 • edited by ghost

Ericson2314 commented Nov 29, 2020 • edited

FRidh commented Nov 29, 2020 • edited

B4dM4n commented Nov 29, 2020

Ericson2314 commented Nov 29, 2020

B4dM4n commented Nov 29, 2020

Ericson2314 commented Nov 29, 2020 • edited

B4dM4n commented Nov 29, 2020

Ericson2314 commented Nov 30, 2020

ghost commented Nov 29, 2020 •

edited by ghost

Ericson2314 commented Nov 29, 2020 •

edited

FRidh commented Nov 29, 2020 •

edited

Ericson2314 commented Nov 29, 2020 •

edited