Skip to content

Commit

Permalink
working_copy: implement symlinks on Windows with a helper function
Browse files Browse the repository at this point in the history
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
  • Loading branch information
gulbanana committed Feb 15, 2024
1 parent f269097 commit aec5677
Show file tree
Hide file tree
Showing 7 changed files with 119 additions and 39 deletions.
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions lib/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
61 changes: 61 additions & 0 deletions lib/src/file_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}")]
Expand All @@ -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<T> {
fn context(self, path: impl AsRef<Path>) -> Result<T, PathError>;
}
Expand Down Expand Up @@ -146,6 +162,51 @@ pub fn persist_content_addressed_temp_file<P: AsRef<Path>>(
}
}

/// Symlinks are always available on UNIX, but on Windows they may not be
/// enabled
#[cfg(unix)]
pub fn check_symlink_support() -> Result<bool, std::io::Error> {
Ok(true)
}

/// Symlinks are always available on UNIX, but on Windows they may not be
/// enabled
#[cfg(windows)]
pub fn check_symlink_support() -> Result<bool, std::io::Error> {
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<P: AsRef<Path>, Q: AsRef<Path>>(
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<P: AsRef<Path>, Q: AsRef<Path>>(
original: P,
link: Q,
) -> Result<(), SymlinkError> {
symlink(original, link).map_err(SymlinkError::Io)
}

#[cfg(test)]
mod tests {
use std::io::Write;
Expand Down
44 changes: 20 additions & 24 deletions lib/src/local_working_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -294,6 +292,7 @@ pub struct TreeState {
// Currently only path prefixes
sparse_patterns: Vec<RepoPathBuf>,
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
Expand Down Expand Up @@ -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,
}
}
Expand Down Expand Up @@ -682,8 +682,7 @@ impl TreeState {
path: &RepoPath,
disk_path: &Path,
) -> Result<SymlinkId, SnapshotError> {
#[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(),
Expand All @@ -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(),
Expand Down Expand Up @@ -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<FileState, CheckoutError> {
#[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(
Expand Down Expand Up @@ -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:?}");
Expand Down
5 changes: 5 additions & 0 deletions lib/src/working_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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 {
Expand Down
31 changes: 17 additions & 14 deletions lib/tests/test_local_working_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -923,7 +922,6 @@ fn test_gitsubmodule() {
);
}

#[cfg(unix)]
#[test]
fn test_existing_directory_symlink() {
let settings = testutils::user_settings();
Expand All @@ -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());
Expand Down

0 comments on commit aec5677

Please sign in to comment.