-
-
Notifications
You must be signed in to change notification settings - Fork 467
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
Remove pip-tools, use uv for all operations #1226
Conversation
rye/src/bootstrap.rs
Outdated
@@ -55,6 +54,8 @@ virtualenv==20.25.0 | |||
ruff==0.4.4 | |||
"#; | |||
|
|||
const LATEST_PIP: &str = "pip==23.3.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.
We can drop it in another 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 may have been mistaken but I thought we needed pip in the uv venv for tools like maturin
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 other concerns @T-256 ?
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 may have been mistaken but I thought we needed pip in the uv venv for tools like maturin
I don't think so.
uv is also drop-in replacement for pip, so I think we can use it instead.
I didn't do full review since a team member should do and approve this PR. I just recommended what could done in next PR.
Regard, @nazq .
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.
Seems you're correct. Looks like maturin added this support with this PR a couple of months back. PyO3/maturin#2015
@@ -111,17 +109,6 @@ fn register(cmd: RegisterCommand) -> Result<(), Error> { | |||
|
|||
/// Checks if a toolchain is still in use. | |||
fn check_in_use(ver: &PythonVersion) -> Result<(), Error> { | |||
// Check if used by rye itself. | |||
let app_dir = get_app_dir(); | |||
for venv in &[app_dir.join("self"), get_pip_tools_venv_path(ver)] { |
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 don't think this whole loop should be removed, only the pip tools element in the array?
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.
0fa01c4
to
e2d856a
Compare
Rebased main, and squashed out my previous commits |
I think I'll take a stab at the docs too now and then move this out of draft |
Remove pip even from uv venvs Add back rye/self toolchain in use check Add docs updates
Docs updates added |
- **No Exposed Pip:** pip is intentionally not exposed. If you were to install something | ||
into the virtualenv, it disappears next time you sync. If you symlink `rye` to | ||
`~/.rye/shims/pip` you can get access to pip without installing it into the | ||
virtualenv. There be dragons. |
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 think some part of this should remain in docs.
It should still say that pip is not exposed in Rye's virtualenvs maybe? Since that's a difference to what python -m venv does, i.e "normal" virtual environments have pip, rye's virtual environments do not.
The symlink to pip thing can probably be removed.
docs/guide/sync.md
Outdated
[pip-tools](https://github.com/jazzband/pip-tools). It currently defaults to | ||
`uv` as it offers significantly better performance, but will offer you the | ||
option to use `pip-tools` instead. | ||
Rye supports only supports [`uv`](https://github.com/astral-sh/uv) to manage dependencies. |
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.
"supports only supports" typo
Sorry for the delay on this one. I'll take a look soon. |
Haven't updated the docs just yet but wanted to get some folks to look at the code changes first.