-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[red-knot] Add support for --system-site-packages
virtual environments
#12759
Conversation
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.
Should this now be moved into the semantic index crate?
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.
at some point, yes
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 exciting. I'll take a closer look tomorrow
Ah dang. The existing tests that I previously added in 7fa76a2 now fail, because we now reject a venv as an invalid venv if the venv's Not sure what to do here... it would be really nice if we could have some way of creating a venv on the fly in the context of our test suite... but the venv would need to depend on some system Python installation, which would mean that our test suite would fail unless you had Python installed... I guess we could just give up on the idea of testing with "real" venvs and create mock venv-like directory structures in the test? But it still feels unclear to me how valuable a test like that would actually be, since we'd really just be repeating the logic from the implementation in the test |
fd049a8
to
18a7eaf
Compare
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 good!
I guess we could just give up on the idea of testing with "real" venvs and create mock venv-like directory structures in the test? But it still feels unclear to me how valuable a test like that would actually be, since we'd really just be repeating the logic from the implementation in the test
I know we touched on this briefly in the previous PR, but I really think this is a non-issue, and having tests that use (accurate) "mock" venvs is very useful, and in fact is precisely equally useful to using "real" venvs that we've committed directly.
Once we commit the files into our repo, it is for all intents and purposes a mock either way, because we don't have any ongoing validation that it continues to match what the venv module (or whatever other venv creator) does, and because the tests aren't running on the real system where the venv was created and can actually function. It's good to base our mocks carefully on real observed venvs, but it's also totally fine to then strip out known-to-be irrelevant parts, change the home path to point to an actual directory, etc. None of that makes the tests any less useful.
If it were me, I'd remove a bunch more unnecessary cruft from the committed mock venvs (like pip
scripts and whatnot that are also non-functional because they refer to non-existent directories.)
I think the home
thing will require (at least partly) switching from pre-committed mocks to written-out-by-the-test mocks, because it should be an absolute path (changing it to a relative path would IMO be too much of a departure from a real venv), and you won't know in advance what absolute path you can write to on some arbitrary system the tests run on.
} | ||
|
||
fn from_executable_home_path(path: &PythonHomePath, system: &dyn System) -> Option<Self> { | ||
let candidate = path.parent()?; |
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.
FWIW I don't think this is true for Windows. I don't have a Windows machine handy to check, but I have a vague memory (from many years ago) that on Windows the Python binary goes directly in the prefix dir of the installation, not inside \Scripts
, which is the Windows equivalent to the bin/
dir.
(This may also explain the lack of clarity in the docs around bin/
dir vs prefix dir; IIRC Vinay, who wrote the docs, uses Windows, where I think there is no distinction to be made.)
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.
This is great.
I mainly left questions around how we handle errors and how to handle relative paths.
I'm not too fond of PythonMinorVersion
. I still think we should just use TargetVersion
@@ -9,7 +9,7 @@ use salsa::plumbing::ZalsaDatabase; | |||
|
|||
use red_knot_server::run_server; | |||
use red_knot_workspace::db::RootDatabase; | |||
use red_knot_workspace::site_packages::site_packages_dirs_of_venv; | |||
use red_knot_workspace::site_packages::VirtualEnvironment; |
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 feels a bit surprising that VirtualEnvironment
is part of the site_packages
module. Shouldn' we rename the site_packages
module to venv
?
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.
That might make @carljm upset, since he keeps pointing out that several of the routines in this module really work with any Python installation, not just virtual environments 😆
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.
Hmm yeah. Not sure. The hierarchy just feels slightly off to me. But we can also revisit this later.
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.
Well, it would depend how you do it :) I was actually going to make a similar comment and decided not to. But I would not want to just rename this module, I would want two modules, one which is specific to venvs and one which contains stuff that works on Python installations in general. But I'm also fine with just keeping it all in one module for now, which is why I didn't make that 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.
Okay, I'll leave this for now; we can come back to it later
c414304
to
a262da6
Compare
… search path if `include-system-site-packages` is specified
bf6ebf5
to
1e0c08b
Compare
1e0c08b
to
d4c933a
Compare
|
I think I've now addressed everything except for https://github.com/astral-sh/ruff/pull/12759/files#r1711881269. Would appreciate it if somebody could take another quick look, though, as it's changed quite a lot today! In particular, the tests have been completely reworked (which was necessary to make them parse): there's no longer any committed virtual environments; we just create small mocks in tests. |
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 good to me!
if cfg!(target_os = "windows") { | ||
Some(Self(path.to_path_buf())) |
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.
Were you able to verify this, or just trusted my vague years-old memory here? 😆
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 verified it on an extremely slow, very old Windows machine I had lying around. I had to install Python on the machine. It took forever -_-
if cfg!(target_os = "windows") { | ||
let system_home_path = system_install_sys_prefix.clone(); | ||
let system_exe_path = system_home_path.join("python.exe"); | ||
let system_site_packages_path = | ||
system_install_sys_prefix.join(r"Lib\site-packages"); | ||
(system_home_path, system_exe_path, system_site_packages_path) |
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.
Related to my question above: I think if it hasn't happened already and doesn't happen before merge, we should track a follow-up task for someone with access to a Windows machine to actually audit this and make sure we are testing for behaviors that actually match what a Windows system / venv looks like.
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.
agreed, these tests really need to be supplemented with manual testing
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.
Manual testing would be great, but if you at least validated that this mock setup matches what you see on a real Windows machine with Python installed on it, that's a great start for now.
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.
Yeah, I did that much. But considering how long installing Python took, there's no way I'm installing the Rust toolchain and doing manual testing of this PR on that machine 😆
let unix_site_packages = if *free_threaded { | ||
format!("lib/python3.{minor_version}t/site-packages") | ||
} else { | ||
format!("lib/python3.{minor_version}/site-packages") | ||
}; |
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 particular reason we calculate this unconditionally out here, rather than in the non-Windows block below?
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 just annoying to have to define such a long variable so many times over and over... and this is a test, so I don't think binary size really matters quite so much here :P
site-packages
as an additional search path if include-system-site-packages = true
is present in a virtual environment's pyvenv.cfg
file--system-site-packages
virtual environments
Summary
Many things about Python virtual environments are not specified, but the following things are:
pyvenv.cfg
file in the venv root.pyvenv.cfg
file is guaranteed to have ahome = <path>
key in it, which will point to the parent directory of the system Python installation that was used to create this venvsite-packages
directory which will be appended tosys.path
at runtime by the stdlibsite
modulepyvenv.cfg
file containsinclude-system-site-packages = true
, (orinclude-system-site-packages = TrUe
, or any other case variations of the word "true"), Python's stdlib module will also append the system Python'ssite-packages
directory as well as the venv's isolatedsite-packages
directory.This PR emulates that runtime behaviour in red-knot.
Test Plan
Several tests have been added to this PR.
The
pyvenv.cfg
files provided by uv, virtualenv and the stdlibvenv
module are all consistent on the above bullets, but differ in many other subtle aspects. As such, I've added two more venvs to theresources/test
directory, in addition to theuv
-created one: one created usingvirtualenv
and one created using the stdlibvenv
module.I haven't added any tests with venvs that actually have
include-system-site-packages = true
in theirpyvenv.cfg
files. I'm not sure how I'd add tests for that without actually committing a complete system Python installation into theresources/test
directory. I did manual testing with--system-site-packages
venvs created using all three tools, however, and everything seemed to be working as expected. (I tried using both apyenv
-installed Python and ahomebrew
-installed Python.)