Skip to content
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

Update pep508_rs and pep440_rs #928

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

bluss
Copy link
Contributor

@bluss bluss commented Mar 23, 2024

Update pep508_rs and pep440_rs crates

Main reason is to use VerbatimUrl from pep508_rs and try to fix issues around file:///${PROJECT_ROOT}/etc dependencies.

Note that monotrail-utils is still using the older pep508_rs.

Fixes #912

@bluss bluss marked this pull request as draft March 23, 2024 12:40
@bluss
Copy link
Contributor Author

bluss commented Mar 23, 2024

This is in an attempt to solve #912. In this case by using the real absolute path as the Url part of VerbatimUrl and the ${PROJECT_ROOT}/../relative path as the "given" part of the URL, let's see how that works out.

Remains to be done

@bluss bluss force-pushed the update-pep508 branch 5 times, most recently from 33aa30d to 81ac63f Compare March 23, 2024 13:13
rye/src/cli/init.rs Outdated Show resolved Hide resolved
@bluss bluss force-pushed the update-pep508 branch 2 times, most recently from 69a8d4a to 1d89d54 Compare March 24, 2024 12:33
@bluss bluss marked this pull request as ready for review March 24, 2024 12:37
@bluss
Copy link
Contributor Author

bluss commented Mar 24, 2024

Now it's working, but it certainly does not feel solid around VerbatimUrl and ${}'s.

@bluss bluss force-pushed the update-pep508 branch 6 times, most recently from 2ce0036 to fc91d87 Compare March 24, 2024 13:10
@bluss bluss mentioned this pull request Mar 25, 2024
@bluss
Copy link
Contributor Author

bluss commented Mar 29, 2024

rebased x3

@bluss
Copy link
Contributor Author

bluss commented May 6, 2024

Feedback on this is welcome. Updating these crates is ok? Fixing relative paths is ok or a bad idea? We could go other ways, like adopting another convention for relative paths than the ${PROJECT_ROOT} stuff here, for example going all in on uv-style relative paths?

@charliermarsh charliermarsh requested review from mitsuhiko and charliermarsh and removed request for mitsuhiko May 20, 2024 19:46
@charliermarsh
Copy link
Member

Can you explain why the original failure is happening? I.e., why is PROJECT_ROOT problematic in a virtual project?

@bluss
Copy link
Contributor Author

bluss commented May 21, 2024

The bug is just general to rye add with relative paths.

The default rye init project is not virtual and build system is hatchling. In this configuration, currently, Rye does not support relative paths with git add. It just makes them absolute. So, no bug with the default project because there are no relative paths.

This bug shows up in for example rye init --virtual and rye init --build-system pdm projects where rye add thinks it supports relative path dependencies (the conditional is build_system != hatchling), and fails to construct the URL for it.

So we could say that ${PROJECT_ROOT} is always problematic for rye add currently - it seems to work in rye sync though.

This PR is my attempt to fix this in a "minimal" way. It's not great that the PR mixes two things - updating deps and fixing bugs in add, but when I wrote this I didn't want to leave stuff in a half broken state. (I even had the energy to write tests. They are not there because I'm a nice guy, it's because I want this functionality to keep working 😉 )

@bluss
Copy link
Contributor Author

bluss commented May 21, 2024

There's a lot more we'd like to do with relative paths, but I need some buy in from @mitsuhiko I feel. It would be great to adopt some ideas from uv where you have done the hard work of finding out what's interoperable with pip and what is not - see PR astral-sh/uv#3682

bluss added 5 commits May 21, 2024 20:25
Update pep508_rs and pep440_rs crates

Main reason is to use VerbatimUrl from pep508_rs.

Note that monotrail-utils is still using the older pep508_rs.
For test uniformity, set author name in test config.
@bluss
Copy link
Contributor Author

bluss commented May 21, 2024

rebased and dropped the changelog update because the newest changelog seemed to be autogenerated

This is to explicitly use / for path separators.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't add relative path dependencies in virtual project
2 participants