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
nrfutil: 5.2 -> 6.1, pc-ble-driver-py: 0.11.4 -> 0.14.2, py-ble-driver: init at 4.1.1 #93063
Conversation
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.
|
||
buildPythonPackage rec { | ||
with python37Packages; buildPythonPackage 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.
with python37Packages; buildPythonPackage rec { | |
buildPythonPackage rec { |
Python packages can be built for different python versions. For example nix-build -A python38Packages.pc-ble-driver-py
. This file should not import python37Packages
.
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. Same for nrfutil? It doesn't build on all Python 3 versions, and not at all on Python 2.
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.
For applications it's fine to hardcode a python version. It's only the libraries where you might want to use them with a different python version.
Please squash the "Don't depend on a fixed Python version" commit into the "nrfutil: ..." commit.
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.
Ok, done! (I'm assuming you meant the pc-ble-driver-py commit)
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.
Oh yes, of course.
It's a dependency of pc-ble-driver-py, but it's no longer built alongside it.
Thank you! |
Motivation for this change
Fix the build of nrfutil by updating it and its dependencies.
cc @gebner
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)