diff --git a/eden/fs/cli/redirect.py b/eden/fs/cli/redirect.py index 4cde2cba67..2c4543dc78 100644 --- a/eden/fs/cli/redirect.py +++ b/eden/fs/cli/redirect.py @@ -50,6 +50,29 @@ def have_apfs_helper() -> bool: return False +def determine_bind_redirection_type(instance: EdenInstance) -> str: + """Determine what bind redirection type should be used on macOS. + There are currently only 2 options: "apfs" or "dmg". We default + to the old behavior, "apfs".""" + config_value = instance.get_config_value( + "redirections.darwin-redirection-type", "apfs" + ) + default_type = "apfs" if have_apfs_helper() else "dmg" + if config_value not in ["apfs", "dmg"]: + print( + f'darwin redirection type {config_value} must be either "apfs" or "dmg". Defaulting to {default_type}.' + ) + config_value = default_type + + if config_value == "apfs" and not have_apfs_helper(): + print( + f"cannot use apfs redirections since apfs_helper '{APFS_HELPER}' is not available. Defaulting to {default_type} redirections." + ) + config_value = default_type + + return config_value + + def is_bind_mount(path: Path) -> bool: """Detect the most common form of a bind mount in the repo; its parent directory will have a different device number than @@ -188,7 +211,10 @@ class Redirection: def expand_target_abspath(self, checkout: EdenCheckout) -> Optional[Path]: if self.type == RedirectionType.BIND: - if have_apfs_helper(): + if ( + sys.platform == "darwin" + and determine_bind_redirection_type(checkout.instance) == "apfs" + ): # Ideally we'd return information about the backing, but # it is a bit awkward to determine this in all contexts; # prior to creating the volume we don't know anything @@ -226,10 +252,10 @@ class Redirection: def _bind_mount_darwin( self, instance: EdenInstance, checkout_path: Path, target: Path ) -> None: - if have_apfs_helper(): - return self._bind_mount_darwin_apfs(instance, checkout_path, target) - else: + if determine_bind_redirection_type(instance) == "dmg": return self._bind_mount_darwin_dmg(instance, checkout_path, target) + else: + return self._bind_mount_darwin_apfs(instance, checkout_path, target) def _bind_mount_darwin_apfs( self, instance: EdenInstance, checkout_path: Path, target: Path @@ -254,6 +280,8 @@ class Redirection: total_kb = total / 1024 mount_path = checkout_path / self.repo_path if not image_file_name.exists(): + if not image_file_name.parent.exists(): + image_file_name.parent.mkdir(exist_ok=True, parents=True) run_cmd_quietly( [ "hdiutil", @@ -270,6 +298,8 @@ class Redirection: ] ) + if not mount_path.parent.exists(): + mount_path.parent.mkdir(exist_ok=True, parents=True) run_cmd_quietly( [ "hdiutil", @@ -283,8 +313,11 @@ class Redirection: def _bind_unmount_darwin(self, checkout: EdenCheckout) -> None: mount_path = checkout.path / self.repo_path - # This will unmount/detach both disk images and apfs volumes - run_cmd_quietly(["diskutil", "unmount", "force", mount_path]) + # This will unmount/detach both disk images and apfs volumes. + # `diskutil eject` fully removes the image from the list of disk + # partitions while `diskutil unmount` leaves leftover state for some + # reason. We will use `eject` since they're essentially the same. + run_cmd_quietly(["diskutil", "eject", mount_path]) def _bind_mount_linux( self, instance: EdenInstance, checkout_path: Path, target: Path diff --git a/eden/fs/cli_rs/edenfs-client/Cargo.toml b/eden/fs/cli_rs/edenfs-client/Cargo.toml index edd5ea2265..e9cc18731a 100644 --- a/eden/fs/cli_rs/edenfs-client/Cargo.toml +++ b/eden/fs/cli_rs/edenfs-client/Cargo.toml @@ -42,6 +42,7 @@ tempfile = "3.3" [target.'cfg(target_os = "macos")'.dependencies] nix = "0.25" +psutil = "3.2.2" [target.'cfg(target_os = "windows")'.dependencies] mkscratch = { version = "0.1.0", path = "../../../scm/exec/scratch" } diff --git a/eden/fs/cli_rs/edenfs-client/src/redirect.rs b/eden/fs/cli_rs/edenfs-client/src/redirect.rs index e9f63cea89..23d01500ed 100644 --- a/eden/fs/cli_rs/edenfs-client/src/redirect.rs +++ b/eden/fs/cli_rs/edenfs-client/src/redirect.rs @@ -35,9 +35,9 @@ use edenfs_utils::stop_buckd_for_repo; use fbinit::expect_init; #[cfg(target_os = "windows")] use mkscratch::zzencode; -#[cfg(target_os = "macos")] -use nix::sys::stat::stat; use pathdiff::diff_paths; +#[cfg(target_os = "macos")] +use psutil::disk::disk_usage; use serde::Deserialize; use serde::Deserializer; use serde::Serialize; @@ -352,8 +352,9 @@ impl Redirection { pub fn expand_target_abspath(&self, checkout: &EdenFsCheckout) -> Result> { match self.redir_type { + #[cfg(target_os = "macos")] RedirectionType::Bind => { - if Redirection::have_apfs_helper()? { + if Self::determine_bind_redirection_type() == DarwinBindRedirectionType::APFS { // Ideally we'd return information about the backing, but // it is a bit awkward to determine this in all contexts; // prior to creating the volume we don't know anything @@ -380,6 +381,11 @@ impl Redirection { )?)) } } + #[cfg(not(target_os = "macos"))] + RedirectionType::Bind => Ok(Some(Redirection::make_scratch_dir( + checkout, + &self.repo_path, + )?)), RedirectionType::Symlink => Ok(Some(Redirection::make_scratch_dir( checkout, &self.repo_path, @@ -477,31 +483,44 @@ impl Redirection { // Since we don't have bind mounts, we set up a disk image file // and mount that instead. let image_file_path = self._dmg_file_name(target); - let target_stat = stat(target) + let target_stat = disk_usage(target) .from_err() .with_context(|| format!("Failed to stat target {}", target.display()))?; // Specify the size in kb because the disk utilities have weird // defaults if the units are unspecified, and `b` doesn't mean // bytes! - let total_kb = target_stat.st_size / 1024; + let total_kib = target_stat.total() / 1024; let mount_path = checkout_path.join(&self.repo_path()); - if !image_file_path.exists() { - // We need to convert paths -> strings for the hdiutil commands - let image_file_name = image_file_path.to_string_lossy(); - let mount_name = mount_path.to_string_lossy(); + // We need to convert paths -> strings for the hdiutil commands + let image_file_name = image_file_path.to_string_lossy(); + let mount_name = mount_path.to_string_lossy(); + if !image_file_path.exists() { + let image_file_dir = image_file_path.parent().with_context(|| { + format!( + "image file {} must exist in some parent directory", + &image_file_path.display() + ) + })?; + if !image_file_dir.exists() { + std::fs::create_dir_all(&image_file_dir) + .from_err() + .with_context(|| { + format!("Failed to create directory {}", &image_file_dir.display()) + })?; + } let args = &[ "create", - "--size", - &format!("{}k", total_kb), - "--type", + "-size", + &format!("{}k", total_kib), + "-type", "SPARSE", - "--fs", + "-fs", "HFS+", - "--volname", - &format!("EdenFS redirection for {}", &mount_name), + "-volname", + &format!("'EdenFS redirection for {}'", &mount_name), &image_file_name, ]; let create_output = Command::new("hdiutil") @@ -520,30 +539,38 @@ impl Redirection { String::from_utf8_lossy(&create_output.stdout) ))); } - - let args = &[ - "attach", - &image_file_name, - "--nobrowse", - "--mountpoint", - &mount_name, - ]; - let attach_output = Command::new("hdiutil") - .args(args) - .output() + } + let args = &[ + "attach", + &image_file_name, + "-nobrowse", + "-mountpoint", + &mount_name, + ]; + let mount_path = mount_path.parent().with_context(|| { + format!( + "mount path {} must exist in some parent directory", + &mount_path.display() + ) + })?; + if !mount_path.exists() { + std::fs::create_dir_all(&mount_path) .from_err() - .with_context(|| { - format!("Failed to execute command `hdiutil {}`", args.join(" ")) - })?; - if !attach_output.status.success() { - return Err(EdenFsError::Other(anyhow!( - "failed to attach dmg volume {} for mount {}. stderr: {}\n stdout: {}", - &image_file_name, - &mount_name, - String::from_utf8_lossy(&attach_output.stderr), - String::from_utf8_lossy(&attach_output.stdout) - ))); - } + .with_context(|| format!("Failed to create directory {}", &mount_path.display()))?; + } + let attach_output = Command::new("hdiutil") + .args(args) + .output() + .from_err() + .with_context(|| format!("Failed to execute command `hdiutil {}`", args.join(" ")))?; + if !attach_output.status.success() { + return Err(EdenFsError::Other(anyhow!( + "failed to attach dmg volume {} for mount {}. stderr: {}\n stdout: {}", + &image_file_name, + &mount_name, + String::from_utf8_lossy(&attach_output.stderr), + String::from_utf8_lossy(&attach_output.stdout) + ))); } Ok(()) } @@ -606,7 +633,10 @@ impl Redirection { #[cfg(target_os = "macos")] fn _bind_unmount_darwin(&self, checkout: &EdenFsCheckout) -> Result<()> { let mount_path = checkout.path().join(&self.repo_path); - let args = &["unmount", "force", &mount_path.to_string_lossy()]; + // `diskutil eject` fully removes the image from the list of disk partitions while + // `diskutil unmount` leaves leftover state for some reason. We will use `eject` since + // they're essentially the same. + let args = &["eject", &mount_path.to_string_lossy()]; let output = Command::new("diskutil") .args(args) .output() diff --git a/eden/integration/lib/testcase.py b/eden/integration/lib/testcase.py index 37349860c3..993af45216 100644 --- a/eden/integration/lib/testcase.py +++ b/eden/integration/lib/testcase.py @@ -256,6 +256,11 @@ class EdenTestCase(EdenTestCaseBase): configs = {"experimental": ["enable-nfs-server = true"]} if self.use_nfs(): configs["clone"] = ['default-mount-protocol = "NFS"'] + # The number of concurrent APFS volumes we can create on macOS + # Sandcastle hosts is extremely limited. Therefore, let's use disk + # image redirections instead of APFS volume redirections. + if sys.platform == "darwin": + configs["redirections"] = ['darwin-redirection-type = "dmg"'] return configs def create_hg_repo(