From ab4da7f0596a7878c6c773554700a8b2aa441231 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Tue, 17 Jan 2023 18:05:07 +1100 Subject: [PATCH] Fix self-update on windows (#712) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed #702 * Add new dep windows v0.44.0 for windows only * Add regression test for #702 * Impl `win::replace_file` * Use `ReplaceFileW` on windows if `fs::rename` in `fs::atomic_install` fails * Improve logging and err messages * Add more regression tests * Make `BinFile::install_link` atomic: Do not remove file if dst exists * Fallback to `ReplaceFileW` in `fs::atomic_symlink_file` * Refactor: Extract new fn `fs::persist` * Use `fs::persist` instead of `TempFile::persist` in `fs::atomic_install`, which fallbacks to `ReplaceFileW` on windows Signed-off-by: Jiahao XU Co-authored-by: Félix Saparelli --- Cargo.lock | 55 ++++++++++++----- crates/binstalk/Cargo.toml | 3 + crates/binstalk/src/bins.rs | 7 --- crates/binstalk/src/fs.rs | 91 +++++++++++++++++++++++----- e2e-tests/self-upgrade-no-symlink.sh | 32 ++++++++++ justfile | 3 +- 6 files changed, 153 insertions(+), 38 deletions(-) create mode 100644 e2e-tests/self-upgrade-no-symlink.sh diff --git a/Cargo.lock b/Cargo.lock index b07434ef..1c569995 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -190,6 +190,7 @@ dependencies = [ "tokio", "tracing", "url", + "windows", "xz2", ] @@ -2895,6 +2896,15 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" +[[package]] +name = "windows" +version = "0.44.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9e745dab35a0c4c77aa3ce42d595e13d2003d6902d6b08c9ef5fc326d08da12b" +dependencies = [ + "windows-targets", +] + [[package]] name = "windows-sys" version = "0.42.0" @@ -2911,46 +2921,61 @@ dependencies = [ ] [[package]] -name = "windows_aarch64_gnullvm" -version = "0.42.0" +name = "windows-targets" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "41d2aa71f6f0cbe00ae5167d90ef3cfe66527d6f613ca78ac8024c3ccab9a19e" +checksum = "8e2522491fbfcd58cc84d47aeb2958948c4b8982e9a2d8a2a35bbaed431390e7" +dependencies = [ + "windows_aarch64_gnullvm", + "windows_aarch64_msvc", + "windows_i686_gnu", + "windows_i686_msvc", + "windows_x86_64_gnu", + "windows_x86_64_gnullvm", + "windows_x86_64_msvc", +] + +[[package]] +name = "windows_aarch64_gnullvm" +version = "0.42.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8c9864e83243fdec7fc9c5444389dcbbfd258f745e7853198f365e3c4968a608" [[package]] name = "windows_aarch64_msvc" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dd0f252f5a35cac83d6311b2e795981f5ee6e67eb1f9a7f64eb4500fbc4dcdb4" +checksum = "4c8b1b673ffc16c47a9ff48570a9d85e25d265735c503681332589af6253c6c7" [[package]] name = "windows_i686_gnu" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fbeae19f6716841636c28d695375df17562ca208b2b7d0dc47635a50ae6c5de7" +checksum = "de3887528ad530ba7bdbb1faa8275ec7a1155a45ffa57c37993960277145d640" [[package]] name = "windows_i686_msvc" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "84c12f65daa39dd2babe6e442988fc329d6243fdce47d7d2d155b8d874862246" +checksum = "bf4d1122317eddd6ff351aa852118a2418ad4214e6613a50e0191f7004372605" [[package]] name = "windows_x86_64_gnu" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bf7b1b21b5362cbc318f686150e5bcea75ecedc74dd157d874d754a2ca44b0ed" +checksum = "c1040f221285e17ebccbc2591ffdc2d44ee1f9186324dd3e84e99ac68d699c45" [[package]] name = "windows_x86_64_gnullvm" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "09d525d2ba30eeb3297665bd434a54297e4170c7f1a44cad4ef58095b4cd2028" +checksum = "628bfdf232daa22b0d64fdb62b09fcc36bb01f05a3939e20ab73aaf9470d0463" [[package]] name = "windows_x86_64_msvc" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f40009d85759725a34da6d89a94e63d7bdc50a862acf0dbc7c8e488f1edcb6f5" +checksum = "447660ad36a13288b1db4d4248e857b510e8c3a225c822ba4fb748c0aafecffd" [[package]] name = "winreg" diff --git a/crates/binstalk/Cargo.toml b/crates/binstalk/Cargo.toml index 0e22f854..c1d9b7ed 100644 --- a/crates/binstalk/Cargo.toml +++ b/crates/binstalk/Cargo.toml @@ -39,6 +39,9 @@ tracing = "0.1.37" url = { version = "2.3.1", features = ["serde"] } xz2 = "0.1.7" +[target.'cfg(target_os = "windows")'.dependencies] +windows = { version = "0.44.0", features = ["Win32_Storage_FileSystem", "Win32_Foundation"] } + [features] default = ["static", "rustls"] diff --git a/crates/binstalk/src/bins.rs b/crates/binstalk/src/bins.rs index 8349337e..38c42fef 100644 --- a/crates/binstalk/src/bins.rs +++ b/crates/binstalk/src/bins.rs @@ -186,13 +186,6 @@ impl BinFile { pub fn install_link(&self) -> Result<(), BinstallError> { if let Some(link) = &self.link { - // Remove existing symlink - // TODO: check if existing symlink is correct - if link.exists() { - debug!("Remove link '{}'", link.display()); - std::fs::remove_file(link)?; - } - let dest = self.link_dest(); debug!( "Create link '{}' pointing to '{}'", diff --git a/crates/binstalk/src/fs.rs b/crates/binstalk/src/fs.rs index 270f6872..8b4c63e5 100644 --- a/crates/binstalk/src/fs.rs +++ b/crates/binstalk/src/fs.rs @@ -1,7 +1,7 @@ use std::{fs, io, path::Path}; -use tempfile::NamedTempFile; -use tracing::debug; +use tempfile::{NamedTempFile, TempPath}; +use tracing::{debug, warn}; /// Atomically install a file. /// @@ -14,7 +14,21 @@ pub fn atomic_install(src: &Path, dst: &Path) -> io::Result<()> { ); if let Err(err) = fs::rename(src, dst) { - debug!("Attempting at atomic rename failed: {err:#?}, fallback to creating tempfile."); + warn!("Attempting at atomic rename failed: {err:#?}, fallback to other methods."); + + #[cfg(target_os = "windows")] + { + match win::replace_file(src, dst) { + Ok(()) => { + debug!("ReplaceFileW succeeded.",); + return Ok(()); + } + Err(err) => { + warn!("ReplaceFileW failed: {err}, fallback to using tempfile plus rename",); + } + } + } + // src and dst is not on the same filesystem/mountpoint. // Fallback to creating NamedTempFile on the parent dir of // dst. @@ -41,12 +55,7 @@ pub fn atomic_install(src: &Path, dst: &Path) -> io::Result<()> { ); tempfile.as_file().set_permissions(permissions)?; - debug!( - "Persisting '{}' to '{}'", - tempfile.path().display(), - dst.display() - ); - tempfile.persist(dst).map_err(io::Error::from)?; + persist(tempfile.into_temp_path(), dst)?; } else { debug!("Attempting at atomically succeeded."); } @@ -80,10 +89,62 @@ pub fn atomic_symlink_file(dest: &Path, link: &Path) -> io::Result<()> { ); symlink_file(dest, &temp_path)?; - debug!( - "Persisting '{}' to '{}'", - temp_path.display(), - link.display() - ); - temp_path.persist(link).map_err(io::Error::from) + persist(temp_path, link) +} + +fn persist(temp_path: TempPath, to: &Path) -> io::Result<()> { + debug!("Persisting '{}' to '{}'", temp_path.display(), to.display()); + match temp_path.persist(to) { + Ok(()) => Ok(()), + #[cfg(target_os = "windows")] + Err(tempfile::PathPersistError { + error, + path: temp_path, + }) => { + warn!( + "Failed to persist symlink '{}' to '{}': {error}, fallback to ReplaceFileW", + temp_path.display(), + to.display(), + ); + win::replace_file(&temp_path, to).map_err(io::Error::from) + } + #[cfg(not(target_os = "windows"))] + Err(err) => Err(err.into()), + } +} + +#[cfg(target_os = "windows")] +mod win { + use std::{os::windows::ffi::OsStrExt, path::Path}; + + use windows::{ + core::{Error, PCWSTR}, + Win32::Storage::FileSystem::{ReplaceFileW, REPLACE_FILE_FLAGS}, + }; + + pub(super) fn replace_file(src: &Path, dst: &Path) -> Result<(), Error> { + let mut src: Vec<_> = src.as_os_str().encode_wide().collect(); + let mut dst: Vec<_> = dst.as_os_str().encode_wide().collect(); + + // Ensure it is terminated with 0 + src.push(0); + dst.push(0); + + // SAFETY: We use it according its doc + // https://learn.microsoft.com/en-nz/windows/win32/api/winbase/nf-winbase-replacefilew + // + // NOTE that this function is available since windows XP, so we don't need to + // lazily load this function. + unsafe { + ReplaceFileW( + PCWSTR::from_raw(dst.as_ptr()), // lpreplacedfilename + PCWSTR::from_raw(src.as_ptr()), // lpreplacementfilename + PCWSTR::null(), // lpbackupfilename, null for no backup file + REPLACE_FILE_FLAGS(0), // dwreplaceflags + None, // lpexclude, unused + None, // lpreserved, unused + ) + } + .ok() + } } diff --git a/e2e-tests/self-upgrade-no-symlink.sh b/e2e-tests/self-upgrade-no-symlink.sh new file mode 100644 index 00000000..66c61ee8 --- /dev/null +++ b/e2e-tests/self-upgrade-no-symlink.sh @@ -0,0 +1,32 @@ +#!/bin/bash + +set -euxo pipefail + +unset CARGO_INSTALL_ROOT + +export CARGO_HOME=$(mktemp -d 2>/dev/null || mktemp -d -t 'cargo-home') +export PATH="$CARGO_HOME/bin:$PATH" + +# first boostrap-install into the CARGO_HOME +mkdir -p "$CARGO_HOME/bin" +cp "./$1" "$CARGO_HOME/bin" + +# now we're running the CARGO_HOME/bin/cargo-binstall (via cargo): + +# self update replacing no-symlinks with no-symlinks +cargo binstall --no-confirm --no-symlinks --force cargo-binstall + +# self update replacing no-symlinks with symlinks +cp "./$1" "$CARGO_HOME/bin" + +cargo binstall --no-confirm --force cargo-binstall + +# self update replacing symlinks with symlinks +ln -snf "$(pwd)/cargo-binstall" "$CARGO_HOME/bin/cargo-binstall" + +cargo binstall --no-confirm --force cargo-binstall + +# self update replacing symlinks with no-symlinks +ln -snf "$(pwd)/cargo-binstall" "$CARGO_HOME/bin/cargo-binstall" + +cargo binstall --no-confirm --force --no-symlinks cargo-binstall diff --git a/justfile b/justfile index aa3f6616..d0d45577 100644 --- a/justfile +++ b/justfile @@ -133,6 +133,7 @@ e2e-test-other-repos: (e2e-test "other-repos") e2e-test-strategies: (e2e-test "strategies") e2e-test-version-syntax: (e2e-test "version-syntax") e2e-test-upgrade: (e2e-test "upgrade") +e2e-test-self-upgrade-no-symlink: (e2e-test "self-upgrade-no-symlink") # WinTLS (Windows in CI) does not have TLS 1.3 support [windows] @@ -141,7 +142,7 @@ e2e-test-tls: (e2e-test "tls" "1.2") [macos] e2e-test-tls: (e2e-test "tls" "1.2") (e2e-test "tls" "1.3") -e2e-tests: e2e-test-live e2e-test-manifest-path e2e-test-other-repos e2e-test-strategies e2e-test-version-syntax e2e-test-upgrade e2e-test-tls +e2e-tests: e2e-test-live e2e-test-manifest-path e2e-test-other-repos e2e-test-strategies e2e-test-version-syntax e2e-test-upgrade e2e-test-tls e2e-test-self-upgrade-no-symlink unit-tests: {{cargo-bin}} test {{cargo-build-args}}