From d243250decad4e76bd57840242ca92cf4ddeb27e Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 27 Jan 2024 19:11:55 -0800 Subject: [PATCH] Avoid unnecessary permissions changes for copy paths (#1152) In Rust, `fs::copy` automatically preserves permissions (see: https://doc.rust-lang.org/std/fs/fn.copy.html). Elsewhere, when copying from the zip archive out to the cache, we can set permissions during file creation, rather than as a separate call. Both of these should be slightly more efficient. --- crates/install-wheel-rs/src/linker.rs | 15 +--- crates/puffin-extract/src/lib.rs | 18 ++-- crates/puffin/tests/pip_install.rs | 120 +++++++++++++++++++++++--- requirements.in | 1 - 4 files changed, 120 insertions(+), 34 deletions(-) delete mode 100644 requirements.in diff --git a/crates/install-wheel-rs/src/linker.rs b/crates/install-wheel-rs/src/linker.rs index 51abc8b48..78f9b269a 100644 --- a/crates/install-wheel-rs/src/linker.rs +++ b/crates/install-wheel-rs/src/linker.rs @@ -353,22 +353,9 @@ fn copy_wheel_files( continue; } - // Copy the file. + // Copy the file, which will also set its permissions. fs::copy(path, &out_path)?; - #[cfg(unix)] - { - use std::fs::Permissions; - use std::os::unix::fs::PermissionsExt; - - if let Ok(metadata) = entry.metadata() { - fs::set_permissions( - &out_path, - Permissions::from_mode(metadata.permissions().mode()), - )?; - } - } - count += 1; } diff --git a/crates/puffin-extract/src/lib.rs b/crates/puffin-extract/src/lib.rs index 518369787..24c0a5178 100644 --- a/crates/puffin-extract/src/lib.rs +++ b/crates/puffin-extract/src/lib.rs @@ -1,3 +1,4 @@ +use std::fs::OpenOptions; use std::path::{Path, PathBuf}; use rayon::prelude::*; @@ -125,21 +126,24 @@ pub fn unzip_archive( fs_err::create_dir_all(parent)?; } - // Write the file. - let mut outfile = fs_err::File::create(&path)?; - std::io::copy(&mut file, &mut outfile)?; + // Create the file, with the correct permissions (on Unix). + let mut options = OpenOptions::new(); + options.write(true); + options.create_new(true); - // Set permissions. #[cfg(unix)] { - use std::fs::Permissions; - use std::os::unix::fs::PermissionsExt; + use std::os::unix::fs::OpenOptionsExt; if let Some(mode) = file.unix_mode() { - std::fs::set_permissions(&path, Permissions::from_mode(mode))?; + options.mode(mode); } } + // Copy the file contents. + let mut outfile = options.open(&path)?; + std::io::copy(&mut file, &mut outfile)?; + Ok(()) }) .collect::>() diff --git a/crates/puffin/tests/pip_install.rs b/crates/puffin/tests/pip_install.rs index 811f7689f..c0aab3f8e 100644 --- a/crates/puffin/tests/pip_install.rs +++ b/crates/puffin/tests/pip_install.rs @@ -962,6 +962,8 @@ fn reinstall_no_binary() -> Result<()> { /// Install a package into a virtual environment, and ensuring that the executable permissions /// are retained. +/// +/// This test uses the default link semantics. (On macOS, this is `clone`.) #[test] fn install_executable() -> Result<()> { let temp_dir = assert_fs::TempDir::new()?; @@ -974,8 +976,8 @@ fn install_executable() -> Result<()> { assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) .arg("pip") .arg("install") - .arg("black==24.1.0") - .arg("--strict") + .arg("pylint==3.0.3") + .arg("--cache-dir") .arg(cache_dir.path()) .env("VIRTUAL_ENV", venv.as_os_str()) @@ -985,20 +987,114 @@ fn install_executable() -> Result<()> { ----- stdout ----- ----- stderr ----- - Resolved 6 packages in [TIME] - Downloaded 6 packages in [TIME] - Installed 6 packages in [TIME] - + black==24.1.0 - + click==8.1.7 - + mypy-extensions==1.0.0 - + packaging==23.2 - + pathspec==0.12.1 + Resolved 7 packages in [TIME] + Downloaded 7 packages in [TIME] + Installed 7 packages in [TIME] + + astroid==3.0.2 + + dill==0.3.8 + + isort==5.13.2 + + mccabe==0.7.0 + platformdirs==4.1.0 + + pylint==3.0.3 + + tomlkit==0.12.3 "###); }); - // Activate the virtual environment, and verify that `black` is executable. - let executable = venv.join("bin/black"); + // Verify that `pylint` is executable. + let executable = venv.join("bin/pylint"); + Command::new(executable).arg("--version").assert().success(); + + Ok(()) +} + +/// Install a package into a virtual environment using copy semantics, and ensure that the +/// executable permissions are retained. +#[test] +fn install_executable_copy() -> Result<()> { + let temp_dir = assert_fs::TempDir::new()?; + let cache_dir = assert_fs::TempDir::new()?; + let venv = create_venv_py312(&temp_dir, &cache_dir); + + insta::with_settings!({ + filters => INSTA_FILTERS.to_vec() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip") + .arg("install") + .arg("pylint==3.0.3") + .arg("--link-mode") + .arg("copy") + + .arg("--cache-dir") + .arg(cache_dir.path()) + .env("VIRTUAL_ENV", venv.as_os_str()) + .current_dir(&temp_dir), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 7 packages in [TIME] + Downloaded 7 packages in [TIME] + Installed 7 packages in [TIME] + + astroid==3.0.2 + + dill==0.3.8 + + isort==5.13.2 + + mccabe==0.7.0 + + platformdirs==4.1.0 + + pylint==3.0.3 + + tomlkit==0.12.3 + "###); + }); + + // Verify that `pylint` is executable. + let executable = venv.join("bin/pylint"); + Command::new(executable).arg("--version").assert().success(); + + Ok(()) +} + +/// Install a package into a virtual environment using hardlink semantics, and ensure that the +/// executable permissions are retained. +#[test] +fn install_executable_hardlink() -> Result<()> { + let temp_dir = assert_fs::TempDir::new()?; + let cache_dir = assert_fs::TempDir::new()?; + let venv = create_venv_py312(&temp_dir, &cache_dir); + + insta::with_settings!({ + filters => INSTA_FILTERS.to_vec() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip") + .arg("install") + .arg("pylint==3.0.3") + .arg("--link-mode") + .arg("hardlink") + .arg("--cache-dir") + .arg(cache_dir.path()) + .env("VIRTUAL_ENV", venv.as_os_str()) + .current_dir(&temp_dir), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 7 packages in [TIME] + Downloaded 7 packages in [TIME] + Installed 7 packages in [TIME] + + astroid==3.0.2 + + dill==0.3.8 + + isort==5.13.2 + + mccabe==0.7.0 + + platformdirs==4.1.0 + + pylint==3.0.3 + + tomlkit==0.12.3 + "###); + }); + + // Verify that `pylint` is executable. + let executable = venv.join("bin/pylint"); Command::new(executable).arg("--version").assert().success(); Ok(()) diff --git a/requirements.in b/requirements.in deleted file mode 100644 index 1e9747bf7..000000000 --- a/requirements.in +++ /dev/null @@ -1 +0,0 @@ -ziglang