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

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

Conversation

joshtriplett
Copy link
Member

When attempting to substitute a local version of a dependency, since the
patch section uses syntax similar to a dependency (and the
documentation even says "The syntax is similar to the [dependencies]
section", it's natural to assume that other things from [dependencies]
also work, such as features or default-features. Attempting to use
them produces this warning:

patch for crate-name-here uses the features mechanism.
default-features and features will not take effect because the patch dependency does not support this mechanism

The phrasing "the patch dependency does not support this mechanism"
makes it seem at first glance like patch just doesn't support this at
all. But the actual problem is that the user needs to move the
features/default-features keys to an entry in dependencies
instead.

Modify the message, to make this more obvious.

Modify the corresponding warning for replace as well.

Update tests accordingly.


Discovered while trying to help @benwis debug an issue.

When attempting to substitute a local version of a dependency, since the
`patch` section uses syntax similar to a dependency (and the
documentation even says "The syntax is similar to the `[dependencies]`
section", it's natural to assume that other things from `[dependencies]`
also work, such as `features` or `default-features`. Attempting to use
them produces this warning:

> patch for `crate-name-here` uses the features mechanism.
> default-features and features will not take effect because the patch dependency does not support this mechanism

The phrasing "the patch dependency does not support this mechanism"
makes it seem at first glance like `patch` just doesn't support this at
all. But the actual problem is that the user needs to move the
`features`/`default-features` keys to an entry in `dependencies`
instead.

Modify the message, to make this more obvious.

Modify the corresponding warning for `replace` as well.

Update tests accordingly.
@rustbot
Copy link
Collaborator

rustbot commented Mar 3, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 3, 2024
Comment on lines +315 to +316
"patch for `{}` attempts to declare features. \
The `features` and `default-features` keys need to appear in a `dependencies` entry, not the `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.

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");

@bors
Copy link
Contributor

bors commented Mar 20, 2024

☔ The latest upstream changes (presumably #13561) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss
Copy link
Contributor

ehuss commented Jun 25, 2024

@joshtriplett Sorry for the delay. It looks like you maybe wanted to change it to the warn()/note() suggested above? Is that something you can do?

@joshtriplett
Copy link
Member Author

joshtriplett commented Jun 26, 2024

@ehuss Yes, though I expect it'll be a little while before I get to updating it. Anyone who wants to take this one over should feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants