Skip to content

x.py says "patch for the non root package will be ignored" #75442

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

Closed
RalfJung opened this issue Aug 12, 2020 · 10 comments · Fixed by #75663
Closed

x.py says "patch for the non root package will be ignored" #75442

RalfJung opened this issue Aug 12, 2020 · 10 comments · Fixed by #75663
Labels
T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@RalfJung
Copy link
Member

x.py recently started saying something like this several times on almost every invocation:

warning: patch for the non root package will be ignored, specify patch at the workspace root:
package:   /home/r/src/rust/rustc/src/tools/rls/Cargo.toml
workspace: /home/r/src/rust/rustc/Cargo.toml
@RalfJung
Copy link
Member Author

Probably caused by #75427
Cc @Xanewok @calebcartwright

@jonas-schievink jonas-schievink added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Aug 12, 2020
@Mark-Simulacrum
Copy link
Member

@rust-lang/cargo, is there any way for us to make that warning an error? Ideally this wouldn't pass CI...

@mati865
Copy link
Contributor

mati865 commented Aug 12, 2020

It was done to fix CI in RLS repo and has no effect on Rust. Why should it be a hard error?

@Xanewok
Copy link
Member

Xanewok commented Aug 12, 2020

It's worth noting that functionally it doesn't change anything - the patch section in the RLS manifest should always point to the version checked in the submodule.

I'll update the RLS.

I understood that this use case might be useful to support in some way - my intent was to patch the dependency from the crate PoV to emphasize that pulling Rustfmt from git is a temporary measure and a regular crates.io dependency should be used eventually there, rather than trying to globally patch the rustfmt (which is already done by the final Rust virtual manifest).

@Mark-Simulacrum
Copy link
Member

It should be a hard error or silenceable -- we shouldn't have warnings on master.

@est31
Copy link
Member

est31 commented Aug 13, 2020

is there any way for us to make that warning an error?

AFAIK it's hardcoded. I've suggested introduction of cargo:: lints in rust-lang/cargo#8437 (comment) , to build upon rust-lang/cargo#5034 .

The largest implementation complexity I see with the cargo feature is in fact non-cargo lints because rustc prints the source of the lint level, and if cargo sets arguments, rustc would only see those and thus complain about arguments to the users.

Maybe initially the feature could only allow cargo:: lints, with rustc lints coming once it's figured out how to get rustc print the source of the lint level.

It would be great to have this feature for rust-lang/cargo#8437 as well so that one doesn't need to support #![deny(unused_crate_dependencies)] in random compilation units.

@est31
Copy link
Member

est31 commented Aug 13, 2020

@Mark-Simulacrum
Copy link
Member

@Xanewok, any news on updating RLS? I can take care of removing the patch and bumping the submodule to a separate branch on rust-lang/rls if you'd like (the patch shouldn't be needed for rust-lang/rust, but I guess is needed upstream).

@est31
Copy link
Member

est31 commented Aug 14, 2020

You don't need a separate branch. You only have to create a different Cargo.toml file and then change ci to do sth like --manifest-path Cargo.toml.ci.

@Mark-Simulacrum
Copy link
Member

I guess that would work too, but personally that seems somewhat more complicated and more likely to lead to problems of only updating one of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants