Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement path-bases (RFC 3529) 2/n: cargo [add|remove|update] support #14427

Merged
merged 2 commits into from
Sep 6, 2024
Merged
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
8 changes: 8 additions & 0 deletions src/bin/cargo/commands/add.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,12 @@ Example uses:
.help("Filesystem path to local crate to add")
.group("selected")
.conflicts_with("git"),
clap::Arg::new("base")
.long("base")
.action(ArgAction::Set)
.value_name("BASE")
.help("The path base to use when adding from a local crate (unstable).")
.requires("path"),
clap::Arg::new("git")
.long("git")
.action(ArgAction::Set)
Expand Down Expand Up @@ -224,6 +230,7 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {

fn parse_dependencies(gctx: &GlobalContext, matches: &ArgMatches) -> CargoResult<Vec<DepOp>> {
let path = matches.get_one::<String>("path");
let base = matches.get_one::<String>("base");
epage marked this conversation as resolved.
Show resolved Hide resolved
let git = matches.get_one::<String>("git");
let branch = matches.get_one::<String>("branch");
let rev = matches.get_one::<String>("rev");
Expand Down Expand Up @@ -329,6 +336,7 @@ fn parse_dependencies(gctx: &GlobalContext, matches: &ArgMatches) -> CargoResult
public,
registry: registry.clone(),
path: path.map(String::from),
base: base.map(String::from),
git: git.map(String::from),
branch: branch.map(String::from),
rev: rev.map(String::from),
Expand Down
57 changes: 44 additions & 13 deletions src/bin/cargo/commands/remove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,20 +167,37 @@ fn gc_workspace(workspace: &Workspace<'_>) -> CargoResult<()> {

let members = workspace
.members()
.map(|p| LocalManifest::try_new(p.manifest_path()))
.map(|p| {
Ok((
LocalManifest::try_new(p.manifest_path())?,
p.manifest().unstable_features(),
))
})
.collect::<CargoResult<Vec<_>>>()?;

let mut dependencies = members
.iter()
.flat_map(|manifest| {
manifest.get_sections().into_iter().flat_map(|(_, table)| {
table
.as_table_like()
.unwrap()
.iter()
.map(|(key, item)| Dependency::from_toml(&manifest.path, key, item))
.collect::<Vec<_>>()
})
.into_iter()
.flat_map(|(manifest, unstable_features)| {
manifest
.get_sections()
.into_iter()
.flat_map(move |(_, table)| {
table
.as_table_like()
.unwrap()
.iter()
.map(|(key, item)| {
Dependency::from_toml(
workspace.gctx(),
workspace.root(),
&manifest.path,
&unstable_features,
key,
item,
)
})
.collect::<Vec<_>>()
})
})
.collect::<CargoResult<Vec<_>>>()?;

Expand All @@ -192,7 +209,14 @@ fn gc_workspace(workspace: &Workspace<'_>) -> CargoResult<()> {
{
deps_table.set_implicit(true);
for (key, item) in deps_table.iter_mut() {
let ws_dep = Dependency::from_toml(&workspace.root(), key.get(), item)?;
let ws_dep = Dependency::from_toml(
workspace.gctx(),
workspace.root(),
&workspace.root(),
workspace.unstable_features(),
key.get(),
item,
)?;

// search for uses of this workspace dependency
let mut is_used = false;
Expand Down Expand Up @@ -329,7 +353,14 @@ fn gc_unused_patches(workspace: &Workspace<'_>, resolve: &Resolve) -> CargoResul
patch_table.set_implicit(true);

for (key, item) in patch_table.iter_mut() {
let dep = Dependency::from_toml(&workspace.root_manifest(), key.get(), item)?;
let dep = Dependency::from_toml(
workspace.gctx(),
workspace.root(),
&workspace.root_manifest(),
workspace.unstable_features(),
key.get(),
item,
)?;

// Generate a PackageIdSpec url for querying
let url = if let MaybeWorkspace::Other(source_id) =
Expand Down
85 changes: 73 additions & 12 deletions src/cargo/ops/cargo_add/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use std::str::FromStr;
use anyhow::Context as _;
use cargo_util::paths;
use cargo_util_schemas::core::PartialVersion;
use cargo_util_schemas::manifest::PathBaseName;
use cargo_util_schemas::manifest::RustVersion;
use indexmap::IndexSet;
use itertools::Itertools;
Expand All @@ -20,6 +21,7 @@ use toml_edit::Item as TomlItem;
use crate::core::dependency::DepKind;
use crate::core::registry::PackageRegistry;
use crate::core::FeatureValue;
use crate::core::Features;
use crate::core::Package;
use crate::core::Registry;
use crate::core::Shell;
Expand All @@ -28,6 +30,7 @@ use crate::core::Workspace;
use crate::sources::source::QueryKind;
use crate::util::cache_lock::CacheLockMode;
use crate::util::style;
use crate::util::toml::lookup_path_base;
use crate::util::toml_mut::dependency::Dependency;
use crate::util::toml_mut::dependency::GitSource;
use crate::util::toml_mut::dependency::MaybeWorkspace;
Expand Down Expand Up @@ -197,7 +200,13 @@ pub fn add(workspace: &Workspace<'_>, options: &AddOptions<'_>) -> CargoResult<(

print_dep_table_msg(&mut options.gctx.shell(), &dep)?;

manifest.insert_into_table(&dep_table, &dep)?;
manifest.insert_into_table(
&dep_table,
&dep,
workspace.gctx(),
workspace.root(),
options.spec.manifest().unstable_features(),
)?;
if dep.optional == Some(true) {
let is_namespaced_features_supported =
check_rust_version_for_optional_dependency(options.spec.rust_version())?;
Expand Down Expand Up @@ -270,8 +279,11 @@ pub struct DepOp {
/// Registry for looking up dependency version
pub registry: Option<String>,

/// Git repo for dependency
/// File system path for dependency
pub path: Option<String>,
/// Specify a named base for a path dependency
pub base: Option<String>,

/// Git repo for dependency
pub git: Option<String>,
/// Specify an alternative git branch
Expand Down Expand Up @@ -332,7 +344,19 @@ fn resolve_dependency(
selected
} else if let Some(raw_path) = &arg.path {
let path = paths::normalize_path(&std::env::current_dir()?.join(raw_path));
let src = PathSource::new(&path);
let mut src = PathSource::new(path);
src.base = arg.base.clone();

if let Some(base) = &arg.base {
// Validate that the base is valid.
let workspace_root = || Ok(ws.root_manifest().parent().unwrap());
lookup_path_base(
&PathBaseName::new(base.clone())?,
&gctx,
&workspace_root,
spec.manifest().unstable_features(),
)?;
}

let selected = if let Some(crate_spec) = &crate_spec {
if let Some(v) = crate_spec.version_req() {
Expand All @@ -349,9 +373,13 @@ fn resolve_dependency(
}
selected
} else {
let mut source = crate::sources::PathSource::new(&path, src.source_id()?, gctx);
let mut source = crate::sources::PathSource::new(&src.path, src.source_id()?, gctx);
let package = source.root_package()?;
Dependency::from(package.summary())
let mut selected = Dependency::from(package.summary());
if let Some(Source::Path(selected_src)) = &mut selected.source {
selected_src.base = src.base;
}
selected
};
selected
} else if let Some(crate_spec) = &crate_spec {
Expand All @@ -361,9 +389,16 @@ fn resolve_dependency(
};
selected_dep = populate_dependency(selected_dep, arg);

let lookup = |dep_key: &_| get_existing_dependency(manifest, dep_key, section);
let lookup = |dep_key: &_| {
get_existing_dependency(
ws,
spec.manifest().unstable_features(),
manifest,
dep_key,
section,
)
};
let old_dep = fuzzy_lookup(&mut selected_dep, lookup, gctx)?;

let mut dependency = if let Some(mut old_dep) = old_dep.clone() {
if old_dep.name != selected_dep.name {
// Assuming most existing keys are not relevant when the package changes
Expand All @@ -385,7 +420,9 @@ fn resolve_dependency(
if dependency.source().is_none() {
// Checking for a workspace dependency happens first since a member could be specified
// in the workspace dependencies table as a dependency
let lookup = |toml_key: &_| Ok(find_workspace_dep(toml_key, ws.root_manifest()).ok());
let lookup = |toml_key: &_| {
Ok(find_workspace_dep(toml_key, ws, ws.root_manifest(), ws.unstable_features()).ok())
};
if let Some(_dep) = fuzzy_lookup(&mut dependency, lookup, gctx)? {
dependency = dependency.set_source(WorkspaceSource::new());
} else if let Some(package) = ws.members().find(|p| p.name().as_str() == dependency.name) {
Expand Down Expand Up @@ -432,7 +469,12 @@ fn resolve_dependency(
let query = dependency.query(gctx)?;
let query = match query {
MaybeWorkspace::Workspace(_workspace) => {
let dep = find_workspace_dep(dependency.toml_key(), ws.root_manifest())?;
let dep = find_workspace_dep(
dependency.toml_key(),
ws,
ws.root_manifest(),
ws.unstable_features(),
)?;
if let Some(features) = dep.features.clone() {
dependency = dependency.set_inherited_features(features);
}
Expand Down Expand Up @@ -547,6 +589,8 @@ fn check_rust_version_for_optional_dependency(
/// If it doesn't exist but exists in another table, let's use that as most likely users
/// want to use the same version across all tables unless they are renaming.
fn get_existing_dependency(
ws: &Workspace<'_>,
unstable_features: &Features,
manifest: &LocalManifest,
dep_key: &str,
section: &DepTable,
Expand All @@ -561,7 +605,7 @@ fn get_existing_dependency(
}

let mut possible: Vec<_> = manifest
.get_dependency_versions(dep_key)
.get_dependency_versions(dep_key, ws, unstable_features)
.map(|(path, dep)| {
let key = if path == *section {
(Key::Existing, true)
Expand Down Expand Up @@ -776,6 +820,11 @@ fn select_package(
if let Some(reg_name) = dependency.registry.as_deref() {
dep = dep.set_registry(reg_name);
}
if let Some(Source::Path(PathSource { base, .. })) = dependency.source() {
if let Some(Source::Path(dep_src)) = &mut dep.source {
dep_src.base = base.clone();
}
}
Ok(dep)
}
_ => {
Expand Down Expand Up @@ -1107,7 +1156,12 @@ fn format_features_version_suffix(dep: &DependencyUI) -> String {
}
}

fn find_workspace_dep(toml_key: &str, root_manifest: &Path) -> CargoResult<Dependency> {
fn find_workspace_dep(
toml_key: &str,
ws: &Workspace<'_>,
root_manifest: &Path,
unstable_features: &Features,
) -> CargoResult<Dependency> {
let manifest = LocalManifest::try_new(root_manifest)?;
let manifest = manifest
.data
Expand All @@ -1127,7 +1181,14 @@ fn find_workspace_dep(toml_key: &str, root_manifest: &Path) -> CargoResult<Depen
let dep_item = dependencies
.get(toml_key)
.with_context(|| format!("could not find {toml_key} in `workspace.dependencies`"))?;
Dependency::from_toml(root_manifest.parent().unwrap(), toml_key, dep_item)
Dependency::from_toml(
ws.gctx(),
ws.root(),
root_manifest.parent().unwrap(),
unstable_features,
toml_key,
dep_item,
)
}

/// Convert a `semver::VersionReq` into a rendered `semver::Version` if all fields are fully
Expand Down
23 changes: 19 additions & 4 deletions src/cargo/ops/cargo_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,11 +398,16 @@ pub fn write_manifest_upgrades(

let mut any_file_has_changed = false;

let manifest_paths = std::iter::once(ws.root_manifest())
.chain(ws.members().map(|member| member.manifest_path()))
let items = std::iter::once((ws.root_manifest(), ws.unstable_features()))
.chain(ws.members().map(|member| {
(
member.manifest_path(),
member.manifest().unstable_features(),
)
}))
.collect::<Vec<_>>();

for manifest_path in manifest_paths {
for (manifest_path, unstable_features) in items {
trace!("updating TOML manifest at `{manifest_path:?}` with upgraded dependencies");

let crate_root = manifest_path
Expand All @@ -417,7 +422,10 @@ pub fn write_manifest_upgrades(
for (mut dep_key, dep_item) in dep_table.iter_mut() {
let dep_key_str = dep_key.get();
let dependency = crate::util::toml_mut::dependency::Dependency::from_toml(
ws.gctx(),
ws.root(),
&manifest_path,
unstable_features,
dep_key_str,
dep_item,
)?;
Expand Down Expand Up @@ -472,7 +480,14 @@ pub fn write_manifest_upgrades(
dep.source = Some(Source::Registry(source));

trace!("upgrading dependency {name}");
dep.update_toml(&crate_root, &mut dep_key, dep_item);
dep.update_toml(
ws.gctx(),
ws.root(),
&crate_root,
unstable_features,
&mut dep_key,
dep_item,
)?;
manifest_has_changed = true;
any_file_has_changed = true;
}
Expand Down
Loading