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 config-cli #7722

Closed
2 tasks done
ehuss opened this issue Dec 19, 2019 · 30 comments · Fixed by #10755
Closed
2 tasks done

Tracking issue for config-cli #7722

ehuss opened this issue Dec 19, 2019 · 30 comments · Fixed by #10755
Labels
A-config-cli C-tracking-issue disposition-merge finished-final-comment-period T-cargo to-announce

Comments

@ehuss
Copy link
Contributor

@ehuss ehuss commented Dec 19, 2019

Implementation: #7649
Original proposal: #6699
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#config-cli

Summary

Adds --config CLI option to pass config options to any command.

Unresolved issues

  • Make TOML restricted to KEY=VALUE. Will need updates to the toml crate to make that possible. DONE: #10176
  • Make --config PATH work has a shorthand for `--config 'include="path"' DONE: #7901
@ehuss ehuss added C-tracking-issue A-config-cli labels Dec 19, 2019
@ehuss ehuss added this to Unstable, baking in Roadmap Dec 30, 2019
bors added a commit that referenced this issue Feb 18, 2020
Support `--config path_to_config.toml` cli syntax.

Updates the `--config` flag so that if the argument appears to be a file, it will load the file.

cc #7722, #7723
@jyn514
Copy link
Member

@jyn514 jyn514 commented Sep 19, 2020

A use case for this: docs.rs wants to pass RUSTFLAGS without having the spacing interpreted by the shell: rust-lang/docs.rs#1057

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Dec 1, 2020

Is it not possible to specify registry sources via this? I wanted to migrate from using registry-index sources in Publishing crates to IPFS so that I could test it with #8890, but trying to use this flag to define the registries it doesn't seem to be picked up:

> cargo -Zunstable-options '--config=source.bs58_cli_0_1_0_anyhow.registry="http://127.0.0.1:8080/ifps/QmP6KmpjQrYpYyeXGqAvmrw45dPoKvmKeTqTanmSqJGJKz"' '--config=source.bs58_cli_0_1_0_bs58.registry="http://127.0.0.1:8080/ifps/Qmb2PJ9vMALRFmQjLDQbcNHYUvcFc96XzKMwAhBMEECxzb"' '--config=source.bs58_cli_0_1_0_paw.registry="http://127.0.0.1:8080/ifps/QmNkJbrmpjJErbX9fTpSSQYb7Ky1Qnpg47QptfMcVQUwBx"' '--config=source.bs58_cli_0_1_0_structopt.registry="http://127.0.0.1:8080/ifps/Qmbh5sZbKezvYq2ESVhbnymqs7VKMTaz7yXUaoMi9tBjJp"' update --manifest-path /tmp/ipfs-registry.bs58-cli-0.1.0.yIgXKp/bs58-cli-0.1.0/Cargo.toml
error: failed to parse manifest at `/tmp/ipfs-registry.bs58-cli-0.1.0.yIgXKp/bs58-cli-0.1.0/Cargo.toml`

Caused by:
  no index found for registry: `bs58_cli_0_1_0_anyhow`

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Dec 1, 2020

🤦 ignore that, I was using the wrong config key.

@dancrossnyc
Copy link

@dancrossnyc dancrossnyc commented Jul 1, 2021

Curious if there's any updated status on this? It would be really great to get stabilized!

@ehuss
Copy link
Contributor Author

@ehuss ehuss commented Jul 1, 2021

There's no update. There's still an unresolved issue noted above that needs someone to work on it.

@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Nov 23, 2021

I'll add what I think is a pretty compelling use-case for this: passing -Zinstrument-coverage for coverage testing (which is stabilizing in rust-lang/rust#90132). It requires leaving any existing rustflags intact since things like linker flags (-Clink-args) may be required for the crate to build in the first place, which using RUSTFLAGS doesn't do (#5376) but --config='build.rustflags = ["-Zinstrument-coverage"]' does (#5376 (comment)).

For the unresolved issue, some guidance on exactly what you're imagining would be helpful @ehuss. Specifically, why do you think that restriction is necessary in the first place? Alternatively, could we just parse into a toml::Value and then check that it only contains one actual entry?

@ehuss
Copy link
Contributor Author

@ehuss ehuss commented Nov 29, 2021

The restriction is to just constrain the format so that it has a straightforward syntax. That way, someone wouldn't be able to pass in comments or [bracketed.tables] or newlines or anything like that. It already parses to a toml::Value and checks for one value.

jonhoo added a commit to jonhoo/cargo that referenced this issue Dec 6, 2021
This addresses the remaining unresolved issue for `config-cli` (rust-lang#7722).
@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Dec 6, 2021

👍
Implemented in #10176

jonhoo added a commit to jonhoo/cargo that referenced this issue Dec 6, 2021
This addresses the remaining unresolved issue for `config-cli` (rust-lang#7722).
bors added a commit that referenced this issue Jan 25, 2022
Check --config for dotted keys only

This addresses the remaining unresolved issue for `config-cli` (#7722).
@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Jan 25, 2022

Now that #10176 has been merged, I think the unchecked box can be ticked!

@michalmuskala
Copy link

@michalmuskala michalmuskala commented Mar 21, 2022

What are the plans for stabilising those features? My use case is using vendored dependencies on CI (while using "live" dependencies locally). Configuring this through an extra argument to cargo commands with --frozen --config vendor_config.toml rather than overriding the .cargo/config.toml file and using --frozen is very, very convenient and significantly less error prone.

This becomes even more powerful, when I realised cargo vendor outputs the necessary config on stdout (while logging progress on stderr), so using cargo vendor > vendor_config.toml to update the file "just works"

@ehuss
Copy link
Contributor Author

@ehuss ehuss commented Mar 21, 2022

I think @jonhoo was looking at stabilizing this. @jonhoo, have you had a chance to look at it?

@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Mar 21, 2022

@ehuss In my opinion config-cli can be stabilized as-is. I think that's also where we arrived at on Zulip. I think there are improvements we could land that are related to this one (like config-include/#7723), but I don't think those are blockers for config-cli.

@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Apr 14, 2022

@ehuss Gentle nudge on this.

@ehuss ehuss added the T-cargo label Apr 14, 2022
@ehuss
Copy link
Contributor Author

@ehuss ehuss commented Apr 14, 2022

Ah, sorry for the confusion. I was expecting you or someone to post a stabilization PR. Perhaps you were expecting an FCP first. I'll go ahead and kick that off.

@rfcbot fcp merge

@rust-lang/cargo This is a proposal to stabilize the --config CLI option. This option allows one to pass a configuration setting using TOML syntax. The documentation is here.

I'll also propose that this also include the ability to pass --config PATH to specify the path of a config file to load. That is technically part of the config-include unstable option, which I'm not proposing to stabilize right now. I think supporting --config PATH is potentially useful, and I don't have any design concerns about it. config-include on the other hand has questionable utility, and may perhaps need more consideration.

@jonhoo or whoever wants to stabilize this, the general steps for stabilizing something is outlined in https://doc.crates.io/contrib/process/unstable.html#stabilization. To support --config PATH, it will require a small change in the code. I think this cane be done by changing these two lines to instead call self.load_file(…).

@rfcbot
Copy link
Collaborator

@rfcbot rfcbot commented Apr 14, 2022

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period disposition-merge final-comment-period labels Apr 14, 2022
@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Apr 18, 2022

I think having a restricted list like that makes a lot of sense! I'll try to find some time to update with that change 👍

jonhoo added a commit to jonhoo/cargo that referenced this issue Apr 19, 2022
@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Apr 19, 2022

@ehuss Implemented that in #10580

bors added a commit that referenced this issue Apr 21, 2022
Disallow setting registry tokens with --config

As per the concern `restricted-values` in
#7722 (comment).

r? `@ehuss`
@ehuss
Copy link
Contributor Author

@ehuss ehuss commented Apr 21, 2022

@rfcbot resolve restricted-values

Thanks @jonhoo!

@rfcbot rfcbot added the final-comment-period label Apr 21, 2022
@rfcbot
Copy link
Collaborator

@rfcbot rfcbot commented Apr 21, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period label Apr 21, 2022
@rfcbot rfcbot added finished-final-comment-period to-announce and removed final-comment-period labels May 1, 2022
@rfcbot
Copy link
Collaborator

@rfcbot rfcbot commented May 1, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Jun 13, 2022

@ehuss Do you want me to send a stabilization PR for this?

@ehuss
Copy link
Contributor Author

@ehuss ehuss commented Jun 13, 2022

Yes, please!

jonhoo added a commit to jonhoo/cargo that referenced this issue Jun 13, 2022
jonhoo added a commit to jonhoo/cargo that referenced this issue Jun 13, 2022
jonhoo added a commit to jonhoo/cargo that referenced this issue Jun 13, 2022
jonhoo added a commit to jonhoo/cargo that referenced this issue Jun 13, 2022
@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Jun 13, 2022

Vroom vroom: #10755

jonhoo added a commit to jonhoo/cargo that referenced this issue Jun 15, 2022
jonhoo added a commit to jonhoo/cargo that referenced this issue Jun 15, 2022
jonhoo added a commit to jonhoo/cargo that referenced this issue Jun 17, 2022
bors added a commit that referenced this issue Jun 22, 2022
Stabilize config-cli

This stabilizes the `--config` CLI argument as per [this FCP](#7722 (comment)).

It also makes the adjustment [suggested by `@ehuss](#7722 (comment) to allow stabilizing `--config path` without _also_ stabilizing [`config-include`](https://doc.rust-lang.org/cargo/reference/unstable.html#config-include).

I took a guess that this would land in 1.63 and put that in the tombstone entry in the unstable docs, but let me know if that's likely to be wrong.

Closes #7722.

Also, I think this should probably be tagged `relnotes`.
@bors bors closed this as completed in 39c3173 Jun 22, 2022
Imberflur pushed a commit to Imberflur/cargo that referenced this issue Jun 27, 2022
Imberflur pushed a commit to Imberflur/cargo that referenced this issue Jun 27, 2022
@ehuss ehuss moved this from Unstable, baking to Done in Roadmap Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-config-cli C-tracking-issue disposition-merge finished-final-comment-period T-cargo to-announce
Projects
Roadmap
  
Done
Development

Successfully merging a pull request may close this issue.

7 participants