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(toml)!: Disallow source-less dependencies #13775

Merged
merged 1 commit into from
Apr 18, 2024
Merged

fix(toml)!: Disallow source-less dependencies #13775

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
9 changes: 3 additions & 6 deletions src/cargo/util/toml/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1672,14 +1672,11 @@ fn detailed_dep_to_dependency<P: ResolveToPath + Clone>(
kind: Option<DepKind>,
) -> CargoResult<Dependency> {
if orig.version.is_none() && orig.path.is_none() && orig.git.is_none() {
let msg = format!(
"dependency ({}) specified without \
anyhow::bail!(
Copy link
Member

@weihanglo weihanglo Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Question: What's the criteria of turning this into

  • a normal warning
  • a critical warning that bails out later
  • a fatal error bailing out here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

turning this into a normal warning

This was already a warning, so no change there

a fatal error bailing out here

When we discussed #13629 as a team, we seemed in favor of a hard error as this was deemed a bug that people haven't been clamoring for us to continue to support.

a critical warning that bails out later

I honestly haven't touched that system much. I think those "recoverable errors", right? As in, we continue moving forward, seeing other messages, and but fail after all of that?

While I would want to move more towards error recovery, this has some weird properties

  • These only show up when emit_warnings is called which looks to only be compile commands and cargo fetch
  • The overall value of error recovery seems relatively low (not much more is really going to be shown but instead people might see other unrelated warnings)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification. I understand this being a fatal error as a team decision. The recovery error is something I am also thinking about, yet it doesn't apply to this situation.

"dependency ({name_in_toml}) specified without \
providing a local path, Git repository, version, or \
workspace dependency to use. This will be considered an \
error in future versions",
name_in_toml
workspace dependency to use"
);
manifest_ctx.warnings.push(msg);
}

if let Some(version) = &orig.version {
Expand Down
9 changes: 6 additions & 3 deletions tests/testsuite/bad_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,10 +792,13 @@ fn empty_dependencies() {
Package::new("bar", "0.0.1").publish();

p.cargo("check")
.with_stderr_contains(
.with_status(101)
.with_stderr(
"\
warning: dependency (bar) specified without providing a local path, Git repository, version, \
or workspace dependency to use. This will be considered an error in future versions
[ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
Caused by:
dependency (bar) specified without providing a local path, Git repository, version, or workspace dependency to use
",
)
.run();
Expand Down
9 changes: 0 additions & 9 deletions tests/testsuite/cargo_add/empty_dep_table/in/Cargo.toml
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion tests/testsuite/cargo_add/empty_dep_table/in/src/lib.rs
View file
Open in desktop

This file was deleted.

32 changes: 0 additions & 32 deletions tests/testsuite/cargo_add/empty_dep_table/mod.rs
View file
Open in desktop

This file was deleted.

9 changes: 0 additions & 9 deletions tests/testsuite/cargo_add/empty_dep_table/out/Cargo.toml
View file
Open in desktop

This file was deleted.

27 changes: 0 additions & 27 deletions tests/testsuite/cargo_add/empty_dep_table/stderr.term.svg
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion tests/testsuite/cargo_add/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ mod dev_build_conflict;
mod dev_prefer_existing_version;
mod dry_run;
mod empty_dep_name;
mod empty_dep_table;
mod features;
mod features_activated_over_limit;
mod features_deactivated_over_limit;
Expand Down
21 changes: 6 additions & 15 deletions tests/testsuite/inheritable_workspace_fields.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1300,14 +1300,10 @@ fn error_workspace_dependency_looked_for_workspace_itself() {
.with_status(101)
.with_stderr(
"\
[WARNING] [CWD]/Cargo.toml: unused manifest key: workspace.dependencies.dep.workspace
[WARNING] [CWD]/Cargo.toml: dependency (dep) specified without providing a local path, Git repository, version, \
or workspace dependency to use. \
This will be considered an error in future versions
[UPDATING] `dummy-registry` index
[ERROR] no matching package named `dep` found
location searched: registry `crates-io`
required by package `bar v1.2.3 ([CWD])`
[ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
Caused by:
dependency (dep) specified without providing a local path, Git repository, version, or workspace dependency to use
",
)
.run();
Expand Down Expand Up @@ -1627,15 +1623,10 @@ fn cannot_inherit_in_patch() {
.with_status(101)
.with_stderr(
"\
[WARNING] [CWD]/Cargo.toml: unused manifest key: patch.crates-io.bar.workspace
[WARNING] [CWD]/Cargo.toml: dependency (bar) specified without providing a local path, Git repository, version, \
or workspace dependency to use. \
This will be considered an error in future versions
[UPDATING] `dummy-registry` index
[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
[ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
Caused by:
patch for `bar` in `https://github.com/rust-lang/crates.io-index` points to the same source, but patches must point to different sources
dependency (bar) specified without providing a local path, Git repository, version, or workspace dependency to use
",
)
.run();
Expand Down