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

dependencies warning control, overriding path heuristic #8546

Open
ijackson opened this issue Jul 25, 2020 · 10 comments
Open

dependencies warning control, overriding path heuristic #8546

ijackson opened this issue Jul 25, 2020 · 10 comments
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@ijackson
Copy link
Contributor

Describe the problem you are trying to solve

I am working on a project with upstream dependencies, some of which I have had to edit locally. So (pending an upstream release, and sometimes even pending locally committing the upstream code) I have edited my Cargo.toml to have a path dependency on the upstream.

However, the upstream has a number of warnings from rustc.

When I compile the same crate as a non-path dependency, cargo gets the version from crates.io - which has the same warnings - but the warnings are suppressed. Evidently cargo treats the use of a path dependency as an indication that I am a developer of the dependency and therefore want to see the warnings. (I failed to find a discussion of this in the cargo documentation.)

Describe the solution you'd like

This path dependency heuristic is a good rule of thumb. Usually it will be right. But it would be good if there were a way to override it.

I suggest an additional entry in the dependency, alongside the path key. propagate_warnings maybe. The default would be false for non-path dependencies, and true for path dependencies, but it could be overridden by the depending crate.

Warnings would be shown to the user if all of the dependency links from the toplevel to the relevant place had propagate_warnings. (I haven't checked but presumably this is what cargo does already, only just checking for path dependencies.)

In my scenario this would mean that I would see the warnings if I ran cargo build in the directory of my dependency, but not in the directory of my own project. That seems right to me.

Notes

It seems that a git dependency suppresses the warnings. So I could use a git dependency instead, as a workaround. In my situation this is less than ideal, because it means I must always be sure to commit all my edits to the dependency. For another user it might well be useful to enable the warnings.

Another possibility would be some kind of global configuration to specify which crates to print warnings for. That would be independently useful but it would be less helpful in my specific situation.

@ijackson ijackson added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Jul 25, 2020
@Eh2406
Copy link
Contributor

Eh2406 commented Jul 25, 2020

Have you tried using the [replace] or [patch] sections instead of a path dep? They may work better for the use case of fixing an upstream bug.

@ehuss
Copy link
Contributor

ehuss commented Jul 25, 2020

@Eh2406 [patch] with a path dependency will also issue warnings.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 25, 2020

How many things can I get wrong in one day? :-P

Just ignore me. Sorry for the noise.

@alexcrichton
Copy link
Member

It might be reasonable to simply not warn for [patch] dependencies (or [replace]). Whenever I've used that I've never really wanted warnings and almost always get a lot to deal with anyway.

@ehuss ehuss added the A-lints Area: rustc lint configuration label Jul 29, 2020
@rtsuk
Copy link

rtsuk commented Aug 10, 2021

I'd like to take this on. Any pointers to how and where cargo decides to suppress warnings when building dependencies?

Edited to add I never actually did start on this and don't plan to now.

@ijackson
Copy link
Contributor Author

ijackson commented Aug 10, 2021

Yay, thanks for looking at this. I'm afraid I don't have any tips for navigating the inside of cargo. But I can say what the behaviour appears to be:

Basically, path dependencies are treated like the toplevel crate and have warnings enabled. Warnings are suppressed for dependencies specified as git= and for crates.io. I assume that this is done transitively, so that in principle if there's a git dependency which has path dependencies within the git tree, the warnings are suppressed for those too. crates.io doesn't allow path dependencies; IDK what happens if you specify a registry that contains things that look like path dependencies. (I assume cargo must do some laundering here or there might be a security risk.)

IDK what [patch] and [replace] do.

Overall it appears that this "you see warnings" thing is a property of a dependency edge. Currently, just from the type of the edge. @alexcrichton is suggesting that [patch] and [replace] should be treated differently. I am suggesting an ability to explicitly control the warnings-propagation-ness of a dependency edge (in the depending package, where the dependency is specified).

I hope this is of some use...

@ehuss
Copy link
Contributor

ehuss commented Aug 20, 2021

The decision to show warnings is controlled by show_warnings. However, whether or not something comes from [patch] is lost by the time it is needed, so I think adding that support will be hard. Unfortunately I don't have the time to look into it in more detail.

@ericseppanen
Copy link

I just encountered this and found this behavior surprising. Since I was doing no local patching of the dependency, the suggestions regarding [patch]/[replace] aren't helpful to me.

My naive expectation is that the transition from crates.io dependency -> remote git dependency -> local path dependency should be doable without any change to the warnings that are printed.

@mattklein123
Copy link

Just found this issue and would definitely appreciate some workaround for this, if only so that it's easier to develop a change for a dependency before submitting a PR upstream. Thank you for everyone's work on such a great system!

@weihanglo
Copy link
Member

See. #12115 (comment)

Yes, one of the future possibilities listed in the RFC is for cargo to have configurable lints.

@weihanglo weihanglo added the S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. label Jun 29, 2023
@epage epage added A-diagnostics Area: Error and warning messages generated by Cargo itself. and removed A-lints Area: rustc lint configuration labels Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

9 participants