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

Add `python3Packages.ckcc-protocol` package, and list it as a dependency for `electrum`. #66516

Merged
merged 2 commits into from Dec 14, 2019

Conversation

@hkjn
Copy link
Contributor

@hkjn hkjn commented Aug 12, 2019

Motivation for this change

The https://coldcardwallet.com/ is a hardware wallet for Bitcoin. This change adds the python3Packages.ckcc-protocol package, and lists it as a dependency for electrum.

Similar to keepkey, trezor and btchip, ckcc-protocol is a plugin,
allowing electrum to communicate with the https://coldcardwallet.com/.

Also add hkjn to maintainers, and list hkjn as initial maintainer for ckcc-protocol. (Would be happy to hand over to official Coldcard maintainers at any point, if they are willing.)

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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

N/A

@hkjn
Copy link
Contributor Author

@hkjn hkjn commented Sep 12, 2019

Ping @FRidh. Let me know if there's anything needed from me here to help with the review process and get this closer to being merged.

@hkjn hkjn changed the title Add `python3Packages.ckcc-protocol` package, and list is as a dependency for `electrum`. Add `python3Packages.ckcc-protocol` package, and list it as a dependency for `electrum`. Sep 20, 2019
@hkjn hkjn force-pushed the hkjn:20190812-add-ckcc branch from 1d2affa to 542ea0f Oct 2, 2019
@hkjn
Copy link
Contributor Author

@hkjn hkjn commented Oct 2, 2019

cc @mmahut @worldofpeace.

Sorry for semi-randomly pulling both of you into this PR, but I wanted to check for your advice on how to proceed with this ~1.5 months old PR to add a new package and list it as a dependency for electrum, to be able to use Coldcard devices with Electrum. I haven't received a review so far from @FRidh, who was auto-added as code owner.

Thanks in advance!

@jb55
jb55 approved these changes Oct 27, 2019
@mmahut
Copy link
Member

@mmahut mmahut commented Oct 27, 2019

I'm sorry for missing this mention.

@GrahamcOfBorg build pythonPackages.ckcc-protocol python3Packages.ckcc-protocol

@mmahut
Copy link
Member

@mmahut mmahut commented Oct 27, 2019

Looks like ckcc-protocol requires python 3.6+, please use something like disabled = ! isPy3k; in the package.

@hkjn hkjn force-pushed the hkjn:20190812-add-ckcc branch from 542ea0f to 76454ef Oct 27, 2019
@hkjn hkjn requested a review from jonringer as a code owner Oct 27, 2019
@hkjn
Copy link
Contributor Author

@hkjn hkjn commented Oct 27, 2019

Thanks for the review @mmahut and @jb55, please take another look!

Looks like ckcc-protocol requires python 3.6+, please use something like disabled = ! isPy3k; in the package.

I added the following to ckcc-protocol package, using other Python packages as a reference:

disabled = ! pythonAtLeast "3.6";

I've also bumped the package version to 0.8.0, which was published between the time I sent this PR out and at time of writing.

@ofborg ofborg bot requested review from ehmry, np and joachifm Oct 27, 2019
@hkjn hkjn force-pushed the hkjn:20190812-add-ckcc branch from 76454ef to 7b71629 Oct 27, 2019
@hkjn
Copy link
Contributor Author

@hkjn hkjn commented Oct 27, 2019

Thanks for the review @jonringer. I've addressed your suggestions at this time, please take another look.

@mmahut
Copy link
Member

@mmahut mmahut commented Oct 27, 2019

@GrahamcOfBorg build python3Packages.ckcc-protocol electrum

hkjn added 2 commits Aug 11, 2019
The ckcc-protocol package installs
https://github.com/Coldcard/ckcc-protocol, which
allows communication with the https://coldcardwallet.com/
hardware wallet.
Similar to keepkey, trezor and btchip, ckcc-protocol is a plugin,
allowing electrum to communicate with the https://coldcardwallet.com/
hardware wallet.
@hkjn
Copy link
Contributor Author

@hkjn hkjn commented Dec 14, 2019

Ping @jonringer @mmahut @jb55. Any chance for a review so this ~4 month old PR can move towards merging?

Thanks in advance!

@hkjn hkjn force-pushed the hkjn:20190812-add-ckcc branch from 7b71629 to 9f483af Dec 14, 2019
@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 14, 2019

the rebase, "Showing 4,569 changed files with 111,966 additions and 116,126 deletions.". That's a lot of changes in a few months O.o

@hkjn
Copy link
Contributor Author

@hkjn hkjn commented Dec 14, 2019

the rebase, "Showing 4,569 changed files with 111,966 additions and 116,126 deletions.". That's a lot of changes in a few months O.o

Agreed, although with 200k+ commits in the repo, I guess this is to be expected? https://github.com/NixOS/nixpkgs/commits/master

Or did I misunderstand, and there's some issue with the two commits I'd like to merge here, or the way I'm rebasing on top of recent commits in upstream's master?

@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 14, 2019

you're good, it's just the force push "brings along all the changes" in the force-push view. Your PR looks good though, reviewing now

Copy link
Contributor

@jonringer jonringer left a comment

diff LGTM
commits LGTM
ckcc shows usage
electrum starts up just fine

only a very small difference in closure size:

$ nix path-info -Sh ./result
/nix/store/if5bzxc9ls6wmrql4fqkfsq3hbv0h1d8-electrum-3.3.8	 654.4M
[nix-shell:/home/jon/.cache/nix-review/pr-66516]$ nix path-info -Sh ./results/electrum
/nix/store/nmc3fraynkghqjm0cysgf38l9f50yz4m-electrum-3.3.8	 654.5M
[5 built, 29 copied (17.1 MiB), 10.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/66516
3 package were built:
electrum python37Packages.ckcc-protocol python38Packages.ckcc-protocol
@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 14, 2019

@GrahamcOfBorg build electrum python37Packages.ckcc-protocol python38Packages.ckcc-protocol

@jonringer jonringer merged commit 9ace40c into NixOS:master Dec 14, 2019
18 checks passed
18 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
electrum on aarch64-linux Success
Details
electrum on x86_64-linux Success
Details
electrum, python37Packages.ckcc-protocol, python38Packages.ckcc-protocol on aarch64-linux Success
Details
electrum, python37Packages.ckcc-protocol, python38Packages.ckcc-protocol on x86_64-darwin Success
Details
electrum, python37Packages.ckcc-protocol, python38Packages.ckcc-protocol 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
@hkjn hkjn deleted the hkjn:20190812-add-ckcc branch Dec 25, 2019
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

4 participants
You can’t perform that action at this time.