Skip to content
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

Merged
merged 6 commits into from
Jul 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion crates/install-wheel-rs/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
//! Takes a wheel and installs it into a venv.

use std::io;

use std::path::PathBuf;

use platform_info::PlatformInfoError;
Expand Down
20 changes: 18 additions & 2 deletions crates/install-wheel-rs/src/linker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub struct Locks(Mutex<FxHashMap<PathBuf, Arc<Mutex<()>>>>);
#[instrument(skip_all, fields(wheel = %filename))]
pub fn install_wheel(
layout: &Layout,
relocatable: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

wheel: impl AsRef<Path>,
filename: &WheelFilename,
direct_url: Option<&DirectUrl>,
Expand Down Expand Up @@ -97,8 +98,22 @@ pub fn install_wheel(
debug!(?name, "Writing entrypoints");

fs_err::create_dir_all(&layout.scheme.scripts)?;
write_script_entrypoints(layout, site_packages, &console_scripts, &mut record, false)?;
write_script_entrypoints(layout, site_packages, &gui_scripts, &mut record, true)?;
write_script_entrypoints(
layout,
relocatable,
site_packages,
&console_scripts,
&mut record,
false,
)?;
write_script_entrypoints(
layout,
relocatable,
site_packages,
&gui_scripts,
&mut record,
true,
)?;
}

// 2.a Unpacked archive includes distribution-1.0.dist-info/ and (if there is data) distribution-1.0.data/.
Expand All @@ -108,6 +123,7 @@ pub fn install_wheel(
debug!(?name, "Installing data");
install_data(
layout,
relocatable,
site_packages,
&data_dir,
&name,
Expand Down
91 changes: 77 additions & 14 deletions crates/install-wheel-rs/src/wheel.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::collections::HashMap;
use std::io::{BufRead, BufReader, Cursor, Read, Write};
use std::io::{BufRead, BufReader, Cursor, Read, Seek, Write};
use std::path::{Path, PathBuf};
use std::{env, io};

Expand Down Expand Up @@ -128,7 +128,7 @@ fn copy_and_hash(reader: &mut impl Read, writer: &mut impl Write) -> io::Result<
/// executable.
///
/// See: <https://github.com/pypa/pip/blob/0ad4c94be74cc24874c6feb5bb3c2152c398a18e/src/pip/_vendor/distlib/scripts.py#L136-L165>
fn format_shebang(executable: impl AsRef<Path>, os_name: &str) -> String {
fn format_shebang(executable: impl AsRef<Path>, os_name: &str, relocatable: bool) -> String {
// Convert the executable to a simplified path.
let executable = executable.as_ref().simplified_display().to_string();

Expand All @@ -139,11 +139,18 @@ fn format_shebang(executable: impl AsRef<Path>, os_name: &str) -> String {
let shebang_length = 2 + executable.len() + 1;

// If the shebang is too long, or contains spaces, wrap it in `/bin/sh`.
if shebang_length > 127 || executable.contains(' ') {
// Same applies for relocatable scripts (executable is relative to script dir, hence `dirname` trick)
// (note: the Windows trampoline binaries natively support relative paths to executable)
if shebang_length > 127 || executable.contains(' ') || relocatable {
let prefix = if relocatable {
r#""$(CDPATH= cd -- "$(dirname -- "$0")" && echo "$PWD")"/"#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

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?

Copy link
Contributor Author

@paveldikov paveldikov Jul 29, 2024

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.

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")/""#.

} else {
""
};
// Like Python's `shlex.quote`:
// > Use single quotes, and put single quotes into double quotes
// > The string $'b is then quoted as '$'"'"'b'
let executable = format!("'{}'", executable.replace('\'', r#"'"'"'"#));
let executable = format!("{}'{}'", prefix, executable.replace('\'', r#"'"'"'"#));
return format!("#!/bin/sh\n'''exec' {executable} \"$0\" \"$@\"\n' '''");
}
}
Expand Down Expand Up @@ -272,6 +279,7 @@ fn entrypoint_path(entrypoint: &Script, layout: &Layout) -> PathBuf {
/// Create the wrapper scripts in the bin folder of the venv for launching console scripts.
pub(crate) fn write_script_entrypoints(
layout: &Layout,
relocatable: bool,
site_packages: &Path,
entrypoints: &[Script],
record: &mut Vec<RecordEntry>,
Expand All @@ -293,9 +301,11 @@ pub(crate) fn write_script_entrypoints(

// Generate the launcher script.
let launcher_executable = get_script_executable(&layout.sys_executable, is_gui);
let launcher_executable =
get_relocatable_executable(launcher_executable, layout, relocatable)?;
let launcher_python_script = get_script_launcher(
entrypoint,
&format_shebang(&launcher_executable, &layout.os_name),
&format_shebang(&launcher_executable, &layout.os_name, relocatable),
);

// If necessary, wrap the launcher script in a Windows launcher binary.
Expand Down Expand Up @@ -432,11 +442,12 @@ pub(crate) fn move_folder_recorded(
Ok(())
}

/// Installs a single script (not an entrypoint)
/// Installs a single script (not an entrypoint).
///
/// Has to deal with both binaries files (just move) and scripts (rewrite the shebang if applicable)
/// Has to deal with both binaries files (just move) and scripts (rewrite the shebang if applicable).
fn install_script(
layout: &Layout,
relocatable: bool,
site_packages: &Path,
record: &mut [RecordEntry],
file: &DirEntry,
Expand Down Expand Up @@ -494,7 +505,19 @@ fn install_script(
let mut start = vec![0; placeholder_python.len()];
script.read_exact(&mut start)?;
let size_and_encoded_hash = if start == placeholder_python {
let start = format_shebang(&layout.sys_executable, &layout.os_name)
let is_gui = {
let mut buf = vec![0; 1];
script.read_exact(&mut buf)?;
if buf == b"w" {
true
} else {
script.seek_relative(-1)?;
false
}
};
let executable = get_script_executable(&layout.sys_executable, is_gui);
let executable = get_relocatable_executable(executable, layout, relocatable)?;
let start = format_shebang(&executable, &layout.os_name, relocatable)
.as_bytes()
.to_vec();

Expand Down Expand Up @@ -555,6 +578,7 @@ fn install_script(
#[instrument(skip_all)]
pub(crate) fn install_data(
layout: &Layout,
relocatable: bool,
site_packages: &Path,
data_dir: &Path,
dist_name: &PackageName,
Expand Down Expand Up @@ -598,7 +622,7 @@ pub(crate) fn install_data(
initialized = true;
}

install_script(layout, site_packages, record, &file)?;
install_script(layout, relocatable, site_packages, record, &file)?;
}
}
Some("headers") => {
Expand Down Expand Up @@ -682,6 +706,31 @@ pub(crate) fn extra_dist_info(
Ok(())
}

/// Get the path to the Python executable for the [`Layout`], based on whether the wheel should
/// be relocatable.
///
/// Returns `sys.executable` if the wheel is not relocatable; otherwise, returns a path relative
/// to the scripts directory.
pub(crate) fn get_relocatable_executable(
executable: PathBuf,
layout: &Layout,
relocatable: bool,
) -> Result<PathBuf, Error> {
Ok(if relocatable {
pathdiff::diff_paths(&executable, &layout.scheme.scripts).ok_or_else(|| {
Error::Io(io::Error::new(
io::ErrorKind::Other,
format!(
"Could not find relative path for: {}",
executable.simplified_display()
),
))
})?
} else {
executable
})
}

/// Reads the record file
/// <https://www.python.org/dev/peps/pep-0376/#record>
pub fn read_record_file(record: &mut impl Read) -> Result<Vec<RecordEntry>, Error> {
Expand Down Expand Up @@ -845,33 +894,47 @@ mod test {
// By default, use a simple shebang.
let executable = Path::new("/usr/bin/python3");
let os_name = "posix";
assert_eq!(format_shebang(executable, os_name), "#!/usr/bin/python3");
assert_eq!(
format_shebang(executable, os_name, false),
"#!/usr/bin/python3"
);

// If the path contains spaces, we should use the `exec` trick.
let executable = Path::new("/usr/bin/path to python3");
let os_name = "posix";
assert_eq!(
format_shebang(executable, os_name),
format_shebang(executable, os_name, false),
"#!/bin/sh\n'''exec' '/usr/bin/path to python3' \"$0\" \"$@\"\n' '''"
);

// And if we want a relocatable script, we should use the `exec` trick with `dirname`.
let executable = Path::new("python3");
let os_name = "posix";
assert_eq!(
format_shebang(executable, os_name, true),
"#!/bin/sh\n'''exec' \"$(CDPATH= cd -- \"$(dirname -- \"$0\")\" && echo \"$PWD\")\"/'python3' \"$0\" \"$@\"\n' '''"
);

// Except on Windows...
let executable = Path::new("/usr/bin/path to python3");
let os_name = "nt";
assert_eq!(
format_shebang(executable, os_name),
format_shebang(executable, os_name, false),
"#!/usr/bin/path to python3"
);

// Quotes, however, are ok.
let executable = Path::new("/usr/bin/'python3'");
let os_name = "posix";
assert_eq!(format_shebang(executable, os_name), "#!/usr/bin/'python3'");
assert_eq!(
format_shebang(executable, os_name, false),
"#!/usr/bin/'python3'"
);

// If the path is too long, we should not use the `exec` trick.
let executable = Path::new("/usr/bin/path/to/a/very/long/executable/executable/executable/executable/executable/executable/executable/executable/name/python3");
let os_name = "posix";
assert_eq!(format_shebang(executable, os_name), "#!/bin/sh\n'''exec' '/usr/bin/path/to/a/very/long/executable/executable/executable/executable/executable/executable/executable/executable/name/python3' \"$0\" \"$@\"\n' '''");
assert_eq!(format_shebang(executable, os_name, false), "#!/bin/sh\n'''exec' '/usr/bin/path/to/a/very/long/executable/executable/executable/executable/executable/executable/executable/executable/name/python3' \"$0\" \"$@\"\n' '''");
}

#[test]
Expand Down
1 change: 1 addition & 0 deletions crates/uv-build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ impl SourceBuild {
uv_virtualenv::Prompt::None,
false,
false,
false,
)?,
BuildIsolation::Shared(venv) => venv.clone(),
};
Expand Down
16 changes: 16 additions & 0 deletions crates/uv-cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1743,6 +1743,22 @@ 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 without
/// invalidating its associated entrypoint and activation scripts.
///
/// 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.
///
/// As a result of making the environment relocatable (by way of writing relative, rather than
/// absolute paths), the entrypoints and scripts themselves will _not_ be relocatable. In other
/// words, copying those entrypoints and scripts to a location outside the environment will not
/// work, as they reference paths relative to the environment itself.
#[arg(long)]
pub relocatable: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely worth marking this as a preview/experimental flag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would that be a simple hide = true, or is there a special annotation for that? (couldn't find one)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking using PreviewMode for this in venv_impl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added warning (assuming that's what you meant?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks!


#[command(flatten)]
pub index_args: IndexArgs,

Expand Down
13 changes: 12 additions & 1 deletion crates/uv-installer/src/installer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,16 @@ impl<'a> Installer<'a> {
let (tx, rx) = oneshot::channel();

let layout = venv.interpreter().layout();
let relocatable = venv.relocatable();
rayon::spawn(move || {
let result = install(wheels, layout, installer_name, link_mode, reporter);
let result = install(
wheels,
layout,
installer_name,
link_mode,
reporter,
relocatable,
);
tx.send(result).unwrap();
});

Expand All @@ -112,6 +120,7 @@ impl<'a> Installer<'a> {
self.installer_name,
self.link_mode,
self.reporter,
self.venv.relocatable(),
)
}
}
Expand All @@ -124,11 +133,13 @@ fn install(
installer_name: Option<String>,
link_mode: LinkMode,
reporter: Option<Box<dyn Reporter>>,
relocatable: bool,
) -> Result<Vec<CachedDist>> {
let locks = install_wheel_rs::linker::Locks::default();
wheels.par_iter().try_for_each(|wheel| {
install_wheel_rs::linker::install_wheel(
&layout,
relocatable,
wheel.path(),
wheel.filename(),
wheel
Expand Down
5 changes: 5 additions & 0 deletions crates/uv-python/src/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,11 @@ impl PythonEnvironment {
Ok(PyVenvConfiguration::parse(self.0.root.join("pyvenv.cfg"))?)
}

/// Returns `true` if the environment is "relocatable".
pub fn relocatable(&self) -> bool {
self.cfg().is_ok_and(|cfg| cfg.is_relocatable())
}

/// Returns the location of the Python executable.
pub fn python_executable(&self) -> &Path {
self.0.interpreter.sys_executable()
Expand Down
19 changes: 17 additions & 2 deletions crates/uv-python/src/virtualenv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ pub struct PyVenvConfiguration {
pub(crate) virtualenv: bool,
/// If the uv package was used to create the virtual environment.
pub(crate) uv: bool,
/// Is the virtual environment relocatable?
pub(crate) relocatable: bool,
}

#[derive(Debug, Error)]
Expand Down Expand Up @@ -136,14 +138,15 @@ impl PyVenvConfiguration {
pub fn parse(cfg: impl AsRef<Path>) -> Result<Self, Error> {
let mut virtualenv = false;
let mut uv = false;
let mut relocatable = false;

// Per https://snarky.ca/how-virtual-environments-work/, the `pyvenv.cfg` file is not a
// valid INI file, and is instead expected to be parsed by partitioning each line on the
// first equals sign.
let content = fs::read_to_string(&cfg)
.map_err(|err| Error::ParsePyVenvCfg(cfg.as_ref().to_path_buf(), err))?;
for line in content.lines() {
let Some((key, _value)) = line.split_once('=') else {
let Some((key, value)) = line.split_once('=') else {
continue;
};
match key.trim() {
Expand All @@ -153,11 +156,18 @@ impl PyVenvConfiguration {
"uv" => {
uv = true;
}
"relocatable" => {
relocatable = value.trim().to_lowercase() == "true";
}
_ => {}
}
}

Ok(Self { virtualenv, uv })
Ok(Self {
virtualenv,
uv,
relocatable,
})
}

/// Returns true if the virtual environment was created with the `virtualenv` package.
Expand All @@ -169,4 +179,9 @@ impl PyVenvConfiguration {
pub fn is_uv(&self) -> bool {
self.uv
}

/// Returns true if the virtual environment is relocatable.
pub fn is_relocatable(&self) -> bool {
self.relocatable
}
}
1 change: 1 addition & 0 deletions crates/uv-tool/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ impl InstalledTools {
uv_virtualenv::Prompt::None,
false,
false,
false,
)?;

Ok(venv)
Expand Down
Loading
Loading