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

Tracking Issue for -Z patch-in-config #9269

Closed
jonhoo opened this issue Mar 15, 2021 · 17 comments
Closed

Tracking Issue for -Z patch-in-config #9269

jonhoo opened this issue Mar 15, 2021 · 17 comments
Labels
C-tracking-issue Category: A tracking issue for something unstable.

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Mar 15, 2021

Summary

Original issue: #5539
Implementation: #9204
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#patch-in-config

The -Z patch-in-config flag enables the use of [patch] sections in cargo configuration files (.cargo/config.toml).

Unresolved issues

None known.

Future extensions

None known.

@jonhoo jonhoo added the C-tracking-issue Category: A tracking issue for something unstable. label Mar 15, 2021
@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 15, 2021

Documentation for feature in #9270

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 15, 2021

I'm trying to figure out the best way for me to test this feature, since I don't want to change cargo/rustc we use internally to be nightly, as it might cause developers to start adopting experimental features. I wonder, is there a way to use a cargo -Z feature on beta/stable specifically for this kind of testing? I'd like to be able to opt into just one feature rather than all of them, but "all" cargo features would probably also be fine. I'm thinking something like -Zallow-Zs :p

Alternatively, is there an "inverse" of RUST_BOOTSTRAP to disallow the use of nightly-only features on nightly? 😅

bors added a commit that referenced this issue Mar 15, 2021
Document `-Zpatch-in-config`

Tracking issue: #9269
bors added a commit that referenced this issue Mar 15, 2021
Add CLI help text for patch-in-config

Tracking issue: #9269
@alexcrichton
Copy link
Member

I believe there's an [unstable] table in .cargo/config.toml which can globally enable nightly features, and then if you use nightly Cargo you should be able to use the feature without any opt-in anywhere else

@jyn514
Copy link
Member

jyn514 commented Mar 16, 2021

Alternatively, is there an "inverse" of RUST_BOOTSTRAP to disallow the use of nightly-only features on nightly? sweat_smile

I've suggested adding RUSTC_BOOTSTRAP=0 in the pass (a little whimsically, but I think the idea is good). https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Require.20users.20to.20confirm.20they.20know.20RUSTC_.E2.80.A6.20compiler-team.23350/near/212978290

@joshtriplett
Copy link
Member

I would love to have a mechanism to prevent the use of nightly features, regardless of compiler branch. That'd be helpful, for instance, if using nightly for toolchain reasons (needing specific options or optimizations or targets), but not wanting to make code depend on nightly.

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 16, 2021

Ah, so, I specifically want to disallow all unstable features except one, since I want the ability to test a particular feature without opening the floodgates to all the others.

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 16, 2021

I started an internals discussion over in https://internals.rust-lang.org/t/mechanism-for-beta-testing-unstable-features/14280 for those interested in that line of discussion :)

@jonhoo
Copy link
Contributor Author

jonhoo commented Aug 19, 2021

I can report that I've been using this to great success in a fairly large internal developer ecosystem for a while now. What kind of evidence report would be helpful to the Cargo team to aid in making an eventual stabilization decision?

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/cargo meeting. We felt like, for this feature, the signal that you've used this extensively in a large internal developer ecosystem is enough of a proof point.

We did have one concern that we felt needs addressing before this stabilizes: it sounds like a [patch] in Cargo.toml takes precedence over one in .cargo/config.toml, whereas in most cases, .cargo/config.toml takes precedence over Cargo.toml. We'd like to be consistent there. Could you make .cargo/config.toml consistently take precedence?

Other than that, we think this can be stabilized, and we'd welcome a stabilization PR.

Separately, and not as a blocker from this: it would be nice if cargo publish ignored a variety of configuration bits that would make others unable to reproduce your build, including patch, rustflags, rustc, and similar. If you have the bandwidth, we'd really appreciate a PR implementing that. Please? 😄

@jonhoo
Copy link
Contributor Author

jonhoo commented Aug 24, 2021

Exciting! Done in #9839.

That would be a really good change, though given that I don't actually use cargo publish for said internal ecosystem, it's not clear that I'm the right person to do it 😅 In fact, in my situation (for just cargo package, which I do use), I would explicitly want to continue using the patches and other config, because the overarching build system ensures that the same configuration options are set for any consumer that wants to build.

@ehuss
Copy link
Contributor

ehuss commented Aug 25, 2021

Sorry, I don't think I expressed my concern about publishing clearly. There are a few concerns here:

  • If you have a config file with [patch] inside a binary package, and you publish it, I believe it will continue to use those patches. That would be a sort of runaround of the stability guarantees of crates.io whereby you can depend on crates from anywhere. I haven't tested that, but I believe that is what would happen. I think that is probably something we would want to prevent.

  • A lesser concern is that if you have [patch] in ~/.cargo/config, and you run cargo publish (or cargo package), the verification step will use those patches. However, the published package will not have those patches, so it may fail when other users try to use it. I'm less concerned about this, since it unlikely to happen in practice, and if it does, the author can just publish a fixed version.

@jonhoo
Copy link
Contributor Author

jonhoo commented Aug 25, 2021

Ah, I see what you mean (for 1). That one's trickier to address. Is your thinking that we should re-write any configuration files that end up in a .crate to strip out [patch] (the same way we do with Cargo.toml)? It'd be helpful if you could point me at where in the code that stripping happens for Cargo.toml, and at a mechanism for getting a list of all the configuration files that apply to the current build, so that I can iterate over them, resolve them relative to the .crate root, and then rewrite them as necessary.

@ehuss
Copy link
Contributor

ehuss commented Aug 25, 2021

EDIT: After writing the following, I realize it may not be an issue, see below...

Hm, I don't really know the answer on how to address it. First, I would verify that my assumption is correct that cargo install will use the [patch] in a published config file (it should be a relatively simple #[cargo_test]).

The rewriting of Cargo.toml is handled by prepare_for_publish.

Offhand I can think of two solutions:

  • Strip [patch] from published config files. This might be difficult because we don't have any existing mechanism for detecting config files or anything. I think that would just involve checking for .cargo/config or .cargo/config.toml in the root of the package?

  • Ignore [patch] in a published crate during cargo install. Hmm, actually just now looking at cargo install, it already avoids loading the config. Perhaps this is a non-concern?

Regardless, can you write a test just to see how cargo install behaves with a [patch]?

@jonhoo
Copy link
Contributor Author

jonhoo commented Aug 25, 2021

It looks like there is already a test to that effect:

fn install_ignores_local_cargo_config() {

Would you like one that specifically checks a config with [patch] as well?

@ehuss
Copy link
Contributor

ehuss commented Aug 25, 2021

Yea, that's probably sufficient. Thanks for checking!

bors added a commit that referenced this issue Sep 1, 2021
Stabilize patch-in-config (and prefer config over manifest)

Tracking issue: #9269

---

This stabilizes the `patch-in-config` feature ([unstable entry](https://doc.rust-lang.org/cargo/reference/unstable.html#patch-in-config)) following the discussion in #9269 (comment).

As requested, this PR _also_ changes the precedence behavior such that a `[patch]` for the same dependency in both `.cargo/config.toml` and `Cargo.toml` prefers the patch from the configuration file over the one from the manifest, which matches the behavior of other overlapping configuration options. The corresponding test has also been updated to reflect this change in behavior.
@jonhoo
Copy link
Contributor Author

jonhoo commented Nov 16, 2021

I believe this can now be closed since the feature has landed?

@ehuss
Copy link
Contributor

ehuss commented Nov 16, 2021

Indeed, thanks @jonhoo!

@ehuss ehuss closed this as completed Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for something unstable.
Projects
Status: Done
Development

No branches or pull requests

5 participants