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.

Sign up for GitHub

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

Jump to bottom

digitalbitbox: init at 2.2.2 #33787

Merged

Mic92 merged 2 commits into NixOS:master from vidbina:add/digitalbitbox

Feb 27, 2018

Contributor

vidbina commented Jan 12, 2018 •

edited

Motivation for this change

I just got a digitalbitbox to play around with and couldn't find the apps (searched for dbb or digitalbitbox) in the nixpkgs index (or any PR or issue for that matter) so here goes...

For context's sake: Squashed it from this branch on top of the latest master which followed from this branch based on my nixos-version --revision. Maybe that helps clarify some decisions made; however messy.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels

Contributor Author

vidbina commented Jan 14, 2018 •

edited

I recently learned about nixpkgs-lint, so ~~I'm pushing~~ I force-pushed a few changes (squashed from a WIP branch) to make this package nixpkgs-lint-compliant 😉.

vidbina force-pushed the add/digitalbitbox branch from 216fce0 to 0a3030f Compare

January 14, 2018 10:19

infinisil requested changes

View reviewed changes

pkgs/applications/misc/digitalbitbox/default.nix Outdated

+                  cp src/libbtc/src/secp256k1/.libs/*.so* $out/lib
+                  cp src/hidapi/libusb/.libs/*.so* $out/lib
+                  cp src/univalue/.libs/*.so* $out/lib
+                  rm -rf $PWD

Member

infinisil Feb 1, 2018

This is not needed, the build directory will be cleaned up by nix automatically

Contributor Author

vidbina Feb 2, 2018

Not removing $PWD results to a forbidden reference error. I once found this as a temporary fix on a nixpkgs issue/pr thread.

pkgs/applications/misc/digitalbitbox/default.nix Outdated

+                  rm -rf $PWD
+                  mkdir -p "$out/etc/udev/rules.d"
+                  printf "SUBSYSTEM==\"usb\", TAG+=\"uaccess\", TAG+=\"udev-acl\", SYMLINK+=\"dbb%%n\", ATTRS{idVendor}==\"03eb\", ATTRS{idProduct}==\"2402\"\n" | tee $out/etc/udev/rules.d/51-hid-digitalbitbox.rules > /dev/null && printf "KERNEL==\"hidraw*\", SUBSYSTEM==\"hidraw\", ATTRS{idVendor}==\"03eb\", ATTRS{idProduct}==\"2402\", TAG+=\"uaccess\", TAG+=\"udev-acl\", SYMLINK+=\"dbbf%%n\"\n" | tee $out/etc/udev/rules.d/52-hid-digitalbitbox.rules > /dev/null

Member

infinisil Feb 1, 2018

Why | tee file > /dev/null instead of directly > file?

Also put these 2 printf's on separate lines, the && doesn't seem needed. And you can use single quotes (') to not need the %% and \"

Contributor Author

vidbina Feb 2, 2018

I copied this from the package's docs. I figured it's easier to maintain parity to the package's instructions in case things change in the future. Otherwise, the person updating this would have to translate that "printf | tee" bit with the added risk of human error.

pkgs/applications/misc/digitalbitbox/default.nix Outdated

+                makeWrapper,
+                pkgconfig,
+                qt59,
+                withDebug ? false

Member

infinisil Feb 1, 2018

Is having this option worth it? Why make it configurable explicitly?

Contributor Author

vidbina Feb 2, 2018

Stripped the option out with 0c5f3d6

pkgs/applications/misc/digitalbitbox/default.nix Outdated

+              in stdenv.mkDerivation rec {
+                name = "dbb-app-${version}";
+                meta = with stdenv.lib; {

Member

infinisil Feb 1, 2018

Nitpick: The meta section is usually at the very bottom

pkgs/applications/misc/digitalbitbox/default.nix Outdated

+                  debugFlag = optionalString withDebug "--enable-debug";
+                in ''
+                  ./autogen.sh
+                  ./configure --prefix=$out ${debugFlag} --enable-libusb $configureFlags

Member

infinisil Feb 1, 2018

Please set the configureFlags attribute (it's a list) and use the default configurePhase for autogen (something like including pkgs.autogen in nativeBuildInputs should do it). Then it's also easy to add the --enable-debug flag for people that need it and you won't have to add an option for it.

pkgs/applications/misc/digitalbitbox/default.nix Outdated

+                ];
+                installPhase = ''
+                  make install

Member

infinisil Feb 1, 2018

Consider also using the default installPhase (which is make install) and add additional stuff with postInstall. I'm not exactly sure on this though.

Contributor Author

vidbina commented Feb 2, 2018 •

edited

I just added separate commits, which I can squash once the review is positive 😉. I'm trying to test this against the real hardware but am having issues with the udev rules. When one installs this package they get to add a udev.packages statement to their nixos-configuration in order to honor the udev rules for this package, however; since I'm building this package through nix-build I'm not sure how to get the udev rules applied to my system. Currently, the app is unable to see the device until I do and I want to perform a thorough test on my own machine so I'll have to test connecting to the device as well.

My first approach was to checkout this branch and attempt to nix-rebuild test against this branch. That way I can add udev.packages to my nixos-configuration and perhaps even add the package in my configuration for installation. The only problem with this is that my machine resorts to pulling all those builds from a month ago (I've upgraded and garbage collected since), which I can't sit through at the moment.

Another approach would be to rebase this entire branch on top of my current nixos-version --revision and then add the udev.packages rule to my configuration and nixos-rebuild-ing it against the rebased branch.

@infinisil how would you go about this? Thanks for the assistance on this 🙇

Contributor

emmanuelrosa commented Feb 2, 2018

Rebasing is perfectly fine.

As for the udev rules, you can create a NixOS module which when enabled adds the rules. You can also have the NixOS module install the package (see nixos/modules/programs/java.nix for an example). That way the user can just do something like programs.digitalbitbox.enabled = true; to install the package and set up the necessary udev rules. Of course, this means the package may have limited, if any, use outside of NixOS.

Contributor Author

vidbina commented Feb 3, 2018 •

edited

Also another nitpick, I've listed the package under the all-packages attribute digitalbitbox but the name is dbb-app-${version}. I'm pretty sure I should be consistent. Now dbb-app is the repository name that I'm pulling this package from, but digitalbitbox is more meaningful at first glance. Shall I just rename all dbb-app names to digitalbitbox?

@emmanuelrosa thanks for the tip. I'll be working on the module next 😉 was looking at the nitrokey, which is relatively similar to what you explained. They added their module in nixos/modules/hardware/nitrokey.nix so I'll basically learn from that and provide a software-only package, but also a hardware module.

As for my other problem. I basically rebased all my work and then cherry-picked it on top of nixos-version --revision in order to apply the udev changes and list this package in my configuration. Just got into the digitalbitbox so it all works 😄... at least on my machine 😨

vidbina force-pushed the add/digitalbitbox branch 2 times, most recently from d6e05be to 865ae77 Compare

February 3, 2018 12:54

Contributor Author

vidbina commented Feb 3, 2018

So here's the current status.

The easiest way to install the Digitalbitbox application is by adding

{
  programs.digitalbitbox.enable = true;
}

to your nixos-configuration which will install the digitalbitbox package and apply the udev rules by enabling the digitalbitbox hardware module.

That programs.digitalbitbox.enable = true line is basically equivalent to:

{
  environment.systemPackages = [ pkgs.digitalbitbox ];
  hardware.digitalbitbox.enable = true;
}

The hardware module for digitalbitbox simply allows the udev rules by means of

{
  services.udev.packages = [ pkgs.digitalbitbox ];
}

Contributor

emmanuelrosa commented Feb 3, 2018

This is looking great 👍

What plan do you have to guide users toward the proper installation procedure? A user may simply try adding digitalbitbox to environment.systemPackages or using nix-env to install it.

Contributor Author

vidbina commented Feb 3, 2018 •

edited

Hey @emmanuelrosa, I don't really have a good plan there. I was thinking about:

providing some usage instructions in the package's expression -- which I just did with this diff
issuing a PR to the NixOS documentation to make a mention of Digital Bitbox and the install procedure
issuing a request for change to the Digital Bitbox Linux installation page in order to add NixOS-specific instructions
embedding a notification or warning on an attempt to just install the software package (I'm clueless as to how I would go about this)
writing a blog post about it (but discoverability on this option is low, hence it is the last resort)

I think option 1 suffices for now, wouldn't you agree? I don't see a good place where I could make a note of this in the NixOS docs and the Digital Bitbox linux installation page is relatively distro agnostic. Not sure they want to start venturing down that road where they'll have to keep and maintain installation instructions for different distros.

Which options do you have mind that I haven't listed yet?

vidbina force-pushed the add/digitalbitbox branch 2 times, most recently from 606b8e7 to 83cabc9 Compare

February 3, 2018 16:00

Contributor

emmanuelrosa commented Feb 3, 2018

Option 1 is very good. The NixOS documentation has a section containing application-specific instructions. Here's an example of one that's short and sweet: https://nixos.org/nixos/manual/index.html#sec-hidepid

Packages can optionally have a long description in the meta set, so that's another option. The long description is visible at https://nixos.org/nixos/packages.html, but I'm not sure if nix-env displays it.

You're right, discover-ability is tough at this point. Hopefully between the code comments, docs, and perhaps also a mention in the package's description, users will find what they need.

vidbina force-pushed the add/digitalbitbox branch from 83cabc9 to 95ea7c8 Compare

February 3, 2018 17:06

Contributor Author

vidbina commented Feb 3, 2018

Okay... I've added the longDescription as proposed by @emmanuelrosa. That sounded like a good idea. 👍

About the manual... the example you gave pertains to a rather general security feature. Structure-wise, I don't think that it would make sense for every maintainer to add information on the usage details for their packages to the manual. I foresee this document becoming a cloud of noise in which it becomes even harder to find things.

Perhaps, I misunderstood you in which case I hope you'll help me clarify what you meant.

Contributor

emmanuelrosa commented Feb 4, 2018

Yeah, that was a bad example on my part. A better one is the Emacs chapter.

My reasoning behind suggesting that you add instructions to the NixOS documentation is that your package requires an abnormal install process. You're right, the documentation should not contain usage details for every package. But documentation is ideal for installs that require something other than the common environment.systemPackages or nix-env due to the current discover-ability limitations. An experienced NixOS user will probably check the Nix expression and see your comments, and intermediate user may see the long description, but new users don't know to look in those places.

My goal is to make the NixOS learning curve shallower (that sounds awful, I know) to encourage more talent to saddle up and become contributors, just like you and I.

vidbina added a commit to vidbina/nixpkgs that referenced this pull request


          digitalbitbox: init at 2.2.2

8a3bc23

The commits that lead to this have been squashed from independent
commits see branch @vidbina/add/digitalbitbox-wip that did the
following:
 - 0a3030f digitalbitbox: init at 2.2.2
 - c18ffa3ffd4 digitalbitbox: moved meta to EOF
 - 0c5f3d6 digitalbitbox: using preConfigure + configureFlags
 - a85b1df digitalbitbox: nativeBuildInputs
 - 90bdd35 digitalbitbox: autoreconfHook
 - 91810ee digitalbitbox: default installPhase & makeWrapper
 - 90e43fb digitalbitbox: doc rm $PWD hack & printf-tee deal
 - fd033b2 digitalbitbox: cleanup, alphabetically sort attrs
 - c590798 digitalbitbox: added hardware module
 - 88e46bc digitalbitbox: added program module
 - ammend to change name: dbb-app -> digitalbitbox
 - ammend to add install instructions based on feedback
 (NixOS#33787 (comment))
 - ammend to add longDescription

vidbina force-pushed the add/digitalbitbox branch from 95ea7c8 to 738ad86 Compare

February 14, 2018 02:10

GrahamcOfBorg added 6.topic: nixos 8.has: documentation 8.has: module (update) 8.has: package (new) labels

vidbina force-pushed the add/digitalbitbox branch from 738ad86 to 59f1332 Compare

February 14, 2018 02:21

vidbina force-pushed the add/digitalbitbox branch from 59f1332 to 282f02d Compare

February 14, 2018 02:28


          digitalbitbox: init at 2.2.2

7ece374

The commits that lead to this have been squashed from independent
commits see branch @vidbina/add/digitalbitbox-wip that did the
following:
 - 0a3030f digitalbitbox: init at 2.2.2
 - c18ffa3ffd4 digitalbitbox: moved meta to EOF
 - 0c5f3d6 digitalbitbox: using preConfigure + configureFlags
 - a85b1df digitalbitbox: nativeBuildInputs
 - 90bdd35 digitalbitbox: autoreconfHook
 - 91810ee digitalbitbox: default installPhase & makeWrapper
 - 90e43fb digitalbitbox: doc rm $PWD hack & printf-tee deal
 - fd033b2 digitalbitbox: cleanup, alphabetically sort attrs
 - c590798 digitalbitbox: added hardware module
 - 88e46bc digitalbitbox: added program module
 - amend to change name: dbb-app -> digitalbitbox
 - amend to add install instructions based on feedback
 (NixOS#33787 (comment))
 - amend to add longDescription
 - moved program to its own dir
 - overridable udev rules handling
 - added docs to manual
 - added package attr to program module
 - added package attr to hardware module

vidbina force-pushed the add/digitalbitbox branch from 282f02d to 7ece374 Compare

February 27, 2018 21:27

Mic92 reviewed

View reviewed changes

nixos/modules/programs/digitalbitbox/doc.xml

+                <para>
+                  Digital Bitbox is a hardware wallet and second-factor authenticator.
+                </para>

Member

Mic92 Feb 27, 2018

Props for writing this documentation!


          digitalbitbox: use libsForQt5.callPackage

bcba375

Mic92 merged commit e41ca3b into NixOS:master

Member

Mic92 commented Feb 27, 2018

Thanks for the pull request. Looked pretty good!

Contributor Author

vidbina commented Feb 27, 2018

@Mic92 thanks for reviewing. This PR has been straddling along for quite a while, after the multiple review session it's at least good to know the outcome was relatively decent acceptable or decent 😄.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment