superseded by #74741

#74741 didn't implement the desktop file and pixmaps, so I'm reopening this PR. It will need to rebased, but I think we can proceed after that.

Rebased and implemented suggested changes, should be good to merge

If you are okay with it, I'd prefer if you move the desktop and pixmap assignments into a let expression surrounding the mkDerivation (and drop the optional). Then use splices in the postInstall as follows:

postInstall = optional withGui ''
  install -Dm644 ${desktop} $out/share/applications/bitcoin-qt.desktop
  install -Dm644 ${pixmap} $out/share/pixmaps/bitcoin128.png
'';

While it is a somewhat a matter of preference, I prefer to have lazy evaluation manage what does and does not need to be downloaded.

Ah, you'll need to float the version number out into the let expression too and inherit it. I think I'm okay with that.

grep "let version" pkgs -R suggests that this is not an uncommon thing to do.

While it is a somewhat a matter of preference, I prefer to have lazy evaluation manage what does and does not need to be downloaded.

Download is lazy already due optional, I do not see benefit here.

Ah, you'll need to float the version number out into the let expression too and inherit it. I think I'm okay with that.

I do not see how this improves anything, why do you think this is cleaner?

I would like to keep things cleaner without using let expression, as I do not see benefits here. I do not think we have guidelines about that, so I am not against it, just do not see benefits.

Download isn't controlled by lazy evaluation. You had to explicitly mark some parts as optional withGui and other parts as not.

Very generally speaking, the more we are explicit about this sort of thing, the more likely it is to cause problems in future revisions when things change and we end up unnecessarily downloading objects for build configurations that do not need them. If we make things implicit than the downloads will exactly occur when the expressions ${desktop} and ${pixmap} end up in the derivation structure's value.

However you strongly prefer things the way you've written them, I won't object any further.

Ok, after reviewing again I agree with suggested changes, as it removes unnecessary optional expression, and also putting version into let removes unnecessary rec. Less code means less room for error, so I will do that, thanks.

should be good now

Were you planning on eliminating the rec?

I couldn't since conditional with doCheck uses it here:

I see. I'm good either way, but if you do choose to abstract the doCheck then move to a top-level function argument with a default value of true.

whupsie, sorry for closing to early, say when this is ready for merge

I don't know what the correct -m permissions ought to be, but 644 seems like a fairly commonly used value. Looks good to me. @prusnak or @jb55 did you want to re-review?