From cc5011866d2d5fc7f02d9af88a0251b18ce62486 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Thu, 18 Jun 2020 13:43:41 +0200 Subject: [PATCH 01/13] Exclude the target directory from backups using CACHEDIR.TAG This patch follows the lead of #4386 (which excludes target directories from Time Machine backups) and is motived by the same reasons listen in #3884. CACHEDIR.TAG is an OS-independent mechanism supported by Borg, restic, GNU Tar and other backup/archiving solutions. See https://bford.info/cachedir/ for more information about the specification. This has been discussed in Rust Internals earlier this year[1] and it seems like it's an uncontroversial improvement so I went ahead with the patch. [1] https://internals.rust-lang.org/t/pre-rfc-put-cachedir-tag-into-target/12262/11 --- src/cargo/core/compiler/layout.rs | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/compiler/layout.rs b/src/cargo/core/compiler/layout.rs index 53c615fbe1d..d6f11845fd5 100644 --- a/src/cargo/core/compiler/layout.rs +++ b/src/cargo/core/compiler/layout.rs @@ -220,14 +220,30 @@ impl Layout { } } +/// Marks the directory as excluded from archives/backups. +/// +/// This is recommended to prevent derived/temporary files from bloating backups. There are two +/// mechanisms used to achieve this right now: +/// +/// * A dedicated resource property excluding from Time Machine backups on macOS +/// * CACHEDIR.TAG files supported by various tools in a platform-independent way +fn exclude_from_backups(path: &Path) { + exclude_from_time_machine(path); + let _ = std::fs::write( + path.join("CACHEDIR.TAG"), + "Signature: 8a477f597d28d172789f06886806bc55 +# This file is a cache directory tag created by cargo. +# For information about cache directory tags see https://bford.info/cachedir/", + ); + // Similarly to exclude_from_time_machine() we ignore errors here as it's an optional feature. +} + #[cfg(not(target_os = "macos"))] -fn exclude_from_backups(_: &Path) {} +fn exclude_from_time_machine(path: &Path) {} #[cfg(target_os = "macos")] /// Marks files or directories as excluded from Time Machine on macOS -/// -/// This is recommended to prevent derived/temporary files from bloating backups. -fn exclude_from_backups(path: &Path) { +fn exclude_from_time_machine(path: &Path) { use core_foundation::base::TCFType; use core_foundation::{number, string, url}; use std::ptr; From 4c9efbd44bf6260ec8d2713118146c88e93df55e Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Thu, 18 Jun 2020 14:20:36 +0200 Subject: [PATCH 02/13] Revert one unintentional change --- src/cargo/core/compiler/layout.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/core/compiler/layout.rs b/src/cargo/core/compiler/layout.rs index d6f11845fd5..8ddaac06131 100644 --- a/src/cargo/core/compiler/layout.rs +++ b/src/cargo/core/compiler/layout.rs @@ -239,7 +239,7 @@ fn exclude_from_backups(path: &Path) { } #[cfg(not(target_os = "macos"))] -fn exclude_from_time_machine(path: &Path) {} +fn exclude_from_time_machine(_: &Path) {} #[cfg(target_os = "macos")] /// Marks files or directories as excluded from Time Machine on macOS From f9807e086d54b5dc999b09c22a3532f5abb8ecb6 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Thu, 18 Jun 2020 16:07:46 +0200 Subject: [PATCH 03/13] Remove CACHEDIR.TAG as part of cargo clean --- src/cargo/ops/cargo_clean.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index a8b39c4b5b9..ad1fafa9cbd 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -175,6 +175,7 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { // TODO: what to do about build_script_build? let incremental = layout.incremental().join(format!("{}-*", crate_name)); rm_rf_glob(&incremental, config)?; + rm_rf(&uplift_dir.join("CACHEDIR.TAG"), config)?; } } } From efac9cc5919fffbcd86ba1be601101ab185c3ca6 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Thu, 18 Jun 2020 17:49:36 +0200 Subject: [PATCH 04/13] Fix a test --- tests/testsuite/clean.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testsuite/clean.rs b/tests/testsuite/clean.rs index 673dc4969a1..e2853e6ac8f 100644 --- a/tests/testsuite/clean.rs +++ b/tests/testsuite/clean.rs @@ -293,6 +293,7 @@ fn clean_verbose() { [REMOVING] [..] [REMOVING] [..] [REMOVING] [..] +[REMOVING] [..] ", ) .run(); From 2793d7bf62aae0212c0c31430069445966aa2c26 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Fri, 19 Jun 2020 15:24:51 +0200 Subject: [PATCH 05/13] Make target directory creation and backup exclusion atomic --- src/cargo/core/compiler/layout.rs | 35 +++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/compiler/layout.rs b/src/cargo/core/compiler/layout.rs index 8ddaac06131..23f31de350b 100644 --- a/src/cargo/core/compiler/layout.rs +++ b/src/cargo/core/compiler/layout.rs @@ -102,7 +102,9 @@ use crate::core::compiler::CompileTarget; use crate::core::Workspace; use crate::util::paths; use crate::util::{CargoResult, FileLock}; +use std::fs; use std::path::{Path, PathBuf}; +use tempfile::Builder as TempFileBuilder; /// Contains the paths of all target output locations. /// @@ -146,13 +148,42 @@ impl Layout { if let Some(target) = target { root.push(target.short_name()); } + // We need root to exist before we do the tempdir/rename dance inside it. + if !root.as_path_unlocked().exists() { + root.create_dir()?; + } let dest = root.join(dest); // If the root directory doesn't already exist go ahead and create it // here. Use this opportunity to exclude it from backups as well if the // system supports it since this is a freshly created folder. + // + // We do this in two steps (first create a temporary directory and exlucde + // it from backups, then rename it to the desired name. If we created the + // directory directly where it should be and then excluded it from backups + // we would risk a situation where cargo is interrupted right after the directory + // creation but before the exclusion the the directory would remain non-excluded from + // backups because we only perform exclusion right after we created the directory + // ourselves. if !dest.as_path_unlocked().exists() { - dest.create_dir()?; - exclude_from_backups(dest.as_path_unlocked()); + // We need the tempdir created in root instead of $TMP, because only then we can be + // easily sure that rename() will succeed (the new name needs to be on the same mount + // point as the old one). + let tempdir = TempFileBuilder::new() + .prefix("cargo-target") + .tempdir_in(root.as_path_unlocked())? + .into_path(); + exclude_from_backups(&tempdir); + // Previously std::fs::create_dir_all() (through paths::create_dir_all()) was used + // here to create the directory directly and fs::create_dir_all() explicitly treats + // the directory being created concurrently by another thread or process as success, + // hence the check below to follow the existing behavior. If we get an error at + // rename() and suddently the directory (which didn't exist a moment earlier) exists + // we can infer from it it's another cargo process doing work. + if let Err(e) = fs::rename(&tempdir, dest.as_path_unlocked()) { + if !dest.as_path_unlocked().exists() { + return Err(anyhow::Error::from(e)); + } + } } // For now we don't do any more finer-grained locking on the artifact From ce024016491b07b73fbc07e79b80333a7fe804ff Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Fri, 19 Jun 2020 15:39:44 +0200 Subject: [PATCH 06/13] Make sure the target tempdir is cleaned if rename fails --- src/cargo/core/compiler/layout.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/compiler/layout.rs b/src/cargo/core/compiler/layout.rs index 23f31de350b..2cb6742480a 100644 --- a/src/cargo/core/compiler/layout.rs +++ b/src/cargo/core/compiler/layout.rs @@ -170,16 +170,15 @@ impl Layout { // point as the old one). let tempdir = TempFileBuilder::new() .prefix("cargo-target") - .tempdir_in(root.as_path_unlocked())? - .into_path(); - exclude_from_backups(&tempdir); + .tempdir_in(root.as_path_unlocked())?; + exclude_from_backups(&tempdir.path()); // Previously std::fs::create_dir_all() (through paths::create_dir_all()) was used // here to create the directory directly and fs::create_dir_all() explicitly treats // the directory being created concurrently by another thread or process as success, // hence the check below to follow the existing behavior. If we get an error at // rename() and suddently the directory (which didn't exist a moment earlier) exists // we can infer from it it's another cargo process doing work. - if let Err(e) = fs::rename(&tempdir, dest.as_path_unlocked()) { + if let Err(e) = fs::rename(tempdir.path(), dest.as_path_unlocked()) { if !dest.as_path_unlocked().exists() { return Err(anyhow::Error::from(e)); } From 7a0001df850556ccf781a48d99a9936a67e96a71 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Fri, 19 Jun 2020 15:41:29 +0200 Subject: [PATCH 07/13] Use more appropriate basename for the tempdir --- src/cargo/core/compiler/layout.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/compiler/layout.rs b/src/cargo/core/compiler/layout.rs index 2cb6742480a..90a50a595be 100644 --- a/src/cargo/core/compiler/layout.rs +++ b/src/cargo/core/compiler/layout.rs @@ -152,6 +152,7 @@ impl Layout { if !root.as_path_unlocked().exists() { root.create_dir()?; } + let dest_base = dest; let dest = root.join(dest); // If the root directory doesn't already exist go ahead and create it // here. Use this opportunity to exclude it from backups as well if the @@ -169,7 +170,7 @@ impl Layout { // easily sure that rename() will succeed (the new name needs to be on the same mount // point as the old one). let tempdir = TempFileBuilder::new() - .prefix("cargo-target") + .prefix(dest_base) .tempdir_in(root.as_path_unlocked())?; exclude_from_backups(&tempdir.path()); // Previously std::fs::create_dir_all() (through paths::create_dir_all()) was used From 3eb2ae045c8853a361c92be7992355265b96e212 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Wed, 24 Jun 2020 23:22:20 +0200 Subject: [PATCH 08/13] Add two tests --- tests/testsuite/build.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index a63bc7a569d..a03eb1c827b 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -4989,3 +4989,32 @@ fn reduced_reproduction_8249() { p.cargo("check").run(); p.cargo("check").run(); } + +#[cargo_test] +fn target_directory_is_excluded_from_backups() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file("src/foo.rs", &main_file(r#""i am foo""#, &[])) + .build(); + + p.cargo("build").run(); + let cachedir_tag = p.target_debug_dir().join("CACHEDIR.TAG"); + assert!(cachedir_tag.is_file()); + assert!(fs::read_to_string(&cachedir_tag) + .unwrap() + .starts_with("Signature: 8a477f597d28d172789f06886806bc55")); +} + +#[cargo_test] +fn target_directory_is_not_excluded_from_backups_if_it_already_exists() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file("src/foo.rs", &main_file(r#""i am foo""#, &[])) + .build(); + + let cachedir_tag = p.target_debug_dir().join("CACHEDIR.TAG"); + p.cargo("build").run(); + fs::remove_file(&cachedir_tag).unwrap(); + p.cargo("build").run(); + assert!(!&cachedir_tag.is_file()); +} From 50f2290cfad265cbc2ac6c969d01ede54bfefcb5 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Wed, 24 Jun 2020 23:49:56 +0200 Subject: [PATCH 09/13] Try to resolve a build-time conflict with master This commit changed things a bit: 6263d726f6b32ff7bcb5e1c292e0c83d65145e41. --- src/cargo/ops/cargo_clean.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index ad1fafa9cbd..bfed10d947b 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -175,7 +175,9 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { // TODO: what to do about build_script_build? let incremental = layout.incremental().join(format!("{}-*", crate_name)); rm_rf_glob(&incremental, config)?; - rm_rf(&uplift_dir.join("CACHEDIR.TAG"), config)?; + if let Some(uplift_dir) = uplift_dir { + rm_rf(&uplift_dir.join("CACHEDIR.TAG"), config)?; + } } } } From 1b5f749739c8e96ad175d2453eec5507c9cd4585 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Wed, 1 Jul 2020 14:11:07 +0200 Subject: [PATCH 10/13] Exclude target/doc from backups as well --- src/cargo/core/compiler/layout.rs | 80 +-------------------------- src/cargo/core/compiler/mod.rs | 2 +- src/cargo/util/paths.rs | 89 +++++++++++++++++++++++++++++++ tests/testsuite/doc.rs | 64 ++++++++++++++++++++++ 4 files changed, 155 insertions(+), 80 deletions(-) diff --git a/src/cargo/core/compiler/layout.rs b/src/cargo/core/compiler/layout.rs index 90a50a595be..3360aa143df 100644 --- a/src/cargo/core/compiler/layout.rs +++ b/src/cargo/core/compiler/layout.rs @@ -102,9 +102,7 @@ use crate::core::compiler::CompileTarget; use crate::core::Workspace; use crate::util::paths; use crate::util::{CargoResult, FileLock}; -use std::fs; use std::path::{Path, PathBuf}; -use tempfile::Builder as TempFileBuilder; /// Contains the paths of all target output locations. /// @@ -148,43 +146,12 @@ impl Layout { if let Some(target) = target { root.push(target.short_name()); } - // We need root to exist before we do the tempdir/rename dance inside it. - if !root.as_path_unlocked().exists() { - root.create_dir()?; - } - let dest_base = dest; let dest = root.join(dest); // If the root directory doesn't already exist go ahead and create it // here. Use this opportunity to exclude it from backups as well if the // system supports it since this is a freshly created folder. // - // We do this in two steps (first create a temporary directory and exlucde - // it from backups, then rename it to the desired name. If we created the - // directory directly where it should be and then excluded it from backups - // we would risk a situation where cargo is interrupted right after the directory - // creation but before the exclusion the the directory would remain non-excluded from - // backups because we only perform exclusion right after we created the directory - // ourselves. - if !dest.as_path_unlocked().exists() { - // We need the tempdir created in root instead of $TMP, because only then we can be - // easily sure that rename() will succeed (the new name needs to be on the same mount - // point as the old one). - let tempdir = TempFileBuilder::new() - .prefix(dest_base) - .tempdir_in(root.as_path_unlocked())?; - exclude_from_backups(&tempdir.path()); - // Previously std::fs::create_dir_all() (through paths::create_dir_all()) was used - // here to create the directory directly and fs::create_dir_all() explicitly treats - // the directory being created concurrently by another thread or process as success, - // hence the check below to follow the existing behavior. If we get an error at - // rename() and suddently the directory (which didn't exist a moment earlier) exists - // we can infer from it it's another cargo process doing work. - if let Err(e) = fs::rename(tempdir.path(), dest.as_path_unlocked()) { - if !dest.as_path_unlocked().exists() { - return Err(anyhow::Error::from(e)); - } - } - } + paths::create_dir_all_excluded_from_backups_atomic(dest.as_path_unlocked())?; // For now we don't do any more finer-grained locking on the artifact // directory, so just lock the entire thing for the duration of this @@ -250,48 +217,3 @@ impl Layout { &self.build } } - -/// Marks the directory as excluded from archives/backups. -/// -/// This is recommended to prevent derived/temporary files from bloating backups. There are two -/// mechanisms used to achieve this right now: -/// -/// * A dedicated resource property excluding from Time Machine backups on macOS -/// * CACHEDIR.TAG files supported by various tools in a platform-independent way -fn exclude_from_backups(path: &Path) { - exclude_from_time_machine(path); - let _ = std::fs::write( - path.join("CACHEDIR.TAG"), - "Signature: 8a477f597d28d172789f06886806bc55 -# This file is a cache directory tag created by cargo. -# For information about cache directory tags see https://bford.info/cachedir/", - ); - // Similarly to exclude_from_time_machine() we ignore errors here as it's an optional feature. -} - -#[cfg(not(target_os = "macos"))] -fn exclude_from_time_machine(_: &Path) {} - -#[cfg(target_os = "macos")] -/// Marks files or directories as excluded from Time Machine on macOS -fn exclude_from_time_machine(path: &Path) { - use core_foundation::base::TCFType; - use core_foundation::{number, string, url}; - use std::ptr; - - // For compatibility with 10.7 a string is used instead of global kCFURLIsExcludedFromBackupKey - let is_excluded_key: Result = "NSURLIsExcludedFromBackupKey".parse(); - let path = url::CFURL::from_path(path, false); - if let (Some(path), Ok(is_excluded_key)) = (path, is_excluded_key) { - unsafe { - url::CFURLSetResourcePropertyForKey( - path.as_concrete_TypeRef(), - is_excluded_key.as_concrete_TypeRef(), - number::kCFBooleanTrue as *const _, - ptr::null_mut(), - ); - } - } - // Errors are ignored, since it's an optional feature and failure - // doesn't prevent Cargo from working -} diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index dd2a3c5da20..2c15ea6932b 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -557,7 +557,7 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { // Create the documentation directory ahead of time as rustdoc currently has // a bug where concurrent invocations will race to create this directory if // it doesn't already exist. - paths::create_dir_all(&doc_dir)?; + paths::create_dir_all_excluded_from_backups_atomic(&doc_dir)?; rustdoc.arg("-o").arg(doc_dir); diff --git a/src/cargo/util/paths.rs b/src/cargo/util/paths.rs index c5a09fdae05..fc52cc14e58 100644 --- a/src/cargo/util/paths.rs +++ b/src/cargo/util/paths.rs @@ -7,6 +7,7 @@ use std::iter; use std::path::{Component, Path, PathBuf}; use filetime::FileTime; +use tempfile::Builder as TempFileBuilder; use crate::util::errors::{CargoResult, CargoResultExt}; @@ -457,3 +458,91 @@ pub fn strip_prefix_canonical>( let canon_base = safe_canonicalize(base.as_ref()); canon_path.strip_prefix(canon_base).map(|p| p.to_path_buf()) } + +/// Creates an excluded from cache directory atomically with its parents as needed. +/// +/// The atomicity only covers creating the leaf directory and exclusion from cache. Any missing +/// parent directories will not be created in an atomic manner. +/// +/// This function is idempotent and in addition to that it won't exclude ``p`` from cache if it +/// already exists. +pub fn create_dir_all_excluded_from_backups_atomic(p: impl AsRef) -> CargoResult<()> { + let path = p.as_ref(); + if path.is_dir() { + return Ok(()); + } + + let parent = path.parent().unwrap(); + let base = path.file_name().unwrap(); + create_dir_all(parent)?; + // We do this in two steps (first create a temporary directory and exlucde + // it from backups, then rename it to the desired name. If we created the + // directory directly where it should be and then excluded it from backups + // we would risk a situation where cargo is interrupted right after the directory + // creation but before the exclusion the the directory would remain non-excluded from + // backups because we only perform exclusion right after we created the directory + // ourselves. + // + // We need the tempdir created in parent instead of $TMP, because only then we can be + // easily sure that rename() will succeed (the new name needs to be on the same mount + // point as the old one). + let tempdir = TempFileBuilder::new().prefix(base).tempdir_in(parent)?; + exclude_from_backups(&tempdir.path()); + // Previously std::fs::create_dir_all() (through paths::create_dir_all()) was used + // here to create the directory directly and fs::create_dir_all() explicitly treats + // the directory being created concurrently by another thread or process as success, + // hence the check below to follow the existing behavior. If we get an error at + // rename() and suddently the directory (which didn't exist a moment earlier) exists + // we can infer from it it's another cargo process doing work. + if let Err(e) = fs::rename(tempdir.path(), path) { + if !path.exists() { + return Err(anyhow::Error::from(e)); + } + } + Ok(()) +} + +/// Marks the directory as excluded from archives/backups. +/// +/// This is recommended to prevent derived/temporary files from bloating backups. There are two +/// mechanisms used to achieve this right now: +/// +/// * A dedicated resource property excluding from Time Machine backups on macOS +/// * CACHEDIR.TAG files supported by various tools in a platform-independent way +fn exclude_from_backups(path: &Path) { + exclude_from_time_machine(path); + let _ = std::fs::write( + path.join("CACHEDIR.TAG"), + "Signature: 8a477f597d28d172789f06886806bc55 +# This file is a cache directory tag created by cargo. +# For information about cache directory tags see https://bford.info/cachedir/", + ); + // Similarly to exclude_from_time_machine() we ignore errors here as it's an optional feature. +} + +#[cfg(not(target_os = "macos"))] +fn exclude_from_time_machine(_: &Path) {} + +#[cfg(target_os = "macos")] +/// Marks files or directories as excluded from Time Machine on macOS +fn exclude_from_time_machine(path: &Path) { + use core_foundation::base::TCFType; + use core_foundation::{number, string, url}; + use std::ptr; + + // For compatibility with 10.7 a string is used instead of global kCFURLIsExcludedFromBackupKey + let is_excluded_key: Result = "NSURLIsExcludedFromBackupKey".parse(); + let path = url::CFURL::from_path(path, false); + if let (Some(path), Ok(is_excluded_key)) = (path, is_excluded_key) { + unsafe { + url::CFURLSetResourcePropertyForKey( + path.as_concrete_TypeRef(), + is_excluded_key.as_concrete_TypeRef(), + number::kCFBooleanTrue as *const _, + ptr::null_mut(), + ); + } + } + // Errors are ignored, since it's an optional feature and failure + // doesn't prevent Cargo from working +} diff --git a/tests/testsuite/doc.rs b/tests/testsuite/doc.rs index 0a4a914ef67..8d02a092a09 100644 --- a/tests/testsuite/doc.rs +++ b/tests/testsuite/doc.rs @@ -1551,3 +1551,67 @@ fn crate_versions_flag_is_overridden() { .run(); asserts(output_documentation()); } + +#[cargo_test] +fn target_directory_is_excluded_from_backups() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + build = "build.rs" + "#, + ) + .file("build.rs", "fn main() {}") + .file("src/lib.rs", "pub fn foo() {}") + .build(); + + p.cargo("doc") + .with_stderr( + "\ +[..] foo v0.0.1 ([CWD]) +[..] foo v0.0.1 ([CWD]) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + ) + .run(); + let cachedir_tag = p.root().join("target/doc/CACHEDIR.TAG"); + assert!(cachedir_tag.is_file()); + assert!(fs::read_to_string(&cachedir_tag) + .unwrap() + .starts_with("Signature: 8a477f597d28d172789f06886806bc55")); +} + +#[cargo_test] +fn target_directory_is_not_excluded_from_backups_if_it_already_exists() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + build = "build.rs" + "#, + ) + .file("build.rs", "fn main() {}") + .file("src/lib.rs", "pub fn foo() {}") + .build(); + fs::create_dir_all(p.root().join("target/doc")).unwrap(); + + p.cargo("doc") + .with_stderr( + "\ +[..] foo v0.0.1 ([CWD]) +[..] foo v0.0.1 ([CWD]) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + ) + .run(); + let cachedir_tag = p.root().join("target/doc/CACHEDIR.TAG"); + assert!(!&cachedir_tag.is_file()); +} From f34b086949fb14bfbe286e818d645798285a3140 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Wed, 1 Jul 2020 22:40:31 +0200 Subject: [PATCH 11/13] Exclude whole target/ from backups This is following the discussion on GitHub. The doc tests are no longer necessary because Layout::new() creates CACHEDIR.TAG directly in target root, no doc-specific code is necessary anymore. --- src/cargo/core/compiler/layout.rs | 5 ++- src/cargo/core/compiler/mod.rs | 2 +- tests/testsuite/build.rs | 4 +- tests/testsuite/clean.rs | 5 ++- tests/testsuite/doc.rs | 64 ------------------------------- 5 files changed, 10 insertions(+), 70 deletions(-) diff --git a/src/cargo/core/compiler/layout.rs b/src/cargo/core/compiler/layout.rs index 3360aa143df..29163122ea1 100644 --- a/src/cargo/core/compiler/layout.rs +++ b/src/cargo/core/compiler/layout.rs @@ -151,7 +151,10 @@ impl Layout { // here. Use this opportunity to exclude it from backups as well if the // system supports it since this is a freshly created folder. // - paths::create_dir_all_excluded_from_backups_atomic(dest.as_path_unlocked())?; + paths::create_dir_all_excluded_from_backups_atomic(root.as_path_unlocked())?; + // Now that the excluded from backups target root is created we can create the + // actual destination (sub)subdirectory. + paths::create_dir_all(dest.as_path_unlocked())?; // For now we don't do any more finer-grained locking on the artifact // directory, so just lock the entire thing for the duration of this diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 2c15ea6932b..dd2a3c5da20 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -557,7 +557,7 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { // Create the documentation directory ahead of time as rustdoc currently has // a bug where concurrent invocations will race to create this directory if // it doesn't already exist. - paths::create_dir_all_excluded_from_backups_atomic(&doc_dir)?; + paths::create_dir_all(&doc_dir)?; rustdoc.arg("-o").arg(doc_dir); diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index a03eb1c827b..b3bc752fea0 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -4998,7 +4998,7 @@ fn target_directory_is_excluded_from_backups() { .build(); p.cargo("build").run(); - let cachedir_tag = p.target_debug_dir().join("CACHEDIR.TAG"); + let cachedir_tag = p.build_dir().join("CACHEDIR.TAG"); assert!(cachedir_tag.is_file()); assert!(fs::read_to_string(&cachedir_tag) .unwrap() @@ -5012,7 +5012,7 @@ fn target_directory_is_not_excluded_from_backups_if_it_already_exists() { .file("src/foo.rs", &main_file(r#""i am foo""#, &[])) .build(); - let cachedir_tag = p.target_debug_dir().join("CACHEDIR.TAG"); + let cachedir_tag = p.build_dir().join("CACHEDIR.TAG"); p.cargo("build").run(); fs::remove_file(&cachedir_tag).unwrap(); p.cargo("build").run(); diff --git a/tests/testsuite/clean.rs b/tests/testsuite/clean.rs index e2853e6ac8f..69b14bd17c7 100644 --- a/tests/testsuite/clean.rs +++ b/tests/testsuite/clean.rs @@ -293,7 +293,6 @@ fn clean_verbose() { [REMOVING] [..] [REMOVING] [..] [REMOVING] [..] -[REMOVING] [..] ", ) .run(); @@ -433,7 +432,9 @@ fn assert_all_clean(build_dir: &Path) { }) { let entry = entry.unwrap(); let path = entry.path(); - if let ".rustc_info.json" | ".cargo-lock" = path.file_name().unwrap().to_str().unwrap() { + if let ".rustc_info.json" | ".cargo-lock" | "CACHEDIR.TAG" = + path.file_name().unwrap().to_str().unwrap() + { continue; } if path.is_symlink() || path.is_file() { diff --git a/tests/testsuite/doc.rs b/tests/testsuite/doc.rs index 8d02a092a09..0a4a914ef67 100644 --- a/tests/testsuite/doc.rs +++ b/tests/testsuite/doc.rs @@ -1551,67 +1551,3 @@ fn crate_versions_flag_is_overridden() { .run(); asserts(output_documentation()); } - -#[cargo_test] -fn target_directory_is_excluded_from_backups() { - let p = project() - .file( - "Cargo.toml", - r#" - [package] - name = "foo" - version = "0.0.1" - authors = [] - build = "build.rs" - "#, - ) - .file("build.rs", "fn main() {}") - .file("src/lib.rs", "pub fn foo() {}") - .build(); - - p.cargo("doc") - .with_stderr( - "\ -[..] foo v0.0.1 ([CWD]) -[..] foo v0.0.1 ([CWD]) -[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] -", - ) - .run(); - let cachedir_tag = p.root().join("target/doc/CACHEDIR.TAG"); - assert!(cachedir_tag.is_file()); - assert!(fs::read_to_string(&cachedir_tag) - .unwrap() - .starts_with("Signature: 8a477f597d28d172789f06886806bc55")); -} - -#[cargo_test] -fn target_directory_is_not_excluded_from_backups_if_it_already_exists() { - let p = project() - .file( - "Cargo.toml", - r#" - [package] - name = "foo" - version = "0.0.1" - authors = [] - build = "build.rs" - "#, - ) - .file("build.rs", "fn main() {}") - .file("src/lib.rs", "pub fn foo() {}") - .build(); - fs::create_dir_all(p.root().join("target/doc")).unwrap(); - - p.cargo("doc") - .with_stderr( - "\ -[..] foo v0.0.1 ([CWD]) -[..] foo v0.0.1 ([CWD]) -[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] -", - ) - .run(); - let cachedir_tag = p.root().join("target/doc/CACHEDIR.TAG"); - assert!(!&cachedir_tag.is_file()); -} From 092781c1fa139a1c7bf86af1ad00b3bcfc976f16 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Wed, 1 Jul 2020 22:47:38 +0200 Subject: [PATCH 12/13] Compress two tests into one --- tests/testsuite/build.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index b3bc752fea0..567a30e773e 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -4991,29 +4991,20 @@ fn reduced_reproduction_8249() { } #[cargo_test] -fn target_directory_is_excluded_from_backups() { +fn target_directory_backup_exclusion() { let p = project() .file("Cargo.toml", &basic_bin_manifest("foo")) .file("src/foo.rs", &main_file(r#""i am foo""#, &[])) .build(); + // Newly created target/ should have CACHEDIR.TAG inside... p.cargo("build").run(); let cachedir_tag = p.build_dir().join("CACHEDIR.TAG"); assert!(cachedir_tag.is_file()); assert!(fs::read_to_string(&cachedir_tag) .unwrap() .starts_with("Signature: 8a477f597d28d172789f06886806bc55")); -} - -#[cargo_test] -fn target_directory_is_not_excluded_from_backups_if_it_already_exists() { - let p = project() - .file("Cargo.toml", &basic_bin_manifest("foo")) - .file("src/foo.rs", &main_file(r#""i am foo""#, &[])) - .build(); - - let cachedir_tag = p.build_dir().join("CACHEDIR.TAG"); - p.cargo("build").run(); + // ...but if target/ already exists CACHEDIR.TAG should not be created in it. fs::remove_file(&cachedir_tag).unwrap(); p.cargo("build").run(); assert!(!&cachedir_tag.is_file()); From 5f2ba2bec81f163f8c4611e7483226035913163d Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Wed, 1 Jul 2020 23:44:14 +0200 Subject: [PATCH 13/13] Remove no longer needed cleanup code --- src/cargo/ops/cargo_clean.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index bfed10d947b..a8b39c4b5b9 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -175,9 +175,6 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { // TODO: what to do about build_script_build? let incremental = layout.incremental().join(format!("{}-*", crate_name)); rm_rf_glob(&incremental, config)?; - if let Some(uplift_dir) = uplift_dir { - rm_rf(&uplift_dir.join("CACHEDIR.TAG"), config)?; - } } } }