From aec5677b37f77134435737e270464a51881b4158 Mon Sep 17 00:00:00 2001 From: Thomas Castiglione Date: Sun, 4 Feb 2024 16:03:03 +0800 Subject: [PATCH] working_copy: implement symlinks on Windows with a helper function enables symlink tests on windows, but ignores failures due to disabled developer mode working_copy: implement symlinks on Windows with a helper function enables symlink tests on windows, but ignores failures due to disabled developer mode --- Cargo.lock | 11 +++++ Cargo.toml | 3 +- lib/Cargo.toml | 3 ++ lib/src/file_util.rs | 61 ++++++++++++++++++++++++++++ lib/src/local_working_copy.rs | 44 +++++++++----------- lib/src/working_copy.rs | 5 +++ lib/tests/test_local_working_copy.rs | 31 +++++++------- 7 files changed, 119 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e5013210565..011f8db95a4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1692,6 +1692,7 @@ dependencies = [ "version_check", "watchman_client", "whoami", + "winreg", "zstd", ] @@ -3468,6 +3469,16 @@ dependencies = [ "memchr", ] +[[package]] +name = "winreg" +version = "0.52.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a277a57398d4bfa075df44f501a17cfdf8542d224f0d36095a2adc7aee4ef0a5" +dependencies = [ + "cfg-if", + "windows-sys 0.48.0", +] + [[package]] name = "yaml-rust" version = "0.4.5" diff --git a/Cargo.toml b/Cargo.toml index 3f7f41e483a..308edd3e7a0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,7 +58,7 @@ insta = { version = "1.34.0", features = ["filters"] } itertools = "0.12.1" libc = { version = "0.2.153" } maplit = "1.0.2" -minus = { version = "5.5.0", features = [ "dynamic_output", "search" ] } +minus = { version = "5.5.0", features = ["dynamic_output", "search"] } num_cpus = "1.16.0" once_cell = "1.19.0" ouroboros = "0.18.0" @@ -104,6 +104,7 @@ unicode-width = "0.1.11" version_check = "0.9.4" watchman_client = { version = "0.8.0" } whoami = "1.4.1" +winreg = "0.52" zstd = "0.12.4" # put all inter-workspace libraries, i.e. those that use 'path = ...' here in diff --git a/lib/Cargo.toml b/lib/Cargo.toml index 7a480cd05e9..88d598f3eba 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -63,6 +63,9 @@ zstd = { workspace = true } [target.'cfg(unix)'.dependencies] rustix = { workspace = true } +[target.'cfg(windows)'.dependencies] +winreg = { workspace = true } + [dev-dependencies] assert_matches = { workspace = true } criterion = { workspace = true } diff --git a/lib/src/file_util.rs b/lib/src/file_util.rs index d92b9f4e75d..644a04a6f43 100644 --- a/lib/src/file_util.rs +++ b/lib/src/file_util.rs @@ -15,11 +15,19 @@ #![allow(missing_docs)] use std::fs::{self, File}; +#[cfg(windows)] +use std::fs::{remove_dir, symlink_metadata}; +#[cfg(unix)] +use std::os::unix::fs::symlink; +#[cfg(windows)] +use std::os::windows::fs::{symlink_dir, symlink_file}; use std::path::{Component, Path, PathBuf}; use std::{io, iter}; use tempfile::{NamedTempFile, PersistError}; use thiserror::Error; +#[cfg(windows)] +use winreg::{enums::HKEY_LOCAL_MACHINE, RegKey}; #[derive(Debug, Error)] #[error("Cannot access {path}")] @@ -29,6 +37,14 @@ pub struct PathError { pub error: io::Error, } +#[derive(Debug, Error)] +pub enum SymlinkError { + #[error("Developer Mode not enabled")] + DeveloperModeNotEnabled, + #[error("Symlink creation failed")] + Io(#[source] std::io::Error), +} + pub(crate) trait IoResultExt { fn context(self, path: impl AsRef) -> Result; } @@ -146,6 +162,51 @@ pub fn persist_content_addressed_temp_file>( } } +/// Symlinks are always available on UNIX, but on Windows they may not be +/// enabled +#[cfg(unix)] +pub fn check_symlink_support() -> Result { + Ok(true) +} + +/// Symlinks are always available on UNIX, but on Windows they may not be +/// enabled +#[cfg(windows)] +pub fn check_symlink_support() -> Result { + let hklm = RegKey::predef(HKEY_LOCAL_MACHINE); + let sideloading = + hklm.open_subkey("SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\AppModelUnlock")?; + let developer_mode: u32 = sideloading.get_value("AllowDevelopmentWithoutDevLicense")?; + Ok(developer_mode == 1) +} + +#[cfg(windows)] +pub fn try_symlink, Q: AsRef>( + original: P, + link: Q, +) -> Result<(), SymlinkError> { + const ERROR_PRIVILEGE_NOT_HELD: i32 = 1314; + + // this will create a nonfunctional link for directories, but at the moment + // we don't have enough information in the tree to determine whether the + // symlink target is a file or a directory + symlink_file(original, link).map_err(|err| { + if err.raw_os_error() == Some(ERROR_PRIVILEGE_NOT_HELD) { + SymlinkError::DeveloperModeNotEnabled + } else { + SymlinkError::Io(err) + } + }) +} + +#[cfg(unix)] +pub fn try_symlink, Q: AsRef>( + original: P, + link: Q, +) -> Result<(), SymlinkError> { + symlink(original, link).map_err(SymlinkError::Io) +} + #[cfg(test)] mod tests { use std::io::Write; diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 35949ad2211..1061641e644 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -21,8 +21,6 @@ use std::fs::{File, Metadata, OpenOptions}; use std::io::{Read, Write}; use std::ops::Range; #[cfg(unix)] -use std::os::unix::fs::symlink; -#[cfg(unix)] use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; use std::sync::mpsc::{channel, Sender}; @@ -46,6 +44,7 @@ use crate::backend::{ }; use crate::commit::Commit; use crate::conflicts::{self, materialize_tree_value, MaterializedTreeValue}; +use crate::file_util::{check_symlink_support, try_symlink, SymlinkError}; #[cfg(feature = "watchman")] use crate::fsmonitor::watchman; use crate::fsmonitor::FsmonitorKind; @@ -118,7 +117,6 @@ impl FileState { } } - #[cfg(unix)] fn for_symlink(metadata: &Metadata) -> Self { // When using fscrypt, the reported size is not the content size. So if // we were to record the content size here (like we do for regular files), we @@ -294,6 +292,7 @@ pub struct TreeState { // Currently only path prefixes sparse_patterns: Vec, own_mtime: MillisSinceEpoch, + symlink_support: bool, /// The most recent clock value returned by Watchman. Will only be set if /// the repo is configured to use the Watchman filesystem monitor and @@ -549,6 +548,7 @@ impl TreeState { file_states: FileStatesMap::new(), sparse_patterns: vec![RepoPathBuf::root()], own_mtime: MillisSinceEpoch(0), + symlink_support: check_symlink_support().unwrap_or(false), watchman_clock: None, } } @@ -682,8 +682,7 @@ impl TreeState { path: &RepoPath, disk_path: &Path, ) -> Result { - #[cfg(unix)] - { + if self.symlink_support { let target = disk_path.read_link().map_err(|err| SnapshotError::Other { message: format!("Failed to read symlink {}", disk_path.display()), err: err.into(), @@ -695,9 +694,7 @@ impl TreeState { path: disk_path.to_path_buf(), })?; Ok(self.store.write_symlink(path, str_target)?) - } - #[cfg(windows)] - { + } else { let target = fs::read(disk_path).map_err(|err| SnapshotError::Other { message: format!("Failed to read file {}", disk_path.display()), err: err.into(), @@ -1204,28 +1201,23 @@ impl TreeState { Ok(FileState::for_file(executable, size, &metadata)) } - #[cfg_attr(windows, allow(unused_variables))] fn write_symlink(&self, disk_path: &Path, target: String) -> Result { - #[cfg(windows)] - { - self.write_file(disk_path, &mut target.as_bytes(), false) - } - #[cfg(unix)] - { - let target = PathBuf::from(&target); - symlink(&target, disk_path).map_err(|err| CheckoutError::Other { + let target = PathBuf::from(&target); + try_symlink(&target, disk_path).map_err(|inner_err| match inner_err { + SymlinkError::DeveloperModeNotEnabled => CheckoutError::SymlinkNotSupported(inner_err), + SymlinkError::Io(err) => CheckoutError::Other { message: format!( "Failed to create symlink from {} to {}", disk_path.display(), target.display() ), err: err.into(), - })?; - let metadata = disk_path - .symlink_metadata() - .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - Ok(FileState::for_symlink(&metadata)) - } + }, + })?; + let metadata = disk_path + .symlink_metadata() + .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; + Ok(FileState::for_symlink(&metadata)) } fn write_conflict( @@ -1388,7 +1380,11 @@ impl TreeState { .. } => self.write_file(&disk_path, &mut reader, executable)?, MaterializedTreeValue::Symlink { id: _, target } => { - self.write_symlink(&disk_path, target)? + if self.symlink_support { + self.write_symlink(&disk_path, target)? + } else { + self.write_file(&disk_path, &mut target.as_bytes(), false)? + } } MaterializedTreeValue::GitSubmodule(_) => { println!("ignoring git submodule at {path:?}"); diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index bb56b5016e8..25670ffcf06 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -24,6 +24,7 @@ use thiserror::Error; use crate::backend::{BackendError, MergedTreeId}; use crate::commit::Commit; +use crate::file_util::SymlinkError; use crate::fsmonitor::FsmonitorKind; use crate::gitignore::GitIgnoreFile; use crate::op_store::{OperationId, WorkspaceId}; @@ -245,6 +246,10 @@ pub enum CheckoutError { /// Reading or writing from the commit backend failed. #[error("Internal backend error")] InternalBackendError(#[from] BackendError), + /// The working copy contains a symlink, which is not supported + /// on this platform. + #[error("Symlinks are not supported on Windows unless Developer Mode is enabled")] + SymlinkNotSupported(#[from] SymlinkError), /// Some other error happened while checking out the working copy. #[error("{message}")] Other { diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index f79634dc98e..14449b8df70 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -21,6 +21,7 @@ use std::sync::Arc; use itertools::Itertools; use jj_lib::backend::{MergedTreeId, TreeId, TreeValue}; +use jj_lib::file_util::{check_symlink_support, try_symlink}; use jj_lib::fsmonitor::FsmonitorKind; use jj_lib::local_working_copy::LocalWorkingCopy; use jj_lib::merge::Merge; @@ -80,7 +81,6 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { // Executable, but same content as Normal, to test transition where only the bit changed ExecutableNormalContent, Conflict, - #[cfg_attr(windows, allow(dead_code))] Symlink, Tree, GitSubmodule, @@ -170,7 +170,6 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { Kind::Conflict, Kind::Tree, ]; - #[cfg(unix)] kinds.push(Kind::Symlink); if backend == TestRepoBackend::Git { kinds.push(Kind::GitSubmodule); @@ -244,10 +243,12 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { Kind::Symlink => { assert!(maybe_metadata.is_ok(), "{path:?} should exist"); let metadata = maybe_metadata.unwrap(); - assert!( - metadata.file_type().is_symlink(), - "{path:?} should be a symlink" - ); + if check_symlink_support().unwrap_or(false) { + assert!( + metadata.file_type().is_symlink(), + "{path:?} should be a symlink" + ); + } } Kind::Tree => { assert!(maybe_metadata.is_ok(), "{path:?} should exist"); @@ -803,13 +804,11 @@ fn test_gitignores_ignored_directory_already_tracked() { ) .unwrap(); std::fs::remove_file(deleted_executable_path.to_fs_path(&workspace_root)).unwrap(); - if cfg!(unix) { - let fs_path = modified_symlink_path.to_fs_path(&workspace_root); - std::fs::remove_file(&fs_path).unwrap(); - #[cfg(unix)] - std::os::unix::fs::symlink("modified", &fs_path).unwrap(); + let fs_path = modified_symlink_path.to_fs_path(&workspace_root); + std::fs::remove_file(&fs_path).unwrap(); + if check_symlink_support().unwrap_or(false) { + try_symlink("modified", &fs_path, false).unwrap(); } else { - let fs_path = modified_symlink_path.to_fs_path(&workspace_root); std::fs::write(fs_path, "modified").unwrap(); } std::fs::remove_file(deleted_symlink_path.to_fs_path(&workspace_root)).unwrap(); @@ -923,7 +922,6 @@ fn test_gitsubmodule() { ); } -#[cfg(unix)] #[test] fn test_existing_directory_symlink() { let settings = testutils::user_settings(); @@ -933,7 +931,12 @@ fn test_existing_directory_symlink() { // Creates a symlink in working directory, and a tree that will add a file under // the symlinked directory. - std::os::unix::fs::symlink("..", workspace_root.join("parent")).unwrap(); + if check_symlink_support().unwrap_or(false) { + try_symlink("..", workspace_root.join("parent"), true).unwrap(); + } else { + return; + } + let file_path = RepoPath::from_internal_string("parent/escaped"); let tree = create_tree(repo, &[(file_path, "contents")]); let commit = commit_with_tree(repo.store(), tree.id());