Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/cargo/core/compiler/build_context/target_info.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not really on topic, but I recently felt it inconvenient that CARGO_TARGET_TMPDIR is with build-dir. I usually cd target/tmp to access test files but now I need to navigate to a shared build-dir directory.

I wonder if we should have a project private directory under artifact directory, so that we only lock that for bins

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought came from me reading this thread #16230 (comment), but placeholder method looks nice also

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bin checks are never usable, forget about my comments

Original file line number Diff line number Diff line change
Expand Up @@ -1181,7 +1181,14 @@ impl RustDocFingerprint {
.bcx
.all_kinds
.iter()
.map(|kind| build_runner.files().layout(*kind).artifact_dir().doc())
.map(|kind| {
build_runner
.files()
.layout(*kind)
.artifact_dir()
.expect("artifact-dir was not locked")
.doc()
})
.filter(|path| path.exists())
.try_for_each(|path| clean_doc(path))?;
write_fingerprint()?;
Expand Down
29 changes: 20 additions & 9 deletions src/cargo/core/compiler/build_runner/compilation_files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,11 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
// Docscrape units need to have doc/ set as the out_dir so sources for reverse-dependencies
// will be put into doc/ and not into deps/ where the *.examples files are stored.
if unit.mode.is_doc() || unit.mode.is_doc_scrape() {
self.layout(unit.kind).artifact_dir().doc().to_path_buf()
self.layout(unit.kind)
.artifact_dir()
.expect("artifact-dir was not locked")
.doc()
.to_path_buf()
} else if unit.mode.is_doc_test() {
panic!("doc tests do not have an out dir");
} else if unit.target.is_custom_build() {
Expand Down Expand Up @@ -249,8 +253,8 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
}

/// Returns the final artifact path for the host (`/…/target/debug`)
pub fn host_dest(&self) -> &Path {
self.host.artifact_dir().dest()
pub fn host_dest(&self) -> Option<&Path> {
self.host.artifact_dir().map(|v| v.dest())
}

/// Returns the root of the build output tree for the host (`/…/build-dir`)
Expand Down Expand Up @@ -283,8 +287,8 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
}

/// Directory where timing output should go.
pub fn timings_dir(&self) -> &Path {
self.host.artifact_dir().timings()
pub fn timings_dir(&self) -> Option<&Path> {
self.host.artifact_dir().map(|v| v.timings())
}

/// Returns the path for a file in the fingerprint directory.
Expand Down Expand Up @@ -378,9 +382,11 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
target: &Target,
kind: CompileKind,
bcx: &BuildContext<'_, '_>,
) -> CargoResult<PathBuf> {
) -> CargoResult<Option<PathBuf>> {
assert!(target.is_bin());
let dest = self.layout(kind).artifact_dir().dest();
let Some(dest) = self.layout(kind).artifact_dir().map(|v| v.dest()) else {
return Ok(None);
};
let info = bcx.target_data.info(kind);
let (file_types, _) = info
.rustc_outputs(
Expand All @@ -396,7 +402,7 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
.find(|file_type| file_type.flavor == FileFlavor::Normal)
.expect("target must support `bin`");

Ok(dest.join(file_type.uplift_filename(target)))
Ok(Some(dest.join(file_type.uplift_filename(target))))
}

/// Returns the filenames that the given unit will generate.
Expand Down Expand Up @@ -450,12 +456,17 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
// Examples live in their own little world.
self.layout(unit.kind)
.artifact_dir()
.expect("artifact-dir was not locked")
.examples()
.join(filename)
} else if unit.target.is_custom_build() {
self.build_script_dir(unit).join(filename)
} else {
self.layout(unit.kind).artifact_dir().dest().join(filename)
self.layout(unit.kind)
.artifact_dir()
.expect("artifact-dir was not locked")
.dest()
.join(filename)
};
if from_path == uplift_path {
// This can happen with things like examples that reside in the
Expand Down
35 changes: 29 additions & 6 deletions src/cargo/core/compiler/build_runner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::sync::{Arc, Mutex};

use crate::core::PackageId;
use crate::core::compiler::compilation::{self, UnitOutput};
use crate::core::compiler::{self, Unit, artifact};
use crate::core::compiler::{self, Unit, UserIntent, artifact};
use crate::util::cache_lock::CacheLockMode;
use crate::util::errors::CargoResult;
use annotate_snippets::{Level, Message};
Expand Down Expand Up @@ -352,11 +352,32 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
#[tracing::instrument(skip_all)]
pub fn prepare_units(&mut self) -> CargoResult<()> {
let dest = self.bcx.profiles.get_dir_name();
let host_layout = Layout::new(self.bcx.ws, None, &dest)?;
// We try to only lock the artifact-dir if we need to.
// For example, `cargo check` does not write any files to the artifact-dir so we don't need
// to lock it.
let must_take_artifact_dir_lock = match self.bcx.build_config.intent {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of intent, can we check if artifacts are produced? Do we know that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked and I think it may be possible to use some of the logic from CompilationFiles output but we would likely need to duplicate it as we need to construct Layout before CompilationFiles.

See https://github.com/rust-lang/cargo/blob/master/src/cargo/core/compiler/build_runner/compilation_files.rs#L405

Let me know if there is a better way to get this data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that can get messy. We could create an enum for the base path and calculate the full path later but unsure how well that will work out.

However, this does highlight a need for either this or conditionally grabbing the lock (#16230 (comment))

if bcx.build_config.sbom && bcx.gctx.cli_unstable().sbom {
let sbom_files: Vec<_> = outputs
.iter()
.filter(|o| matches!(o.flavor, FileFlavor::Normal | FileFlavor::Linkable))
.map(|output| OutputFile {
path: Self::append_sbom_suffix(&output.path),
hardlink: output.hardlink.as_ref().map(Self::append_sbom_suffix),
export_path: output.export_path.as_ref().map(Self::append_sbom_suffix),
flavor: FileFlavor::Sbom,
})
.collect();
outputs.extend(sbom_files.into_iter());

We would have been writing out SBOMs without holding the lock. If we don't feel SBOMs are relevant for cargo check then making the layout optional (to catch problems like this) is sufficient. If not, then we need to somehow be aware of this for locking and then that starts to look like needing to resolve this item.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: if we change SBOMs, lets do that in its own PR and make a note against rust-lang/rfcs#3553

CC @arlosi

UserIntent::Check { .. } => {
// Generally cargo check does not need to take the artifact-dir lock but there is
// one exception: If check has `--timings` we still need to lock artifact-dir since
// we will output the report files.
!self.bcx.build_config.timing_outputs.is_empty()
}
UserIntent::Build
| UserIntent::Test
| UserIntent::Doc { .. }
| UserIntent::Doctest
| UserIntent::Bench => true,
};
let host_layout = Layout::new(self.bcx.ws, None, &dest, must_take_artifact_dir_lock)?;
let mut targets = HashMap::new();
for kind in self.bcx.all_kinds.iter() {
if let CompileKind::Target(target) = *kind {
let layout = Layout::new(self.bcx.ws, Some(target), &dest)?;
let layout = Layout::new(
self.bcx.ws,
Some(target),
&dest,
must_take_artifact_dir_lock,
)?;
targets.insert(target, layout);
}
}
Expand Down Expand Up @@ -392,9 +413,11 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
let files = self.files.as_ref().unwrap();
for &kind in self.bcx.all_kinds.iter() {
let layout = files.layout(kind);
self.compilation
.root_output
.insert(kind, layout.artifact_dir().dest().to_path_buf());
if let Some(artifact_dir) = layout.artifact_dir() {
self.compilation
.root_output
.insert(kind, artifact_dir.dest().to_path_buf());
}
if self.bcx.gctx.cli_unstable().build_dir_new_layout {
for (unit, _) in self.bcx.unit_graph.iter() {
let dep_dir = self.files().deps_dir(unit);
Expand Down
12 changes: 7 additions & 5 deletions src/cargo/core/compiler/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,12 +305,14 @@ impl<'gctx> Compilation<'gctx> {
}
search_path.push(self.deps_output[&CompileKind::Host].clone());
} else {
search_path.extend(super::filter_dynamic_search_path(
self.native_dirs.iter(),
&self.root_output[&kind],
));
if let Some(path) = self.root_output.get(&kind) {
search_path.extend(super::filter_dynamic_search_path(
self.native_dirs.iter(),
path,
));
search_path.push(path.clone());
}
search_path.push(self.deps_output[&kind].clone());
search_path.push(self.root_output[&kind].clone());
// For build-std, we don't want to accidentally pull in any shared
// libs from the sysroot that ships with rustc. This may not be
// required (at least I cannot craft a situation where it
Expand Down
8 changes: 5 additions & 3 deletions src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul
let output_file = script_run_dir.join("output");
let err_file = script_run_dir.join("stderr");
let root_output_file = script_run_dir.join("root-output");
let host_target_root = build_runner.files().host_dest().to_path_buf();
let host_target_root = build_runner.files().host_dest().map(|v| v.to_path_buf());
let all = (
id,
library_name.clone(),
Expand Down Expand Up @@ -541,12 +541,14 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul
);
}
}
if let Some(build_scripts) = build_scripts {
if let Some(build_scripts) = build_scripts
&& let Some(ref host_target_root) = host_target_root
{
super::add_plugin_deps(
&mut cmd,
&build_script_outputs,
&build_scripts,
&host_target_root,
host_target_root,
)?;
}
}
Expand Down
47 changes: 29 additions & 18 deletions src/cargo/core/compiler/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ use std::path::{Path, PathBuf};
///
/// See module docs for more information.
pub struct Layout {
artifact_dir: ArtifactDirLayout,
artifact_dir: Option<ArtifactDirLayout>,
build_dir: BuildDirLayout,
}

Expand All @@ -127,6 +127,7 @@ impl Layout {
ws: &Workspace<'_>,
target: Option<CompileTarget>,
dest: &str,
must_take_artifact_dir_lock: bool,
) -> CargoResult<Layout> {
let is_new_layout = ws.gctx().cli_unstable().build_dir_new_layout;
let mut root = ws.target_dir();
Expand All @@ -150,15 +151,6 @@ impl Layout {
// 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
// compile.
let artifact_dir_lock = if is_on_nfs_mount(root.as_path_unlocked()) {
None
} else {
Some(dest.open_rw_exclusive_create(".cargo-lock", ws.gctx(), "artifact directory")?)
};

let build_dir_lock = if root == build_root || is_on_nfs_mount(build_root.as_path_unlocked())
{
None
Expand All @@ -169,21 +161,38 @@ impl Layout {
"build directory",
)?)
};
let root = root.into_path_unlocked();
let build_root = build_root.into_path_unlocked();
let dest = dest.into_path_unlocked();
let build_dest = build_dest.as_path_unlocked();
let deps = build_dest.join("deps");
let artifact = deps.join("artifact");

Ok(Layout {
artifact_dir: ArtifactDirLayout {
let artifact_dir = if must_take_artifact_dir_lock {
// 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
// compile.
let artifact_dir_lock = if is_on_nfs_mount(root.as_path_unlocked()) {
None
} else {
Some(dest.open_rw_exclusive_create(
".cargo-lock",
ws.gctx(),
"artifact directory",
)?)
};
let root = root.into_path_unlocked();
let dest = dest.into_path_unlocked();
Some(ArtifactDirLayout {
dest: dest.clone(),
examples: dest.join("examples"),
doc: root.join("doc"),
timings: root.join("cargo-timings"),
_lock: artifact_dir_lock,
},
})
} else {
None
};
Ok(Layout {
artifact_dir,
build_dir: BuildDirLayout {
root: build_root.clone(),
deps,
Expand All @@ -201,14 +210,16 @@ impl Layout {

/// Makes sure all directories stored in the Layout exist on the filesystem.
pub fn prepare(&mut self) -> CargoResult<()> {
self.artifact_dir.prepare()?;
if let Some(ref mut artifact_dir) = self.artifact_dir {
artifact_dir.prepare()?;
}
self.build_dir.prepare()?;

Ok(())
}

pub fn artifact_dir(&self) -> &ArtifactDirLayout {
&self.artifact_dir
pub fn artifact_dir(&self) -> Option<&ArtifactDirLayout> {
self.artifact_dir.as_ref()
}

pub fn build_dir(&self) -> &BuildDirLayout {
Expand Down
20 changes: 13 additions & 7 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ fn rustc(
exec.init(build_runner, unit);
let exec = exec.clone();

let root_output = build_runner.files().host_dest().to_path_buf();
let root_output = build_runner.files().host_dest().map(|v| v.to_path_buf());
let build_dir = build_runner.bcx.ws.build_dir().into_path_unlocked();
let pkg_root = unit.pkg.root().to_path_buf();
let cwd = rustc
Expand Down Expand Up @@ -361,7 +361,9 @@ fn rustc(
current_id,
mode,
)?;
add_plugin_deps(&mut rustc, &script_outputs, &build_scripts, &root_output)?;
if let Some(ref root_output) = root_output {
add_plugin_deps(&mut rustc, &script_outputs, &build_scripts, root_output)?;
}
add_custom_flags(&mut rustc, &script_outputs, script_metadatas)?;
}

Expand Down Expand Up @@ -1399,11 +1401,15 @@ fn build_base_args(
.iter()
.filter(|target| target.is_bin())
{
let exe_path = build_runner.files().bin_link_for_target(
bin_target,
unit.kind,
build_runner.bcx,
)?;
// For `cargo check` builds we do not uplift the CARGO_BIN_EXE_ artifacts to the
// artifact-dir. We do not want to provide a path to a non-existent binary but we still
// need to provide *something* so `env!("CARGO_BIN_EXE_...")` macros will compile.
let exe_path = build_runner
.files()
.bin_link_for_target(bin_target, unit.kind, build_runner.bcx)?
.map(|path| path.as_os_str().to_os_string())
.unwrap_or_else(|| OsString::from(format!("placeholder:{}", bin_target.name())));

let name = bin_target
.binary_filename()
.unwrap_or(bin_target.name().to_string());
Expand Down
5 changes: 4 additions & 1 deletion src/cargo/core/compiler/timings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,10 @@ impl<'gctx> Timings<'gctx> {
) -> CargoResult<()> {
let duration = self.start.elapsed().as_secs_f64();
let timestamp = self.start_str.replace(&['-', ':'][..], "");
let timings_path = build_runner.files().timings_dir();
let timings_path = build_runner
.files()
.timings_dir()
.expect("artifact-dir was not locked");
paths::create_dir_all(&timings_path)?;
let filename = timings_path.join(format!("cargo-timing-{}.html", timestamp));
let mut f = BufWriter::new(paths::create(&filename)?);
Expand Down
27 changes: 14 additions & 13 deletions src/cargo/ops/cargo_clean.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,15 +116,17 @@ fn clean_specs(
let target_data = RustcTargetData::new(ws, &requested_kinds)?;
let (pkg_set, resolve) = ops::resolve_ws(ws, dry_run)?;
let prof_dir_name = profiles.get_dir_name();
let host_layout = Layout::new(ws, None, &prof_dir_name)?;
let host_layout = Layout::new(ws, None, &prof_dir_name, true)?;
// Convert requested kinds to a Vec of layouts.
let target_layouts: Vec<(CompileKind, Layout)> = requested_kinds
.into_iter()
.filter_map(|kind| match kind {
CompileKind::Target(target) => match Layout::new(ws, Some(target), &prof_dir_name) {
Ok(layout) => Some(Ok((kind, layout))),
Err(e) => Some(Err(e)),
},
CompileKind::Target(target) => {
match Layout::new(ws, Some(target), &prof_dir_name, true) {
Ok(layout) => Some(Ok((kind, layout))),
Err(e) => Some(Err(e)),
}
}
CompileKind::Host => None,
})
.collect::<CargoResult<_>>()?;
Expand Down Expand Up @@ -236,19 +238,18 @@ fn clean_specs(
let (file_types, _unsupported) = target_data
.info(*compile_kind)
.rustc_outputs(mode, target.kind(), triple, clean_ctx.gctx)?;
let artifact_dir = layout
.artifact_dir()
.expect("artifact-dir was not locked during clean");
let (dir, uplift_dir) = match target.kind() {
TargetKind::ExampleBin | TargetKind::ExampleLib(..) => (
layout.build_dir().examples(),
Some(layout.artifact_dir().examples()),
),
TargetKind::ExampleBin | TargetKind::ExampleLib(..) => {
(layout.build_dir().examples(), Some(artifact_dir.examples()))
}
// Tests/benchmarks are never uplifted.
TargetKind::Test | TargetKind::Bench => {
(layout.build_dir().legacy_deps(), None)
}
_ => (
layout.build_dir().legacy_deps(),
Some(layout.artifact_dir().dest()),
),
_ => (layout.build_dir().legacy_deps(), Some(artifact_dir.dest())),
};
let mut dir_glob_str = escape_glob_path(dir)?;
let dir_glob = Path::new(&dir_glob_str);
Expand Down
Loading