-
Notifications
You must be signed in to change notification settings - Fork 76
Add python devShell to nix flake #864
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
base: master
Are you sure you want to change the base?
Add python devShell to nix flake #864
Conversation
Pull Request Test Coverage Report for Build 17097642875Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| export LD_LIBRARY_PATH=${pkgs.lib.makeLibraryPath [pkgs.openssl]}:$LD_LIBRARY_PATH | ||
| cd payjoin-ffi/python | ||
| # Create and activate virtual environment | ||
| python -m venv venv | ||
| source venv/bin/activate | ||
| # Install dependencies, allowing PyPI fetches for version mismatches | ||
| pip install --requirement requirements.txt --requirement requirements-dev.txt | ||
| # Generate bindings, setting PYBIN to the venv's python binary | ||
| export PYBIN=./venv/bin/python | ||
| bash ./scripts/generate_${platform}.sh | ||
| # Set CARGO_TOML_PATH for setup.py | ||
| export CARGO_TOML_PATH=${./.}/Cargo.toml | ||
| # Build the wheel | ||
| python setup.py bdist_wheel --verbose | ||
| # Install payjoin | ||
| pip install ./dist/payjoin-*.whl |
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 chance we can use uv and uv2nix instead of pip?
requirements.txt is not reproducible, and poetry2nix is kinda dead these days
doing this in a shell hook also means we can't really use this in a flake check in a clean way, so while this is convenient it seems like it would be pretty brittle in the long term and if people run into problems we would have a hard time debugging without a lot of manual intervention
especially because venv is likely to survive long term and accumulate cruft unless the user deliberately cleans and reruns the shell hook (ideally the built artifacts should end up in the nix store)
| pip install --requirement requirements.txt --requirement requirements-dev.txt | ||
| # Generate bindings, setting PYBIN to the venv's python binary | ||
| export PYBIN=./venv/bin/python | ||
| bash ./scripts/generate_${platform}.sh |
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.
wouldn't using ${system} potentially be more future proof? they can still be a single file using symlinks but if there are any platform specific quirks down the line i suspect the net result would be adding another layer on top of this, instead of going the more direct route
Adds a top-level python devShell to the nix flake which creates a python virtual environment with all necessary dependencies installed. Now, assuming Docker is running and nix is installed on the system, a developer can simply run `nix develop .#python` and have a shell with all the requirements to run `python -m unittest --verbose` setup, or import the `payjoin` package (locally built) to their project.
55b7e8b to
a830625
Compare
|
@thebrandonlucas are you waiting on review for this? Is something else blocking? |
@arminsabouri I was away from this for awhile but am now actively working on it addressing @nothingmuch's feedback (trying to add the flake check, refactor the That said, I don't think we lose anything by adding this as-is considering this just incorporates the current build instructions for python ffi into a nix flake. So this would probably prove immediately useful and was working last I checked, despite @nothingmuch's improvement suggestions which definitely should be addressed at some point IMO. |
|
@thebrandonlucas thanks for the quick response. Makes sense. |
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.
cACK - I agree @nothingmuch's comments can be addressed in follow-ups, and also fyi the generate scripts are getting overhauled in #980
|
Because this needs a rebase, I don't think a little delay to get this to be actually reproducible is the worst thing in the world. Makes sense to figure out how to do it nixpilled. |
|
From @nothingmuch
|
|
see #997 for some WIP to do this, we (@thebrandonlucas and I) haven't done the uv2nix part of it yet, just uv instead of pip, i think the most logical thing would be for that PR to only be the uv instead of pip change and this PR can get rebased on top of that |
|
at Dan's request, the reproduceability rationale for uv and uv2nix over pip:
|
uv was merged. I think this PR needs a rebase and another review. Not an urgent priority. |
Adds a top-level python devShell to the nix flake which creates a shell with a python virtual environment with all necessary dependencies installed.
Now, assuming Docker is running and nix is installed on the system, a developer can simply run
nix develop .#pythonand have a shell with all the requirements to runpython -m unittest --verbosesetup, or import thepayjoinpackage (locally built) to their project.Addresses this issue referenced in the nix flake tracking issue.
Also updates the README accordingly, de-duplicates some instructions, and removes some crufty language