feat(venv): add relocatable flag#5515
Conversation
| /// Other scripts may be adjusted if they ship with a generic `#!python[w]` shebang, | ||
| /// and binaries are left as-is. | ||
| #[arg(long)] | ||
| pub relocatable: bool, |
There was a problem hiding this comment.
Likely worth marking this as a preview/experimental flag
There was a problem hiding this comment.
would that be a simple hide = true, or is there a special annotation for that? (couldn't find one)
There was a problem hiding this comment.
I was thinking using PreviewMode for this in venv_impl
There was a problem hiding this comment.
Added warning (assuming that's what you meant?)
|
This has a little bit of overlap with #5509 which just does the installer piece and isn't exposed to users. |
|
One potential issue here is that users can't use |
Yes, I see that! Feel free to cherry-pick pieces out of this as you see fit (and vice versa -- let me know if I am straying too far away from the direction you had in mind) |
I thought |
crates/install-wheel-rs/src/wheel.rs
Outdated
| fn format_shebang(executable: impl AsRef<Path>, os_name: &str) -> String { | ||
| // Convert the executable to a simplified path. | ||
| let executable = executable.as_ref().simplified_display().to_string(); | ||
| let is_relocatable = !executable.contains(['\\', '/']); |
There was a problem hiding this comment.
Does this assume that the relative path doesn't contain multiple segments? (That's true for virtual environments, but could be false in general, right?)
There was a problem hiding this comment.
On second thought, this check is probably unnecessary and I might just pass the already-known is_relocatable as an argument, rather than trying to reverse-engineer it.
Sounds good! Ultimately pretty similar, I'll likely merge yours first. |
|
We might want to add some tests by wiring this up through |
I'll try to dig a little deeper tonight, but if you have examples of similar tests handy, that would be grand! |
|
@paveldikov -- A basic test would be like |
|
You can run |
Adds a `--relocatable` CLI arg to `uv venv`. This flag does two things: * ensures that the associated activation scripts do not rely on a hardcoded absolute path to the virtual environment (to the extent possible; `.csh` and `.nu` left as-is) * persists a `relocatable` flag in `pyvenv.cfg`. The flag in `pyvenv.cfg` in turn instructs the wheel `Installer` to create script entrypoints in a relocatable way (use `exec` trick + `dirname $0` on POSIX; use relative path to `python[w].exe` on Windows). Fixes: astral-sh#3863
|
Added a test case for the boilerplate on the edit: woops, saw that you'd asked for
Going to add those in. Does any of the already-existing stub packages in |
|
Added coverage for
Unless I'm reading this wrong, running a console script also implies importing the package, so I didn't handle that separately. |
|
Thank you! Haven't reviewed yet but per your last question, I believe |
Yep, I did find it and used it specifically for its Hello World :-P |
|
Cool, this generally looks good to me! I'm going to make a few small tweaks to make it a bit easier to rebase #5509, but not anticipating any major changes. Thank you for the tests too. |
| // (note: the Windows trampoline binaries natively support relative paths to executable) | ||
| if shebang_length > 127 || executable.contains(' ') || is_relocatable { | ||
| let prefix = if is_relocatable { | ||
| r#""$(CDPATH= cd -- "$(dirname -- "$0")" && echo "$PWD")"/"# |
There was a problem hiding this comment.
What's the benefit of this over just dirname $0 alone as in https://github.com/astral-sh/uv/pull/5509/files#diff-c1686969b46b2c133e184a8c25069ada51363d91354e35fac208bb67239fcf2cR183? (I have no idea!)
There was a problem hiding this comment.
Seems like there's a check if the directory exists (avoiding readlink/realpath friends) before substituting it with PWD which makes this a bit more cross-posix-platform and handle a couple error scenarios. Interestingly enough, I'd expect the / to be inside the echo though if this was the intention? Unless it's expected if cd fails that the output is "/" instead of "". Thoughts?
There was a problem hiding this comment.
Seems like there's a check if the directory exists (avoiding readlink/realpath friends) before substituting it with PWD which makes this a bit more cross-posix-platform and handle a couple error scenarios.
Yes, that's exactly it, readlink -f and realpath are a real POSIX-feature-availability-matrix nightmare, so that's what the CDPATH= cd -- SOME_PATH && echo "$PWD" part is -- a cross-platform way of doing realpath SOME_PATH.
The -- bits in the middle are there to ensure that $0 is parsed as a positional argument (and never as an option), just in case decided to call their console script -myscript ;^)
Interestingly enough, I'd expect the / to be inside the echo though if this was the intention? Unless it's expected if cd fails that the output is "/" instead of "". Thoughts?
There was no intention behind the slash's positioning, to be honest -- if the echo fails we are in undefined territory -- I think I really just placed it according to my aesthetic whims! Perhaps a set -eo pipefail could further harden this boilerplate to ensure that it fails fast? (need to check if this is portable, though)
I guess one might argue that resolving the path down to absolute is not needed in the first place! I guess that would strip much of the complexity off, leaving you with r#""$(dirname -- "$0")/""#.
2c80308 to
19cef6c
Compare
| #[instrument(skip_all, fields(wheel = %filename))] | ||
| pub fn install_wheel( | ||
| layout: &Layout, | ||
| relocatable: bool, |
There was a problem hiding this comment.
I moved these next to each other because part of me wishes that it were on Layout (though I don't think creating layout should require querying the filesystem, so leaving them separate).
There was a problem hiding this comment.
I did originally put this inside of Layout, but for some reason decided to back out of that.
(though I don't think creating layout should require querying the filesystem, so leaving them separate).
Could use the layout.as_relocatable() pattern? (only mark it as relocatable if specifically requested by the caller)
|
Thanks! |
| @REM WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
|
|
||
| @set "VIRTUAL_ENV={{ VIRTUAL_ENV_DIR }}" | ||
| @for %%i in ("{{ VIRTUAL_ENV_DIR }}") do @set "VIRTUAL_ENV=%%~fi" |
There was a problem hiding this comment.
This seems to be for expanding a relative path in command prompt since the previous command didn't, awesome!
|
Please excuse my ignorance… does this mean that portable virtual environments are possible now? |
It depends -- a lot of people, myself included, would be wary of using the word 'portable'. (it could imply factors such as host OS or system architecture, in which case: no, these environments are defintely not portable.) But on a high level, yes, assuming a homogenised host environment, these virtual environments can be shipped around from one host to another with most (if not all) functionality being retained. (also, this is a preview feature, so I imagine it is probably premature to talk of it as a done deal. Matter of fact, I just found a bug with the activate script on |
Summary
Adds a
--relocatableCLI arg touv venv. This flag does two things:absolute path to the virtual environment (to the extent possible;
.cshand.nuleft as-is)relocatableflag inpyvenv.cfg.The flag in
pyvenv.cfgin turn instructs the wheelInstallerto create scriptentrypoints in a relocatable way (use
exectrick +dirname $0on POSIX;use relative path to
python[w].exeon Windows).Fixes: #3863
Test Plan
venv.uv venvwith and without--relocatable