-
Notifications
You must be signed in to change notification settings - Fork 605
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
feat(venv): add relocatable flag #5515
Changes from 2 commits
fbbe0d9
9eb8f8b
a9b9bd3
5a38a6c
5508f23
19cef6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1743,6 +1743,17 @@ pub struct VenvArgs { | |
#[arg(long)] | ||
pub system_site_packages: bool, | ||
|
||
/// Make the virtual environment relocatable. | ||
/// | ||
/// A relocatable virtual environment can be moved around and redistributed with its | ||
/// associated entrypoint and activation scripts functioning as usual. | ||
/// | ||
/// Note that this can only be guaranteed for standard `console_scripts` and `gui_scripts`. | ||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likely worth marking this as a preview/experimental flag There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would that be a simple There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added warning (assuming that's what you meant?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, thanks! |
||
|
||
#[command(flatten)] | ||
pub index_args: IndexArgs, | ||
|
||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's exactly it,
readlink -f
andrealpath
are a real POSIX-feature-availability-matrix nightmare, so that's what theCDPATH= cd -- SOME_PATH && echo "$PWD"
part is -- a cross-platform way of doingrealpath 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
;^)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 aset -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")/""#
.