bitbox-bridge: init at 1.6.1#387888
Conversation
34c895c to
45bbb66
Compare
There was a problem hiding this comment.
please do not use with lib; or anything like that. It increases eval times and is generally bad practice
There was a problem hiding this comment.
I've removed it, however just for my curiousity & understanding, could you elaborate why it increases eval times? I'd appreciate a reference to the good practice(s) as well, it is my first nixpkgs contribution! :)
45bbb66 to
7d89f2f
Compare
|
Requested changes done @ethancedwards8 , please have a look once more ;) |
There was a problem hiding this comment.
Any reasons for disabled checks?
There was a problem hiding this comment.
I just wanted to speed up package builds/re-installations. Would it not speed up the builds?
There was a problem hiding this comment.
Always do checks. Build speed is irrelevant because hydra provides a binary cache.
There was a problem hiding this comment.
So when I do nixos-rebuild or home-manager switch these checks won't run in CI? Just for my understanding, I removed the doCheck = false;
There was a problem hiding this comment.
When this package is merged into Nixpkgs, the build will be created and checked by hydra and then a binary will be cached. Whenever you put this package into your config, your system will download the binary from cache.nixos.org. You will not have to build the package or run any tests on your system, those will be done by hydra.
There was a problem hiding this comment.
Thanks for the good explanation @ethancedwards8 ! I guess I've worried about this because I've been cross-compiling an image for my Raspberry Pi, then these tests do run, however I suppose thats a good thing, as I assume there is no way to differentiate between a Hydra build and a cross-compilation build on local. Let me know if there is anything else needed for your approval of this PR ;) Thank you!
7d89f2f to
b5f4bc5
Compare
|
Hi @HeitorAugustoLN , I've done all the requested changes, and have one question regarding the |
bf6b029 to
61e2fd5
Compare
|
Hi @HeitorAugustoLN, looking forward to your approval as well. Let's get this merged! ;) |
Going to take a look at it soon |
|
There was a problem hiding this comment.
Package currently does not build on darwin because of this dependency
| buildInputs = [ | |
| libudev-zero | |
| ]; | |
| buildInputs = lib.optionals stdenv.hostPlatform.isLinux [ | |
| libudev-zero | |
| ]; |
There was a problem hiding this comment.
Fixed, thanks for pointing it out!
61e2fd5 to
2ce8e27
Compare
2ce8e27 to
dacf246
Compare
|
dacf246 to
6a284d1
Compare
|
HeitorAugustoLN
left a comment
There was a problem hiding this comment.
LGTM! Tested on linux and darwin
6a284d1 to
c174900
Compare
|
@SuperSandro2000 done the requested changes, please have a look/approve ;) |
|
@SuperSandro2000 @JohnRTitor @ambroisie Can we get this PR merged? All the requested changes are made and I've 2 approvals already. Let me know if anything else is missing but I suppose this PR is ready to be merged. |
There was a problem hiding this comment.
Please migrate to the new finalAttrs pattern, consider rebasing to latest master first, as finalAttrs support got merged a while ago.
There was a problem hiding this comment.
I'm going to push back on this. It is not yet clear that finalAttrs will be officially adopted and moved to treewide. Additionally, it increases eval times.
There was a problem hiding this comment.
it increases eval times
Not that significant increase to despise the advantages it has, in my opinion.
There was a problem hiding this comment.
@JohnRTitor could you refer me to the finalAttrs pattern you mention more specifically?
I've found these links on the web but it is still NOT clear to me how I can achieve this here, perhaps you can provide a specimen?:
#194475
https://discourse.nixos.org/t/is-it-possible-to-override-cargosha256-in-buildrustpackage/4393/22
Merged PR: #234651
I'm still not sure how to implement this and there is no clear documentation it seems on what is being asked @JohnRTitor
There was a problem hiding this comment.
Take a look at https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/co/cosmic-files/package.nix#L12 for an example
There was a problem hiding this comment.
@HeitorAugustoLN previously I could run:
nix-build -E 'with import <nixpkgs> { }; callPackage ./pkgs/by-name/bi/bitbox-bitbox/package.nix { }'and it would build this package. How can I build it now? I get this error:
16:18:06 izelnakri ~/Github/nixpkgs -> package-bitbox-bridge nix-build -E 'with import <nixpkgs> { }; callPackage ./pkgs/by-name/bi/bitbox-bridge/package.nix { }'
error:
… while calling a functor (an attribute set with a '__functor' attribute)
at /nix/store/s3bhg6p26cqi1hm2lamjjdwaiq49g2hn-source/lib/customisation.nix:264:13:
263| in if missingArgs == {}
264| then makeOverridable f allArgs
| ^
265| # This needs to be an abort so it can't be caught with `builtins.tryEval`,
… while evaluating a branch condition
at /nix/store/s3bhg6p26cqi1hm2lamjjdwaiq49g2hn-source/lib/customisation.nix:148:7:
147| in
148| if isAttrs result then
| ^
149| result // {
(stack trace truncated; use '--show-trace' to show the full, detailed trace)
error: expected a set but found a function: «lambda @ /home/izelnakri/Github/nixpkgs/pkgs/by-name/bi/bitbox-bridge/package.nix:10:32»There was a problem hiding this comment.
You should run nix build .#bitbox-bridge to build this package (if you have enabled flakes), otherwise nix-build -a bitbox-bridge should run fine.
JohnRTitor
left a comment
There was a problem hiding this comment.
Otherwise LGTM. Let me know (@) when you have done the changes and I will merge.
c174900 to
435d37a
Compare
pkgs/by-name/bi/bitbox-bridge/rules.d/50-hid-digitalbitbox.rules
Outdated
Show resolved
Hide resolved
435d37a to
f920286
Compare
f920286 to
05fd3d9
Compare
|
maintainers/maintainer-list.nix
Outdated
There was a problem hiding this comment.
This change ideally should be a in a separate commit.
First commit should be:
maintainers: add izelnakri
Second commit is fine as it is:
bitbox-bridge: init at 1.6.1
05fd3d9 to
0c81874
Compare
0c81874 to
f547d8a
Compare
f547d8a to
cc4e462
Compare
bitbox-bridgeis an essential software that allowsBitBox2hardware wallets to communicate with the target machine for blockchain transactions.Things done
./result/bin/)Add a 👍 reaction to pull requests you find important.