-
Notifications
You must be signed in to change notification settings - Fork 37
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
Migrate from poetry & tox to uv #950
Open
ItsDrike
wants to merge
12
commits into
master
Choose a base branch
from
move-to-uv
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ItsDrike
force-pushed
the
move-to-uv
branch
2 times, most recently
from
January 17, 2025 01:30
72c73d2
to
15c8b5a
Compare
ItsDrike
added
area: dependencies
Related to package dependencies
area: CI
Related to continuous intergration and deployment
type: rewrite
Complete or partial rewrite of a part of the codebase
labels
Jan 17, 2025
Open
PerchunPak
reviewed
Jan 18, 2025
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.
Great PR, thanks! Just a few nits (mostly)
Co-authored-by: Kevin Tindall <[email protected]>
PerchunPak
approved these changes
Jan 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area: CI
Related to continuous intergration and deployment
area: dependencies
Related to package dependencies
type: rewrite
Complete or partial rewrite of a part of the codebase
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes our primary dependency management tool from
poetry
touv
.Alongside, it also drops
tox
, asuv
doesn't have the equivalent of apoetry-tox
dependency, and I didn't really feel like bothering with getting it to work, considering there were plans to remove it anyways (#716). P.S. Sorry for taking this over Kevin, but the PR was stale for a long time now, so hope you don't mind, I did mark you as a co-author on one of the commits, as I pretty much just copied your work on the docs adjustments & poe tasks.I've already mentioned some of the reasons why
uv
is likely going to be a better choice for us in the associated issue: #925.Closes: #925, #488
Superseds & closes: #716
Some notes:
poetry shell
, so venv activation should be done in the standard way, as it would be with thevenv
module alone (source .venv/bin/activate
).v
). We can do this both from the GitHub UI or the CLI. After this is merged, it will also be necessary to change and maintain theversion
value in thepyproject.toml
. Even though I did find an equivalent topoetry-dynamic-versioning
, which was doing this for us automatically before:uv-dynamic-versioning
, looking at the GitHub stats of this project, it's not very popular, and I'd rather not introduce such a dependency. I did however add a check in the CI that ensures the version in pyproject.toml was in fact bumped before attempting a release (The release would fail anyways as PyPI would refuse it, but this way, it's at least cleaner what happened)pyproject.toml
file. This is handy when bumping the project version, as it's easy to forget about runninguv lock
afterwards, to update project version there too.uv sync
command handles groups, as there's no easy way to say "exclude all groups and only install these ones". Well, there is:--only-group <ONLY_GROUP>
, however, it only works with a single group and using it only installs the dependencies from this group, and I do mean only, it doesn't even install the project & it's dependenceis. From what I read in the docs &uv sync --help
, I didn't see any way to use this flag & install the project dependencies too. This means that we could either make all of the groups optional, and move the burden up to the contributors installing the project to include the--all-groups
or--group <GROUP>
for each needed group, or do what I did here, where all the groups are included by default and the CI workflows then exclude those that aren't needed. I will admit that this feels a bit awkward in comparison to poetry there and if we add more dependency groups, it would require updating the commands to exclude those (if desired). I'm fine with making the groups optional instead, if you think that'd be better.minecraft
,protocol
keywords. (This will show up on PyPI)docsutils
dependency to<0.21
, as later versions break them2r2
parser. See: docutils 0.21 has removed nodes.reprunicode() and therefore building documentation using m2r2 fails CrossNox/m2r2#68. I've just glanced over this, but it seems like m2r2 might be unmaintained, someone in that thread did mention an alternative: myst-parser. It might be worth it to look into this a bit more.