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.

Sign up for GitHub

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

Jump to bottom

Fallback to top-level config.toml if not present in current directory, and remove fallback for env vars and CLI flags #94592

Merged
merged 1 commit into from
Mar 11, 2022
Merged

Fallback to top-level config.toml if not present in current directory, and remove fallback for env vars and CLI flags #94592

merged 1 commit into from
Mar 11, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 4, 2022 •
edited
Loading

This preserves the behavior where x.py will only give a hard error on a missing config file if it was configured through --config or RUST_BOOTSTRAP_CONFIG. It also removes the top-level fallback for everything except the default path; presumably if you're passing the path explicitly, you expect it to be exactly there and don't want to look in the root directory.

Fixes #94589.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2022
@bors
Copy link
Contributor

bors commented Mar 5, 2022

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

@Mark-Simulacrum
Copy link
Member

Speaking purely about desire end-state, loading order I think I would want:

  1. --config $path
  2. RUST_BOOTSTRAP_CONFIG $path [*]
  3. $CWD/config.toml, if exists
  4. $ROOT/config.toml, if exists
  • If either (1) or (2) is set, we should not continue on if the file doesn't exist to later entries (i.e., no fallback); that is an error.
    • Where $path, if absolute, is just loaded, and otherwise is $CWD/$path.

I believe this PR is adding (4) to the list above, correct? The remaining delta from the list would then be the adjustment to relative path treatment in --config and RUST_BOOTSTRAP_CONFIG, which I am suggesting should be always relative to the current working directory, not falling back to the root (as is the case today, per #94589, if I follow correctly).

Please feel free to tell me why I shouldn't want this. I did briefly look at what other tooling does -- it seems like for example Git will not try to look in $CWD at all for configuration (per man git-config). It does seem pretty useful to look in CWD by default, though, and I think probably makes sense for our purposes.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2022
@jyn514
Copy link
Member Author

jyn514 commented Mar 10, 2022

If either (1) or (2) is set, we should not continue on if the file doesn't exist to later entries (i.e., no fallback); that is an error.

That makes sense to me, yeah. I added the fallback behavior for --config at the same time as config.toml in #71994 and didn't think super hard about whether that was a good idea. The fallback for RUST_BOOTSTRAP_CONFIG has been present since it was added in #73317; I don't think it was intentional to have a fallback there. Removing the fallback for the default happened in #73590 which likewise seems unintentional.

I'll change this to remove the fallback for everything except the default path.

@jyn514 jyn514 force-pushed the consistent-config-loading branch from fdaa1d6 to 9f7d321 Compare March 10, 2022 04:30
@jyn514
Fallback to top-level config.toml if not present in current directory
4d56f09 
This also preserves the behavior where x.py will only give a hard error on a missing config file
if it was configured through `--config` or RUST_BOOTSTRAP_CONFIG.
It also removes the top-level fallback for everything except the default path.
@jyn514 jyn514 force-pushed the consistent-config-loading branch from 9f7d321 to 4d56f09 Compare March 10, 2022 04:35
@jyn514
Copy link
Member Author

jyn514 commented Mar 10, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 10, 2022
@Mark-Simulacrum
Copy link
Member

r=me with PR description updated

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2022
@jyn514 jyn514 changed the title Fallback to top-level config.toml if not present in current directory Fallback to top-level config.toml if not present in current directory, and remove fallback for env vars and CLI flags Mar 10, 2022
@jyn514
Copy link
Member Author

jyn514 commented Mar 10, 2022

@rustbot ready

(I no longer have bors permissions.)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 10, 2022
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 11, 2022

📌 Commit 4d56f09 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 11, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2022
@bors
Auto merge of rust-lang#94845 - Dylan-DPC:rollup-3phylaq, r=Dylan-DPC
af8604f 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#93283 (Fix for localized windows editions in testcase fn read_link() Issue#93211)
 - rust-lang#94592 (Fallback to top-level config.toml if not present in current directory, and remove fallback for env vars and CLI flags)
 - rust-lang#94776 (Optimize ascii::escape_default)
 - rust-lang#94840 (update `replace_bound_vars_with_placeholders` doc comment)
 - rust-lang#94842 (Remove unnecessary try_opt for operations that cannot fail)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b711d17 into rust-lang:master Mar 11, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 11, 2022
@jyn514 jyn514 deleted the consistent-config-loading branch November 5, 2022 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.

Fallback to top-level config.toml if not present in current directory
5 participants
@jyn514 @rust-highfive @bors @Mark-Simulacrum @rustbot