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.

Sign up for GitHub

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

Jump to bottom

fix WorkspaceSource's issues with rename and default-features #10697

Closed
wants to merge 1 commit into from
Closed

fix WorkspaceSource's issues with rename and default-features #10697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
133 changes: 113 additions & 20 deletions src/cargo/ops/cargo_add/dependency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use anyhow::bail;
use std::collections::BTreeMap;
use std::fmt::{Display, Formatter};
use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -61,8 +62,13 @@ impl Dependency {
}
}

/// Set dependency to a given version
/// Set dependency to a given source and clear any conflicting fields
pub fn set_source(mut self, source: impl Into<Source>) -> Self {
let source = source.into();
if let Source::Workspace(_) = source {
self.default_features = None;
self.rename = None;
Copy link
Contributor

Choose a reason for hiding this comment

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

This clears rename which means that toml_key will change when setting the source.

Is that what we should expect?

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 debated heavily between clearing the fields or using bail!. I decided against clearing the fields as if you want to set a new source the idea is that you do not care about those fields. I can see how a CargoResult<Self> could be better and that should be an easy change to make if you feel it is better

Copy link
Contributor

Choose a reason for hiding this comment

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

This left out another options: move rename into name so that the dependency table is as if the package field was dropped. I was wondering about that vs what is here

}
self.source = Some(source.into());
Copy link
Contributor

Choose a reason for hiding this comment

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

This .into() isn't needed

self
}
Expand Down Expand Up @@ -139,15 +145,26 @@ impl Dependency {

/// Set the value of default-features for the dependency
#[allow(dead_code)]
pub fn set_default_features(mut self, default_features: bool) -> Self {
self.default_features = Some(default_features);
self
pub fn set_default_features(mut self, default_features: bool) -> CargoResult<Self> {
if let Some(Source::Workspace(_)) = self.source {
bail!(invalid_source_for_set(
"default-features",
"WorkspaceSource"
))
} else {
self.default_features = Some(default_features);
Ok(self)
}
}

/// Set the alias for the dependency
pub fn set_rename(mut self, rename: &str) -> Self {
self.rename = Some(rename.into());
self
pub fn set_rename(mut self, rename: &str) -> CargoResult<Self> {
if let Some(Source::Workspace(_)) = self.source {
bail!(invalid_source_for_set("rename", "WorkspaceSource"))
} else {
self.rename = Some(rename.into());
Ok(self)
}
}

/// Set the value of registry for the dependency
Expand Down Expand Up @@ -486,9 +503,7 @@ impl Dependency {
if str_or_1_len_table(item) {
// Nothing to preserve
*item = self.to_toml(crate_root);
if self.source != Some(Source::Workspace(WorkspaceSource)) {
key.fmt();
}
Comment on lines -489 to -491
Copy link
Contributor

Choose a reason for hiding this comment

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

This is in the same commit as everything else without an explanation

Copy link
Member Author

Choose a reason for hiding this comment

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

There was a problem with formatting after removal and this fixed it. Do you think it would be better in a separate commit or described in the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separate PR would mean its not blocked on everything else

key.fmt();
} else if let Some(table) = item.as_table_like_mut() {
match &self.source {
Some(Source::Registry(src)) => {
Expand Down Expand Up @@ -592,9 +607,19 @@ impl Dependency {
})
.unwrap_or_default();
features.extend(new_features.iter().map(|s| s.as_str()));
let features = toml_edit::value(features.into_iter().collect::<toml_edit::Value>());
table.set_dotted(false);
table.insert("features", features);
if let Some(inherit_features) = &self.inherited_features {
inherit_features.iter().for_each(|f| {
features.remove(f.as_str());
});
}
if !features.is_empty() {
let features =
toml_edit::value(features.into_iter().collect::<toml_edit::Value>());
table.set_dotted(false);
table.insert("features", features);
} else {
table.remove("features");
}
Comment on lines +610 to +622
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite convinced this is the right route to go. inherit_features is just advisory before this change

Copy link
Member Author

Choose a reason for hiding this comment

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

This is my bad, I accidentally added this. It should've been moved to my shelf

} else {
table.remove("features");
}
Expand All @@ -619,6 +644,12 @@ fn invalid_type(dep: &str, key: &str, actual: &str, expected: &str) -> anyhow::E
anyhow::format_err!("Found {actual} for {key} when {expected} was expected for {dep}")
}

fn invalid_source_for_set(field: &str, source: &str) -> anyhow::Error {
anyhow::format_err!(
"you cannot set `{field}` when the `Source` for the `Dependency` is a `{source}`"
)
}

impl std::fmt::Display for Dependency {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
if let Some(source) = self.source() {
Expand Down Expand Up @@ -941,9 +972,24 @@ mod tests {
use std::path::Path;

use cargo_util::paths;
use toml_edit::{Document, Key};

use super::*;

#[test]
fn error_using_set_method() {
// default-features
Dependency::new("dep")
.set_source(WorkspaceSource::new())
.set_default_features(false)
.expect_err("expected error, found");
// rename
Dependency::new("dep")
.set_source(WorkspaceSource::new())
.set_default_features(false)
.expect_err("expected error, found");
}

#[test]
fn to_toml_simple_dep() {
let crate_root =
Expand Down Expand Up @@ -991,12 +1037,12 @@ mod tests {
}

#[test]
fn to_toml_dep_without_default_features() {
fn to_toml_dep_without_default_features() -> CargoResult<()> {
let crate_root =
paths::normalize_path(&std::env::current_dir().unwrap().join(Path::new("/")));
let dep = Dependency::new("dep")
.set_source(RegistrySource::new("1.0"))
.set_default_features(false);
.set_default_features(false)?;
let key = dep.toml_key();
let item = dep.to_toml(&crate_root);

Expand All @@ -1007,6 +1053,7 @@ mod tests {
assert_eq!(dep.get("default-features").unwrap().as_bool(), Some(false));

verify_roundtrip(&crate_root, key, &item);
Ok(())
}

#[test]
Expand Down Expand Up @@ -1047,12 +1094,12 @@ mod tests {
}

#[test]
fn to_toml_renamed_dep() {
fn to_toml_renamed_dep() -> CargoResult<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it take for this to show a stack trace? If the experience is sub-par wrt backtraces, I'd just unwrap instead

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'm not entirely sure what it would take, so I will change it to unwrap

let crate_root =
paths::normalize_path(&std::env::current_dir().unwrap().join(Path::new("/")));
let dep = Dependency::new("dep")
.set_source(RegistrySource::new("1.0"))
.set_rename("d");
.set_rename("d")?;
let key = dep.toml_key();
let item = dep.to_toml(&crate_root);

Expand All @@ -1063,6 +1110,7 @@ mod tests {
assert_eq!(dep.get("package").unwrap().as_str(), Some("dep"));

verify_roundtrip(&crate_root, key, &item);
Ok(())
}

#[test]
Expand All @@ -1085,13 +1133,13 @@ mod tests {
}

#[test]
fn to_toml_complex_dep() {
fn to_toml_complex_dep() -> CargoResult<()> {
let crate_root =
paths::normalize_path(&std::env::current_dir().unwrap().join(Path::new("/")));
let dep = Dependency::new("dep")
.set_source(RegistrySource::new("1.0"))
.set_default_features(false)
.set_rename("d");
.set_default_features(false)?
.set_rename("d")?;
let key = dep.toml_key();
let item = dep.to_toml(&crate_root);

Expand All @@ -1104,6 +1152,51 @@ mod tests {
assert_eq!(dep.get("default-features").unwrap().as_bool(), Some(false));

verify_roundtrip(&crate_root, key, &item);
Ok(())
}

#[test]
fn update_source_to_workspace_simple() -> CargoResult<()> {
let crate_root =
paths::normalize_path(&std::env::current_dir().unwrap().join(Path::new("/")));
let dep = Dependency::new("dep")
.set_source(RegistrySource::new("1.0"))
.set_default_features(true)?
.set_rename("d")?;
let key = dep.toml_key();
let item = dep.to_toml(&crate_root);

assert_eq!(key, "d".to_owned());
assert!(item.is_inline_table());

let dep_table = item.as_inline_table().unwrap();
assert_eq!(dep_table.get("package").unwrap().as_str(), Some("dep"));
assert_eq!(dep_table.get("version").unwrap().as_str(), Some("1.0"));
assert_eq!(
dep_table.get("default-features").unwrap().as_bool(),
Some(true)
);

let dep = dep.clone().set_source(WorkspaceSource::new());
let key = dep.toml_key();
let mut key_mut = Key::new(key);
let mut dep_item = item.clone();
dep.update_toml(&crate_root, &mut key_mut.as_mut(), &mut dep_item);

assert_eq!(key, "dep".to_owned());
assert!(str_or_1_len_table(&dep_item));
let dep_table = dep_item.as_inline_table().unwrap();
assert!(dep_table.get("package").is_none());
assert!(dep_table.get("version").is_none());
assert!(dep_table.get("default-features").is_none());
assert_eq!(dep_table.get("workspace").unwrap().as_bool(), Some(true));
assert!(dep_table.is_dotted());

let mut doc = Document::new();
doc.insert(key_mut.get(), dep_item);
assert_eq!(doc.to_string(), "dep.workspace = true\n");
verify_roundtrip(&crate_root, key, &item);
Ok(())
}

#[test]
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/ops/cargo_add/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ fn resolve_dependency(
// Checking for a workspace dependency happens first since a member could be specified
// in the workspace dependencies table as a dependency
if let Some(_dep) = find_workspace_dep(dependency.toml_key(), ws.root_manifest()).ok() {
check_invalid_ws_keys(dependency.toml_key(), arg)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason this was added? This is done in the same commit as everything else but its not clear how its related?

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 missed adding this to the PR, if this was not here it changes the error message. It prints the error message according to the toml_key which changes in the next line (potentially)

dependency = dependency.set_source(WorkspaceSource::new());
} else if let Some(package) = ws.members().find(|p| p.name().as_str() == dependency.name) {
// Only special-case workspaces when the user doesn't provide any extra
Expand Down Expand Up @@ -565,7 +566,7 @@ fn populate_dependency(mut dependency: Dependency, arg: &DepOp) -> Dependency {
}

if let Some(rename) = &arg.rename {
dependency = dependency.set_rename(rename);
dependency = dependency.set_rename(rename).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have a reason to believe an unwrap is justified, it should be documented, ideally as an except. Otherwise, chain the error

}

dependency
Expand Down