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

bitcoin: install desktop file and pixmaps #72418

Merged
merged 1 commit into from Dec 14, 2019

Conversation

@offlinehacker
Copy link
Contributor

@offlinehacker offlinehacker commented Nov 1, 2019

Motivation for this change

Add desktop file and pixmaps, so we have better intergration

Things done

I had to switch source from one on bitcoin.org to github, as distributed package does not include pixmaps. This is also something that archlinux uses.

  • 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 nix-review --run "nix-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.
Notify maintainers

cc @roconnor

pkgs/applications/blockchains/bitcoin.nix Outdated Show resolved Hide resolved
@offlinehacker offlinehacker force-pushed the xtruder:pkgs/bitcoin/desktop branch from 5038931 to 8d6d7b8 Nov 5, 2019
@jb55
jb55 approved these changes Nov 5, 2019
Copy link
Contributor

@jb55 jb55 left a comment

nice! ACK 8d6d7b8

pkgs/applications/blockchains/bitcoin.nix Outdated Show resolved Hide resolved
@roconnor roconnor mentioned this pull request Nov 30, 2019
3 of 10 tasks complete
@Lassulus
Copy link
Contributor

@Lassulus Lassulus commented Dec 8, 2019

superseded by #74741

@Lassulus Lassulus closed this Dec 8, 2019
@roconnor
Copy link
Contributor

@roconnor roconnor commented Dec 12, 2019

#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.

@roconnor roconnor reopened this Dec 12, 2019
@offlinehacker offlinehacker force-pushed the xtruder:pkgs/bitcoin/desktop branch from 8d6d7b8 to e0dbb07 Dec 12, 2019
@offlinehacker
Copy link
Contributor Author

@offlinehacker offlinehacker commented Dec 12, 2019

Rebased and implemented suggested changes, should be good to merge

@ofborg ofborg bot requested a review from roconnor Dec 12, 2019
@roconnor
Copy link
Contributor

@roconnor roconnor commented Dec 12, 2019

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.

@roconnor
Copy link
Contributor

@roconnor roconnor commented Dec 12, 2019

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.

@offlinehacker
Copy link
Contributor Author

@offlinehacker offlinehacker commented Dec 12, 2019

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.

@offlinehacker
Copy link
Contributor Author

@offlinehacker offlinehacker commented Dec 12, 2019

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?

@offlinehacker
Copy link
Contributor Author

@offlinehacker offlinehacker commented Dec 12, 2019

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.

@roconnor
Copy link
Contributor

@roconnor roconnor commented Dec 12, 2019

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.

@offlinehacker
Copy link
Contributor Author

@offlinehacker offlinehacker commented Dec 12, 2019

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.

@offlinehacker offlinehacker force-pushed the xtruder:pkgs/bitcoin/desktop branch from e0dbb07 to ad9b68c Dec 13, 2019
@offlinehacker
Copy link
Contributor Author

@offlinehacker offlinehacker commented Dec 13, 2019

should be good now

@roconnor
Copy link
Contributor

@roconnor roconnor commented Dec 13, 2019

Were you planning on eliminating the rec?

@offlinehacker
Copy link
Contributor Author

@offlinehacker offlinehacker commented Dec 13, 2019

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

] ++ optionals (!doCheck) [
Since it's no-op I can remove it or refactor in let block, but just keeping rec was easiest, what do you prefer?

@roconnor
Copy link
Contributor

@roconnor roconnor commented Dec 13, 2019

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.

@Lassulus
Copy link
Contributor

@Lassulus Lassulus commented Dec 13, 2019

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

@roconnor
Copy link
Contributor

@roconnor roconnor commented Dec 14, 2019

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?

@jb55
jb55 approved these changes Dec 14, 2019
Copy link
Contributor

@jb55 jb55 left a comment

LGTM

@roconnor roconnor merged commit 6a75080 into NixOS:master Dec 14, 2019
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
bitcoin on aarch64-linux Success
Details
bitcoin on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
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
You can’t perform that action at this time.