-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add venv
-based build isolation behind a flag
#11619
base: main
Are you sure you want to change the base?
Conversation
This will make it easier to organise the additional complexity of having multiple build environment mechanisms.
This separates the various concerns internally, while still exposing the same API for the sub-package.
if sys.platform == "win32": | ||
libpath = os.path.join( | ||
self._temp_dir.path, | ||
"Lib", | ||
"site-packages", | ||
) | ||
else: | ||
libpath = os.path.join( | ||
self._temp_dir.path, | ||
"lib", | ||
f"python{sys.version_info.major}.{sys.version_info.minor}", | ||
"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.
Maybe we can use sysconfig here instead? New versions have a venv
key for this specific use case, and we can fall back to the prefix scheme otherwise (or maybe limit this to 3.11+).
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.
TBH, I'd like to switch over to this in a regular/slightly-long deprecation cycle to let us kill the old isolation logic and apply pressure on redistributors/users to unbreak their Python distributions. But, I digress.
Regarding sysconfig/venv scheme: yea, I think we could but this is matching the current implementation realities across all versions, and isn't particularly complex or difficult to maintain. It's only feasible for this to change in a new Python version which we will have the ability to adapt to. :)
This method is not used or called by anything.
This makes it possible to reuse this logic in places other than the existing custom `BuildEnvironment` class.
This isn't particularly useful since the `_install_requirements` method is no longer as complicated.
This enables using a virtual environment directly for handling the creation of the build environment.
This makes room for a base / protocol for the build environment interface and for alternative implementations.
This moves shared logic into the base class for all build environments, providing both runtime protections as well as type checker protections.
ed515b6
to
9c49e9e
Compare
This makes it possible for this to have multiple states while still providing some level of protection via type-checking.
9c49e9e
to
f202324
Compare
Do what you feel is best, GitHub commits is a meaningless metric, OSS work doubly so 🤷 |
This is still very much WIP, but it's been something I've been chipping away at for the past few weeks on a regular basis. I'm filing this eagerly, on the basis that "it can't hurt to file early".
I've incorporated some of the choices made in #11602 as a part of this; but that's triggered a bit of a rework of the existing changes I had.
Closes #7778