-
Notifications
You must be signed in to change notification settings - Fork 81
Use uv to manage Python dependencies #255
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
Conversation
Hexxeh
left a comment
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.
I'm fine with this, let's get @gmarull to weigh in from Core side though.
45bfb8b to
69b0d73
Compare
|
ping @gmarull now that he's back in the office :) |
|
Not sure what's up with that build failure, can't reproduce it locally - maybe try rerun it, if it's still broken after that I'll dig into it further. |
|
I'm not really a fan of fancy new Python package managers unless they become the official package manager. Who knows if |
|
I had similar thoughts, but I think the things that convinced me were:
|
|
I kicked it to rerun it. The new SDK requires |
docs/Makefile
Outdated
| # from the environment for the first two. | ||
| SPHINXOPTS ?= | ||
| SPHINXBUILD ?= sphinx-build | ||
| SPHINXBUILD ?= uv run sphinx-build |
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.
I prefer not to have this managed.
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.
Same reason as waf
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env python3 | |||
| #!/usr/bin/env -S uv run --script | |||
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.
same here, this creates a hidden development flow dependency on uv, which I'd avoid. If we use uv, let's make it transparent, explicit and optional.
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.
It's not really a hidden dependency, I've documented the requirement in the getting-started docs. I can drop this, but it means you'd have to prefix every Python command with uv run (worse for the docs - more like SPHINXBUILD="uv run sphinx-build" make which is very unintuitive). Feels like putting a fair bit of overhead on developers, which is part of what I was trying to reduce with this PR.
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.
the opposite version of this is: if you don't want to use uv, you can still python3 waf build
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.
By hidden dependency I mean waf now depends on uv to run, you have no other choice. Considering uv is not the official Python package manager solution, I'd avoid this. We should be able to still use pip.
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.
I'd agree it's hidden in a sense, but as @jwise pointed out you do have another choice.
This is also to my mind the biggest benefit of uv for me, so I'd prefer to keep it as it is currently in this PR.
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.
I'm not sold this is an advantage. If activating the venv is an issue, there are quite a few shell scripts/plugins etc that will automatically load venvs if found when entering a directory, and then, regardless of who created such venv or which tool, commands will work as usual.
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.
It doesn't - as Joshua mentioned, you can still use python3 waf and it will be run with whatever Python is currently in PATH, without interacting with uv at all.
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.
I know, but that's precisely what I do not want. This forces a different workflow if I do not stick with uv.
| "svg-path>=7.0", | ||
| ] | ||
|
|
||
| [tool.uv.sources] |
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.
isn't there any standard way to define this? it worked pre-uv...
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.
Looks like there is. I'll do that instead.
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.
Actually, nope, I've run into astral-sh/uv#3271 - doesn't work if you run uv outside of the repo root (eg. in docs/). Will probably have to put that one back, unless there's any better way to do it.
3c09c68 to
d783c65
Compare
|
I'm going to look into that build failure. nanopb/nanopb#599 seems relevant, some sort of race condition probably? |
15a32fb to
ada73a9
Compare
|
Think I squashed it. Solution feels a little hacky, so I'm open to better suggestions. |
gmarull
left a comment
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.
My only objection now is to not force uv, ie, make it part of official GSG, but keep the option to use pip if someone prefers to (of course it'll become 'officially unsupported', same as other toolchains than the ARM GNU). I think it's a more future-proof solution.
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env python3 | |||
| #!/usr/bin/env -S uv run --script | |||
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.
By hidden dependency I mean waf now depends on uv to run, you have no other choice. Considering uv is not the official Python package manager solution, I'd avoid this. We should be able to still use pip.
docs/Makefile
Outdated
| # from the environment for the first two. | ||
| SPHINXOPTS ?= | ||
| SPHINXBUILD ?= sphinx-build | ||
| SPHINXBUILD ?= uv run sphinx-build |
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.
Same reason as waf
Dependencies imported from requirements.txt with `uv add -r requirements.txt`. Signed-off-by: Ruby Iris Juric <[email protected]>
Signed-off-by: Ruby Iris Juric <[email protected]>
Signed-off-by: Ruby Iris Juric <[email protected]>
Signed-off-by: Ruby Iris Juric <[email protected]>
Imported with `uv add -r docs/requirements.txt`. Signed-off-by: Ruby Iris Juric <[email protected]>
The packages weren't being installed into uv's venv, since they had no build system defined. The declare_namespace calls are also deprecated in recent setuptools, and are no longer necessary. Signed-off-by: Ruby Iris Juric <[email protected]>
Signed-off-by: Ruby Iris Juric <[email protected]>
nanopb_generator will attempt to use protoc to generate a Python file as part of it's operation, if that file doesn't exist yet. It seems like when waf runs those generations in parallel, there's a race to generate that file that results in build failures when the race is lost. To resolve this, we call the function that generates that file during configure so we avoid the parallel race during build. Signed-off-by: Ruby Iris Juric <[email protected]>
|
So my last take on this: happy to use |
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note, that you can always re-open a closed pull request at any time. |
Using plain pip+venvs to manage dependencies for build tooling has a few downsides:
source .venv/bin/activateon each new terminal session, and possiblydeactivateas well when they're donerequirements.txtonly specifies ranges of versions, which can potentially cause breakages due to dependency updates without warningWhile the second can be solved with just pip, the first has no easy solution. Instead, to tackle both issues, we can use uv, a new Python package manager which tackles these problems.
This PR creates a
pyproject.tomlfile with dependencies imported from bothrequirements.txtanddocs/requirements.txt, with the intention of using uv (or possibly some other tool later down the line, if uv stops being the new hotness for whatever reason) to install them. It also sets up bothwafand the docs site build to useuv runfor execution, automatically running them within uv's managed venv and saving developers from having to manually activate the venv themselves.The docs have been updated to include uv setup instructions for both Ubuntu and macOS, and remove the venv setup step (since uv handles this automatically).
A few changes have also been made to the
pyproject.tomlfiles inpython_libs, to ensureuvbuilds and installs the libraries inside it's venv correctly. The__init__.pyfiles have been removed, as thedeclare_namespacecalls were emitting deprecation warnings with recent versions of setuptools, and are obsoleted by PEP 420.