-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
bitbox-bridge: init at 1.6.1 #387888
bitbox-bridge: init at 1.6.1 #387888
Conversation
34c895c
to
45bbb66
Compare
|
||
doCheck = false; | ||
|
||
meta = with stdenv.lib; { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;) |
pkgs.libudev-zero | ||
]; | ||
|
||
doCheck = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reasons for disabled checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to speed up package builds/re-installations. Would it not speed up the builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always do checks. Build speed is irrelevant because hydra provides a binary cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
|
buildInputs = [ | ||
libudev-zero | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks for pointing it out!
61e2fd5
to
2ce8e27
Compare
2ce8e27
to
dacf246
Compare
|
dacf246
to
6a284d1
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
libudev-zero, | ||
}: | ||
|
||
rustPlatform.buildRustPackage rec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it increases eval times
Not that significant increase to despise the advantages it has, in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JohnRTitor done ;)
05fd3d9
to
0c81874
Compare
0c81874
to
f547d8a
Compare
f547d8a
to
cc4e462
Compare
bitbox-bridge
is an essential software that allowsBitBox2
hardware wallets to communicate with the target machine for blockchain transactions.Things done
./result/bin/
)Add a 👍 reaction to pull requests you find important.