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

Clarify warning for using features or default-features in patch #13522

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Clarify warning for using features or default-features in patch #13522

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/cargo/core/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,8 @@ impl<'gctx> PackageRegistry<'gctx> {

if dep.features().len() != 0 || !dep.uses_default_features() {
self.source_config.gctx().shell().warn(format!(
"patch for `{}` uses the features mechanism. \
default-features and features will not take effect because the patch dependency does not support this mechanism",
"patch for `{}` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `patch` entry.",
Comment on lines +315 to +316
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a key piece of information is still missing: that the features will be ignored (if I understand correctly).

What do you think of:

Suggested change
"patch for `{}` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `patch` entry.",
"ignoring features in patch for `{}`. \
Patch-specific `features` and `default-features` are unsupported; instead set these in the `dependencies` entry.",

Copy link
Contributor

Choose a reason for hiding this comment

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

imo I think I'd also do this as:

shell().warn("ignoring features in patch for `{}`");
shell().note("patch-specific `features` and `default-features` are unsupported; instead set these in the `dependencies` entry");

Copy link
Member Author

Choose a reason for hiding this comment

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

@epage I'd like to put the recommendation first, if possible. What about:

Suggested change
"patch for `{}` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `patch` entry.",
"patch for `{}` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry; they will be ignored in a `patch` entry.",

Copy link
Contributor

Choose a reason for hiding this comment

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

That feels odd to me as this is a warning and should lead with what the warning is about. I see that being like rustc saying

error: consider introducing a named lifetime parameter

note: missing lifetime specifier

I do want to change my suggestion though after thinking more about why these are ignored:

shell().warn("unused fields in patch for `{}`: {}");
shell().note("configure {} in the `dependencies` entry");

Copy link
Member Author

Choose a reason for hiding this comment

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

The warning that this leads with is patch for `{}` attempts to declare features.

But 👍 for the warn+note version you just gave:

shell().warn("unused fields in patch for `{}`: {}");
shell().note("configure {} in the `dependencies` entry");

dep.package_name()
))?;
}
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/ops/resolve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@ pub fn resolve_ws_with_opts<'gctx>(
ws.gctx()
.shell()
.warn(format!(
"replacement for `{}` uses the features mechanism. \
default-features and features will not take effect because the replacement dependency does not support this mechanism",
"replacement for `{}` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `replace` entry.",
dep.package_name()
))?
}
Expand Down
16 changes: 8 additions & 8 deletions tests/testsuite/patch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,8 +883,8 @@ fn add_patch_with_features() {
p.cargo("check")
.with_stderr(
"\
[WARNING] patch for `bar` uses the features mechanism. \
default-features and features will not take effect because the patch dependency does not support this mechanism
[WARNING] patch for `bar` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `patch` entry.
[UPDATING] `dummy-registry` index
[CHECKING] bar v0.1.0 ([CWD]/bar)
[CHECKING] foo v0.0.1 ([CWD])
Expand All @@ -895,8 +895,8 @@ default-features and features will not take effect because the patch dependency
p.cargo("check")
.with_stderr(
"\
[WARNING] patch for `bar` uses the features mechanism. \
default-features and features will not take effect because the patch dependency does not support this mechanism
[WARNING] patch for `bar` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `patch` entry.
[FINISHED] [..]
",
)
Expand Down Expand Up @@ -932,8 +932,8 @@ fn add_patch_with_setting_default_features() {
p.cargo("check")
.with_stderr(
"\
[WARNING] patch for `bar` uses the features mechanism. \
default-features and features will not take effect because the patch dependency does not support this mechanism
[WARNING] patch for `bar` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `patch` entry.
[UPDATING] `dummy-registry` index
[CHECKING] bar v0.1.0 ([CWD]/bar)
[CHECKING] foo v0.0.1 ([CWD])
Expand All @@ -944,8 +944,8 @@ default-features and features will not take effect because the patch dependency
p.cargo("check")
.with_stderr(
"\
[WARNING] patch for `bar` uses the features mechanism. \
default-features and features will not take effect because the patch dependency does not support this mechanism
[WARNING] patch for `bar` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `patch` entry.
[FINISHED] [..]
",
)
Expand Down
8 changes: 4 additions & 4 deletions tests/testsuite/replace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ fn override_with_features() {
"\
[UPDATING] [..] index
[UPDATING] git repository `[..]`
[WARNING] replacement for `bar` uses the features mechanism. default-features and features \
will not take effect because the replacement dependency does not support this mechanism
[WARNING] replacement for `bar` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `replace` entry.
[CHECKING] bar v0.1.0 (file://[..])
[CHECKING] foo v0.0.1 ([CWD])
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
Expand Down Expand Up @@ -143,8 +143,8 @@ fn override_with_setting_default_features() {
"\
[UPDATING] [..] index
[UPDATING] git repository `[..]`
[WARNING] replacement for `bar` uses the features mechanism. default-features and features \
will not take effect because the replacement dependency does not support this mechanism
[WARNING] replacement for `bar` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `replace` entry.
[CHECKING] bar v0.1.0 (file://[..])
[CHECKING] foo v0.0.1 ([CWD])
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
Expand Down