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

bitbox-bridge: init at 1.6.1 #387888

Merged
merged 2 commits into from
Mar 19, 2025
Merged

Conversation

izelnakri
Copy link
Contributor

bitbox-bridge is an essential software that allows BitBox2 hardware wallets to communicate with the target machine for blockchain transactions.

Things done

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Mar 7, 2025
@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from 34c895c to 45bbb66 Compare March 7, 2025 12:13
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1 10.rebuild-linux: 1-10 labels Mar 7, 2025
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Mar 7, 2025

doCheck = false;

meta = with stdenv.lib; {
Copy link
Member

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

Copy link
Contributor Author

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! :)

@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from 45bbb66 to 7d89f2f Compare March 7, 2025 13:54
@izelnakri
Copy link
Contributor Author

Requested changes done @ethancedwards8 , please have a look once more ;)

@izelnakri izelnakri requested a review from ethancedwards8 March 7, 2025 14:05
pkgs.libudev-zero
];

doCheck = false;
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

@izelnakri izelnakri Mar 10, 2025

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;

Copy link
Member

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.

Copy link
Contributor Author

@izelnakri izelnakri Mar 10, 2025

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!

@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from 7d89f2f to b5f4bc5 Compare March 10, 2025 02:02
@izelnakri
Copy link
Contributor Author

Hi @HeitorAugustoLN , I've done all the requested changes, and have one question regarding the doCheck. Could you have a look once more?

@izelnakri izelnakri force-pushed the package-bitbox-bridge branch 2 times, most recently from bf6b029 to 61e2fd5 Compare March 10, 2025 03:15
@izelnakri
Copy link
Contributor Author

Hi @HeitorAugustoLN, looking forward to your approval as well. Let's get this merged! ;)

@HeitorAugustoLN
Copy link
Member

Hi @HeitorAugustoLN, looking forward to your approval as well. Let's get this merged! ;)

Going to take a look at it soon

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 11, 2025
@HeitorAugustoLN
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 387888


x86_64-linux

✅ 1 package built:
  • bitbox-bridge

aarch64-linux

✅ 1 package built:
  • bitbox-bridge

Comment on lines 29 to 35
buildInputs = [
libudev-zero
];
Copy link
Member

@HeitorAugustoLN HeitorAugustoLN Mar 11, 2025

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

Suggested change
buildInputs = [
libudev-zero
];
buildInputs = lib.optionals stdenv.hostPlatform.isLinux [
libudev-zero
];

Copy link
Contributor Author

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!

@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from 61e2fd5 to 2ce8e27 Compare March 11, 2025 10:56
@github-actions github-actions bot added 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Mar 11, 2025
@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from 2ce8e27 to dacf246 Compare March 11, 2025 11:13
@HeitorAugustoLN
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 387888


x86_64-linux

✅ 1 package built:
  • bitbox-bridge

aarch64-linux

✅ 1 package built:
  • bitbox-bridge

x86_64-darwin

❌ 1 package failed to build:
  • bitbox-bridge

aarch64-darwin

❌ 1 package failed to build:
  • bitbox-bridge

@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from dacf246 to 6a284d1 Compare March 11, 2025 16:26
@HeitorAugustoLN
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 387888


x86_64-linux

✅ 1 package built:
  • bitbox-bridge

aarch64-linux

✅ 1 package built:
  • bitbox-bridge

x86_64-darwin

✅ 1 package built:
  • bitbox-bridge

aarch64-darwin

✅ 1 package built:
  • bitbox-bridge

Copy link
Member

@HeitorAugustoLN HeitorAugustoLN left a 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

@FliegendeWurst FliegendeWurst changed the title bitbox-bridge: init at v1.6.1 bitbox-bridge: init at 1.6.1 Mar 12, 2025
@FliegendeWurst FliegendeWurst added the 8.has: package (new) This PR adds a new package label Mar 12, 2025
@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from 6a284d1 to c174900 Compare March 12, 2025 14:15
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 12, 2025
@izelnakri
Copy link
Contributor Author

@SuperSandro2000 done the requested changes, please have a look/approve ;)

@izelnakri
Copy link
Contributor Author

izelnakri commented Mar 18, 2025

@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 {
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@izelnakri izelnakri Mar 19, 2025

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»

Copy link
Contributor

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.

Copy link
Contributor

@JohnRTitor JohnRTitor left a 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.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 19, 2025
@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from c174900 to 435d37a Compare March 19, 2025 15:09
@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from 435d37a to f920286 Compare March 19, 2025 17:01
@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from f920286 to 05fd3d9 Compare March 19, 2025 17:29
@JohnRTitor
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review-gha

Command: nixpkgs-review pr 387888

Logs: https://github.com/JohnRTitor/nixpkgs-review-gha/actions/runs/13952630384


x86_64-linux

✅ 1 package built:
  • bitbox-bridge

aarch64-linux

✅ 1 package built:
  • bitbox-bridge

x86_64-darwin

✅ 1 package built:
  • bitbox-bridge

aarch64-darwin

✅ 1 package built:
  • bitbox-bridge

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnRTitor done ;)

@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from 05fd3d9 to 0c81874 Compare March 19, 2025 19:00
@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from 0c81874 to f547d8a Compare March 19, 2025 19:01
@izelnakri izelnakri force-pushed the package-bitbox-bridge branch from f547d8a to cc4e462 Compare March 19, 2025 19:42
@JohnRTitor JohnRTitor merged commit 3a48773 into NixOS:master Mar 19, 2025
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1 This PR was reviewed and approved by one reputable person 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants