Skip to content

Commit

Permalink
Auto merge of #12289 - epage:basic, r=weihanglo
Browse files Browse the repository at this point in the history
fix: Allow embedded manifests in all commands

### What does this PR try to resolve?

This is a part of #12207.

One of the goals is for embedded manifests to be a first class citizen.  If you have a script, you should be able to run tests on it, for example.

This expands the error check from just `Cargo.toml` to also single-file packages so you can use it in `--manifest-path`.

This, however, does mean that these *can* be used in places that likely won't work yet, like `cargo publish`.

### How should we test and review this PR?

By commit.  We introduce tests for basic commands and then implement and refine the support for this.

### Additional information

Other information you want to mention in this PR, such as prior arts,
future extensions, an unresolved problem, or a TODO list.
  • Loading branch information
bors committed Jun 21, 2023
2 parents 3de1cc4 + d8583f4 commit 4cebd13
Show file tree
Hide file tree
Showing 4 changed files with 491 additions and 27 deletions.
11 changes: 2 additions & 9 deletions src/bin/cargo/commands/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
ops::run(&ws, &compile_opts, &values_os(args, "args")).map_err(|err| to_run_error(config, err))
}

/// See also `util/toml/mod.rs`s `is_embedded`
pub fn is_manifest_command(arg: &str) -> bool {
let path = Path::new(arg);
1 < path.components().count()
Expand All @@ -98,15 +99,7 @@ pub fn exec_manifest_command(config: &Config, cmd: &str, args: &[OsString]) -> C
}

let manifest_path = Path::new(cmd);
let manifest_path = config.cwd().join(manifest_path);
let manifest_path = cargo_util::paths::normalize_path(&manifest_path);
if !manifest_path.exists() {
return Err(anyhow::format_err!(
"manifest path `{}` does not exist",
manifest_path.display()
)
.into());
}
let manifest_path = root_manifest(Some(manifest_path), config)?;
let mut ws = Workspace::new(&manifest_path, config)?;
if config.cli_unstable().avoid_dev_deps {
ws.set_require_optional_deps(false);
Expand Down
45 changes: 29 additions & 16 deletions src/cargo/util/command_prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use crate::CargoResult;
use anyhow::bail;
use cargo_util::paths;
use std::ffi::{OsStr, OsString};
use std::path::Path;
use std::path::PathBuf;

pub use crate::core::compiler::CompileMode;
Expand Down Expand Up @@ -339,22 +340,7 @@ pub trait ArgMatchesExt {
}

fn root_manifest(&self, config: &Config) -> CargoResult<PathBuf> {
if let Some(path) = self.value_of_path("manifest-path", config) {
// In general, we try to avoid normalizing paths in Cargo,
// but in this particular case we need it to fix #3586.
let path = paths::normalize_path(&path);
if !path.ends_with("Cargo.toml") {
anyhow::bail!("the manifest-path must be a path to a Cargo.toml file")
}
if !path.exists() {
anyhow::bail!(
"manifest path `{}` does not exist",
self._value_of("manifest-path").unwrap()
)
}
return Ok(path);
}
find_root_manifest_for_wd(config.cwd())
root_manifest(self._value_of("manifest-path").map(Path::new), config)
}

fn workspace<'a>(&self, config: &'a Config) -> CargoResult<Workspace<'a>> {
Expand Down Expand Up @@ -792,6 +778,33 @@ pub fn values_os(args: &ArgMatches, name: &str) -> Vec<OsString> {
args._values_of_os(name)
}

pub fn root_manifest(manifest_path: Option<&Path>, config: &Config) -> CargoResult<PathBuf> {
if let Some(manifest_path) = manifest_path {
let path = config.cwd().join(manifest_path);
// In general, we try to avoid normalizing paths in Cargo,
// but in this particular case we need it to fix #3586.
let path = paths::normalize_path(&path);
if !path.ends_with("Cargo.toml") && !crate::util::toml::is_embedded(&path) {
anyhow::bail!("the manifest-path must be a path to a Cargo.toml file")
}
if !path.exists() {
anyhow::bail!("manifest path `{}` does not exist", manifest_path.display())
}
if path.is_dir() {
anyhow::bail!(
"manifest path `{}` is a directory but expected a file",
manifest_path.display()
)
}
if crate::util::toml::is_embedded(&path) && !config.cli_unstable().script {
anyhow::bail!("embedded manifest `{}` requires `-Zscript`", path.display())
}
Ok(path)
} else {
find_root_manifest_for_wd(config.cwd())
}
}

#[track_caller]
pub fn ignore_unknown<T: Default>(r: Result<T, clap::parser::MatchesError>) -> T {
match r {
Expand Down
7 changes: 5 additions & 2 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,12 @@ pub fn read_manifest(
.map_err(|err| ManifestError::new(err, path.into()))
}

fn is_embedded(path: &Path) -> bool {
/// See also `bin/cargo/commands/run.rs`s `is_manifest_command`
pub fn is_embedded(path: &Path) -> bool {
let ext = path.extension();
ext.is_none() || ext == Some(OsStr::new("rs"))
ext == Some(OsStr::new("rs")) ||
// Provide better errors by not considering directories to be embedded manifests
(ext.is_none() && path.is_file())
}

/// Parse an already-loaded `Cargo.toml` as a Cargo manifest.
Expand Down
Loading

0 comments on commit 4cebd13

Please sign in to comment.