add pico-openpgp and pico-fido2#7
Conversation
d4c47bb to
cb553be
Compare
There was a problem hiding this comment.
Excellent changes! I really appreciate you taking the time to review the existing code and reorganize it in a much more maintainable way.
As an initial contributor to this repository, I’d like to get your thoughts on a few "implicit conventions." These were mostly established during past discussions with other project members, and it is my oversight that they weren't reflected in the documentation sooner.
Package Naming
Currently, the packages in this repo use their upstream repository names as the package name. This helps users quickly identify which project is being packaged. I noticed you set the name for pico-fido2 to pico-fido-gpg. While this is a great name that accurately describes the specific implementation, I am slightly hesitant because users looking for the original project might not recognize it. I’d like to hear your thoughts on this.
Custom Build Support
I currently use a packaging format that is fully compatible with nixpkgs. This is to support users who prefer to compile their own firmware (e.g., to change vidpid or enable debug info) without the overhead of setting up a full development environment.
For example, users—including those not using flakes—can run a command like this:
nix-build -E 'with import <nixpkgs> {}; callPackage ./pkgs/pico-fido/default.nix {
picoBoard="waveshare_rp2350_one";
vidpid="Yubikey5";
extraCmakeFlags=[ "-DPICO_DEBUG_INFO_IN_RELEASE=ON" ];
}'Since your changes introduce a pico-lib, could you set a default import path for it within the base packages (like pico-fido)? This would ensure that users preferring the traditional workflow can still use it easily.
CI/CD & Versioning
The automatic logic introduced in the new CI workflow is a fantastic idea. However, since the librekeys repositories are transitioning to "hard forks," we haven't yet finalized how to handle version naming for these upstream-related changes. You are more than welcome to join our discussion on Matrix regarding this.
Future Roadmap & Considerations
I am planning a few upcoming changes that might be affected by this refactor:
- Auto-update support: I plan to add an automatic update workflow (likely using
nix-updateornvfetcher) to regularly update hashes and reduce maintenance burden. You can find examples in thepicoforgerepo. I'll need to test later if these scripts can still locate the required fields after your structural changes. - Cross-compilation (crossSystem): I’m working on shifting the build process to use
crossSystem. This helps Nix automatically handlecmakeFlagslike-DCMAKE_ARinstead of us patching them manually, which caused issues in the past. - ESP32 Support: We are planning to build for the ESP32 platform, so the packaging functions may need further modifications for cross-platform compatibility.
Please note that these points are not mandatory requirements. Not meeting them will not block the merging process. The final decision on merging depends on the collective opinion of the team, and I am currently discussing this with other members.
Additionally, I'd be happy to grant you write permissions to the repository. This will allow you to create branches and push changes directly to the repo.
Thank you again for your valuable contribution! We appreciate your patience while the community makes its final decision.
|
Please wait for librekeys/pico-keys-sdk#3 to complete and update your package. |
You're right, the only viable path to handle this is by not using flake
It was just a kick up the backside regarding this stupid naming scheme (Soon it will be 'pico-fido3' for 'pico-fido-gpg-hsm'...). |
97d1bd9 to
b43d343
Compare
|
@jetcookies does it means, the firmware releases will not have pico-fido, pico-fido2, pico-hsm and pico-openpgp firmwares too? or there will be different releases for each firmware. If later is the case, then it would be better to put links in readme to point to latest releases of each firmware. And put some more info about this repo in readme, so users do not open invalid issues. |
Based on the CI files, pico-fido, pico-fido2, and pico-openpgp will be released under tags starting with their respective names. These releases are independent of each other, each tracking its intended stable version. Adding guidance on where to download the firmware is indeed necessary, but alternative approaches exist. For instance, configuring picoforge to automatically download the required firmware. Or simply splitting this repository and building each firmware in its own dedicated repository. I have no personal preference on this matter; it depends on which implementation other members favor. |
|
Or I was thinking, to split firmware builds into the firmware repos itself. pico-fido will releases the firmware builds for pico-fido, pico-fido2 will release firmware builds for pico-fido2. etc... and pico-fido-firmwares just stores all the nix code( or code for any other build system as proposed by @gxcreator ) that will be used by workflows in each repo to build the firmware. However I doubt how well it would scale in future. Either way, let the people here decide how it should be done and will proceed with that. |
That PR has been merged. You can use the tag v8.5.1-librekeys to get the corresponding commit |
24d5c72 to
b5bf9b2
Compare
0f895be to
61a6830
Compare
|
Please also update .github/workflows/check-flake-packages.yml#L27 to include pico-openpgp and pico-fido2: nix build --no-link --print-build-logs .#pico-fido .#pico-fido2 .#pico-openpgpThis way we can still automatically check whether the compilation passes (for the automatic update script). |
e27a7f0 to
d6efee3
Compare
61a6f73 to
f358eb0
Compare
jetcookies
left a comment
There was a problem hiding this comment.
LGTM. Could you also update .github/workflows/update-flake-packages.yml#L38 by adding the remaining *-firmwares there to prevent the update script from failing?
174a793 to
dced7ed
Compare
Following librekeys/pico-fido2#8
This basically rewrite the entire repo, the purpose is to reuse code for multiple firmware build, while keeping the core logic.
The build (should) work, (I stop it when the first 16 packages of each firmware 'flavor' are built).
I have not tested the workflow.