Skip to content

Commit 88abfa1

Browse files
authored
Validate target source paths before compilation with clearer errors (#16338)
### What does this PR try to resolve? Closes #10173 This PR adds early validation of target source paths with clearer error messages. For context, I tried to solve this issue before ( #16329 ) but ended up making an oversight, I would recommend reading [this](#10173 (comment)) to understand the issues faced in resolving this. ### So what changed in this PR? - Root `Target` `Units` are validated early on in the compilation pipeline when the `BuildContext` is generated [here](https://github.com/rust-lang/cargo/blob/e0dd406b88933e82136bbc9073e06d029db8da45/src/cargo/ops/cargo_compile/mod.rs#L177). This is right after command line arguments/manifest targets are parsed and just before the build tasks are spawned. - **Cases that are covered by this PR:** - If the target path is invalid and points to nothing. - If the target path is valid, but is a directory (not a file) - If the target path is a valid directory with a `main.rs`/`lib.rs` depending on the Target kind: In this case a `help:` message is emitted. Doing this also means that Cargo validates each root target's path that is requested to be compiled before spawning any build tasks. This PR enforces the invariant: No compilation occurs before validating every required target's source path. I'm not sure if this should be expected behaviour and I'd love to hear from others on this.
2 parents df100d3 + f203f09 commit 88abfa1

File tree

2 files changed

+554
-1
lines changed

2 files changed

+554
-1
lines changed

src/cargo/ops/cargo_compile/mod.rs

Lines changed: 121 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ use crate::util::log_message::LogMessage;
6060
use crate::util::{CargoResult, StableHasher};
6161

6262
mod compile_filter;
63-
use annotate_snippets::Level;
63+
use annotate_snippets::{Group, Level, Origin};
6464
pub use compile_filter::{CompileFilter, FilterRule, LibRule};
6565

6666
pub(super) mod unit_generator;
@@ -535,6 +535,27 @@ pub fn create_bcx<'a, 'gctx>(
535535
.extend(args);
536536
}
537537

538+
// Validate target src path for each root unit
539+
let mut error_count: usize = 0;
540+
for unit in &units {
541+
if let Some(target_src_path) = unit.target.src_path().path() {
542+
validate_target_path_as_source_file(
543+
gctx,
544+
target_src_path,
545+
unit.target.name(),
546+
unit.target.kind(),
547+
unit.pkg.manifest_path(),
548+
&mut error_count,
549+
)?
550+
}
551+
}
552+
if error_count > 0 {
553+
let plural: &str = if error_count > 1 { "s" } else { "" };
554+
anyhow::bail!(
555+
"could not compile due to {error_count} previous target resolution error{plural}"
556+
);
557+
}
558+
538559
if honor_rust_version.unwrap_or(true) {
539560
let rustc_version = target_data.rustc.version.clone().into();
540561

@@ -602,6 +623,105 @@ where `<compatible-ver>` is the latest version supporting rustc {rustc_version}"
602623
Ok(bcx)
603624
}
604625

626+
// Checks if a target path exists and is a source file, not a directory
627+
fn validate_target_path_as_source_file(
628+
gctx: &GlobalContext,
629+
target_path: &std::path::Path,
630+
target_name: &str,
631+
target_kind: &TargetKind,
632+
unit_manifest_path: &std::path::Path,
633+
error_count: &mut usize,
634+
) -> CargoResult<()> {
635+
if !target_path.exists() {
636+
*error_count += 1;
637+
638+
let err_msg = format!(
639+
"can't find {} `{}` at path `{}`",
640+
target_kind.description(),
641+
target_name,
642+
target_path.display()
643+
);
644+
645+
let group = Group::with_title(Level::ERROR.primary_title(err_msg)).element(Origin::path(
646+
unit_manifest_path.to_str().unwrap_or_default(),
647+
));
648+
649+
gctx.shell().print_report(&[group], true)?;
650+
} else if target_path.is_dir() {
651+
*error_count += 1;
652+
653+
// suggest setting the path to a likely entrypoint
654+
let main_rs = target_path.join("main.rs");
655+
let lib_rs = target_path.join("lib.rs");
656+
657+
let suggested_files_opt = match target_kind {
658+
TargetKind::Lib(_) => {
659+
if lib_rs.exists() {
660+
Some(format!("`{}`", lib_rs.display()))
661+
} else {
662+
None
663+
}
664+
}
665+
TargetKind::Bin => {
666+
if main_rs.exists() {
667+
Some(format!("`{}`", main_rs.display()))
668+
} else {
669+
None
670+
}
671+
}
672+
TargetKind::Test => {
673+
if main_rs.exists() {
674+
Some(format!("`{}`", main_rs.display()))
675+
} else {
676+
None
677+
}
678+
}
679+
TargetKind::ExampleBin => {
680+
if main_rs.exists() {
681+
Some(format!("`{}`", main_rs.display()))
682+
} else {
683+
None
684+
}
685+
}
686+
TargetKind::Bench => {
687+
if main_rs.exists() {
688+
Some(format!("`{}`", main_rs.display()))
689+
} else {
690+
None
691+
}
692+
}
693+
TargetKind::ExampleLib(_) => {
694+
if lib_rs.exists() {
695+
Some(format!("`{}`", lib_rs.display()))
696+
} else {
697+
None
698+
}
699+
}
700+
TargetKind::CustomBuild => None,
701+
};
702+
703+
let err_msg = format!(
704+
"path `{}` for {} `{}` is a directory, but a source file was expected.",
705+
target_path.display(),
706+
target_kind.description(),
707+
target_name,
708+
);
709+
let mut group = Group::with_title(Level::ERROR.primary_title(err_msg)).element(
710+
Origin::path(unit_manifest_path.to_str().unwrap_or_default()),
711+
);
712+
713+
if let Some(suggested_files) = suggested_files_opt {
714+
group = group.element(
715+
Level::HELP.message(format!("an entry point exists at {}", suggested_files)),
716+
);
717+
}
718+
719+
gctx.shell().print_report(&[group], true)?;
720+
}
721+
722+
Ok(())
723+
}
724+
605725
/// This is used to rebuild the unit graph, sharing host dependencies if possible,
606726
/// and applying other unit adjustments based on the whole graph.
607727
///

0 commit comments

Comments
 (0)