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

electrum: 3.3.4 -> 3.3.5 (not your typical version bump) #61211

Merged
merged 5 commits into from May 11, 2019

Conversation

@dtzWill
Copy link
Member

@dtzWill dtzWill commented May 9, 2019

Motivation for this change

Bah, see commits for details.

I thought we'd gone down this path before,
but if we did I can't find the PR on github.

Please take a look :).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dtzWill dtzWill requested a review from FRidh as a code owner May 9, 2019
Not crazy about this solution, but seems better than
not running tests or ignoring upstream's signatures.
@Yarny0
Copy link
Contributor

@Yarny0 Yarny0 commented May 11, 2019

Review per https://nixos.org/nixpkgs/manual/#reviewing-contributions-package-updates

  • package name fits guidelines
  • package version fits guidelines
  • package build on x86_64-linux
  • executables tested on x86_64-linux
  • all depending packages build: I haven't found any

Notes on testing:

  • Tested electrum with https://github.com/Mic92/nix-review on nixos-unstable in xfce.
  • Used so-called "watching-only wallet" (no private keys). Listing addresses, transactions, history etc. works -- apparently without problem. Conversion of wallet value to fiat currency works.
  • The patched .desktop file works, i.e., it starts the problem as expected.
  • Not tested doing any transactions.

General comments:

  • Notifying electrum maintainers: @ehmry @joachifm @np
  • This pull request touches python3Packages.aiorpcx, which has no maintainer.
  • Licenses are still correct (still MIT for both electrum and python3Packages.aiorpcx).
  • The strange PATH that is set in the upstream .desktop file looks to me like a debugging leftover. Maybe upstream remove it if a patch is provided to them.
  • Summary: To me this appears suitable for merging 👍

@joachifm
Copy link
Contributor

@joachifm joachifm commented May 11, 2019

Thank you. I failed to notice that the sources had been changed, so I'm very happy they are changed back ... Given the nature of this app it is incredibly important to verify the provenance of the source.

@joachifm joachifm merged commit b6300ba into NixOS:master May 11, 2019
16 checks passed
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

3 participants