From 71073f8b7ccf33d421369e672463da77830dcef0 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 19 Mar 2018 15:40:33 +0300 Subject: [PATCH] Extract common code for dealing with path-valued arguments --- src/bin/command_prelude.rs | 29 +++++++++++++++++--- src/bin/commands/init.rs | 2 +- src/bin/commands/install.rs | 4 +-- src/bin/commands/new.rs | 2 +- src/cargo/ops/cargo_new.rs | 23 ++++++++-------- src/cargo/ops/registry.rs | 4 +-- src/cargo/util/important_paths.rs | 44 ++++--------------------------- 7 files changed, 47 insertions(+), 61 deletions(-) diff --git a/src/bin/command_prelude.rs b/src/bin/command_prelude.rs index b93d16dc2..bc738a052 100644 --- a/src/bin/command_prelude.rs +++ b/src/bin/command_prelude.rs @@ -1,10 +1,12 @@ use std::path::PathBuf; +use std::fs; use clap::{self, SubCommand}; use cargo::CargoResult; use cargo::core::Workspace; use cargo::ops::{CompileFilter, CompileMode, CompileOptions, MessageFormat, NewOptions, Packages, VersionControl}; +use cargo::util::paths; use cargo::util::important_paths::find_root_manifest_for_wd; pub use clap::{AppSettings, Arg, ArgMatches}; @@ -196,9 +198,28 @@ pub trait ArgMatchesExt { Ok(arg) } + /// Returns value of the `name` command-line argument as an absolute path + fn value_of_path(&self, name: &str, config: &Config) -> Option { + self._value_of(name).map(|path| config.cwd().join(path)) + } + fn root_manifest(&self, config: &Config) -> CargoResult { - let manifest_path = self._value_of("manifest-path"); - find_root_manifest_for_wd(manifest_path, config.cwd()) + 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") { + bail!("the manifest-path must be a path to a Cargo.toml file") + } + if !fs::metadata(&path).is_ok() { + bail!( + "manifest path `{}` does not exist", + self._value_of("manifest-path").unwrap() + ) + } + return Ok(path); + } + find_root_manifest_for_wd(config.cwd()) } fn workspace<'a>(&self, config: &'a Config) -> CargoResult> { @@ -281,7 +302,7 @@ pub trait ArgMatchesExt { Ok(compile_opts) } - fn new_options(&self) -> CargoResult { + fn new_options(&self, config: &Config) -> CargoResult { let vcs = self._value_of("vcs").map(|vcs| match vcs { "git" => VersionControl::Git, "hg" => VersionControl::Hg, @@ -294,7 +315,7 @@ pub trait ArgMatchesExt { vcs, self._is_present("bin"), self._is_present("lib"), - self._value_of("path").unwrap().to_string(), + self.value_of_path("path", config).unwrap(), self._value_of("name").map(|s| s.to_string()), ) } diff --git a/src/bin/commands/init.rs b/src/bin/commands/init.rs index 9155813ad..c32dead4d 100644 --- a/src/bin/commands/init.rs +++ b/src/bin/commands/init.rs @@ -10,7 +10,7 @@ pub fn cli() -> App { } pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult { - let opts = args.new_options()?; + let opts = args.new_options(config)?; ops::init(&opts, config)?; config .shell() diff --git a/src/bin/commands/install.rs b/src/bin/commands/install.rs index 369b52898..4c259e4d3 100644 --- a/src/bin/commands/install.rs +++ b/src/bin/commands/install.rs @@ -91,8 +91,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult { GitReference::Branch("master".to_string()) }; SourceId::for_git(&url, gitref)? - } else if let Some(path) = args.value_of("path") { - SourceId::for_path(&config.cwd().join(path))? + } else if let Some(path) = args.value_of_path("path", config) { + SourceId::for_path(&path)? } else if krates.is_empty() { SourceId::for_path(config.cwd())? } else { diff --git a/src/bin/commands/new.rs b/src/bin/commands/new.rs index 68d199222..8b8e60740 100644 --- a/src/bin/commands/new.rs +++ b/src/bin/commands/new.rs @@ -10,7 +10,7 @@ pub fn cli() -> App { } pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult { - let opts = args.new_options()?; + let opts = args.new_options(config)?; ops::new(&opts, config)?; let path = args.value_of("path").unwrap(); config diff --git a/src/cargo/ops/cargo_new.rs b/src/cargo/ops/cargo_new.rs index c433b739c..4d53ede2f 100644 --- a/src/cargo/ops/cargo_new.rs +++ b/src/cargo/ops/cargo_new.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use std::env; use std::fs; use std::fmt; -use std::path::Path; +use std::path::{Path, PathBuf}; use git2::Config as GitConfig; use git2::Repository as GitRepository; @@ -28,7 +28,8 @@ pub enum VersionControl { pub struct NewOptions { pub version_control: Option, pub kind: NewProjectKind, - pub path: String, + /// Absolute path to the directory for the new project + pub path: PathBuf, pub name: Option, } @@ -72,7 +73,7 @@ impl NewOptions { version_control: Option, bin: bool, lib: bool, - path: String, + path: PathBuf, name: Option, ) -> CargoResult { let kind = match (bin, lib) { @@ -303,17 +304,16 @@ fn plan_new_source_file(bin: bool, project_name: String) -> SourceFileInformatio } pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<()> { - let path = config.cwd().join(&opts.path); - if fs::metadata(&path).is_ok() { + let path = &opts.path; + if fs::metadata(path).is_ok() { bail!( "destination `{}` already exists\n\n\ - Use `cargo init` to initialize the directory\ - ", + Use `cargo init` to initialize the directory", path.display() ) } - let name = get_name(&path, opts)?; + let name = get_name(path, opts)?; check_name(name, opts)?; let mkopts = MkOptions { @@ -335,10 +335,9 @@ pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<()> { } pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> { - let path = config.cwd().join(&opts.path); + let path = &opts.path; - let cargotoml_path = path.join("Cargo.toml"); - if fs::metadata(&cargotoml_path).is_ok() { + if fs::metadata(&path.join("Cargo.toml")).is_ok() { bail!("`cargo init` cannot be run on existing Cargo projects") } @@ -395,7 +394,7 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> { let mkopts = MkOptions { version_control, - path: &path, + path, name, bin: src_paths_types.iter().any(|x| x.bin), source_files: src_paths_types, diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 60e4641c1..ed997ccb2 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -446,7 +446,7 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> { let name = match opts.krate { Some(ref name) => name.clone(), None => { - let manifest_path = find_root_manifest_for_wd(None, config.cwd())?; + let manifest_path = find_root_manifest_for_wd(config.cwd())?; let ws = Workspace::new(&manifest_path, config)?; ws.current()?.package_id().name().to_string() } @@ -507,7 +507,7 @@ pub fn yank( let name = match krate { Some(name) => name, None => { - let manifest_path = find_root_manifest_for_wd(None, config.cwd())?; + let manifest_path = find_root_manifest_for_wd(config.cwd())?; let ws = Workspace::new(&manifest_path, config)?; ws.current()?.package_id().name().to_string() } diff --git a/src/cargo/util/important_paths.rs b/src/cargo/util/important_paths.rs index f6d207ada..2fb4dea59 100644 --- a/src/cargo/util/important_paths.rs +++ b/src/cargo/util/important_paths.rs @@ -3,57 +3,23 @@ use std::path::{Path, PathBuf}; use util::errors::CargoResult; use util::paths; -/// Iteratively search for `file` in `pwd` and its parents, returning -/// the path of the directory. -pub fn find_project(pwd: &Path, file: &str) -> CargoResult { - find_project_manifest(pwd, file).map(|mut p| { - // remove the file, leaving just the directory - p.pop(); - p - }) -} - -/// Iteratively search for `file` in `pwd` and its parents, returning -/// the path to the file. -pub fn find_project_manifest(pwd: &Path, file: &str) -> CargoResult { - let mut current = pwd; - - loop { +/// Find the root Cargo.toml +pub fn find_root_manifest_for_wd(cwd: &Path) -> CargoResult { + let file = "Cargo.toml"; + for current in paths::ancestors(cwd) { let manifest = current.join(file); if fs::metadata(&manifest).is_ok() { return Ok(manifest); } - - match current.parent() { - Some(p) => current = p, - None => break, - } } bail!( "could not find `{}` in `{}` or any parent directory", file, - pwd.display() + cwd.display() ) } -/// Find the root Cargo.toml -pub fn find_root_manifest_for_wd(manifest_path: Option<&str>, cwd: &Path) -> CargoResult { - match manifest_path { - Some(path) => { - let absolute_path = paths::normalize_path(&cwd.join(&path)); - if !absolute_path.ends_with("Cargo.toml") { - bail!("the manifest-path must be a path to a Cargo.toml file") - } - if !fs::metadata(&absolute_path).is_ok() { - bail!("manifest path `{}` does not exist", path) - } - Ok(absolute_path) - } - None => find_project_manifest(cwd, "Cargo.toml"), - } -} - /// Return the path to the `file` in `pwd`, if it exists. pub fn find_project_manifest_exact(pwd: &Path, file: &str) -> CargoResult { let manifest = pwd.join(file); -- 2.30.2