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

chore: Rename Config to GlobalContext #13409

Merged
merged 2 commits into from
Feb 20, 2024
Merged

Conversation

Muscraft
Copy link
Member

@Muscraft Muscraft commented Feb 6, 2024

This PR :

  • renames Config to GlobalContext
    • it also renames its references to gctx (including lifetimes)
    • This was done to reflect better what it really is at this point
  • renames (and moves) core::compiler::context::Context to core::compiler::build_runner::BuildRunner

I believe I got all references to Config removed, Everything is on GlobalContext, but I could've missed renaming a variable or lifetime somewhere. I tried to find all references to config: &GlobalContext and rename them and I think I did so successfully. I also renamed all 'cfg to 'gctx.

Note: I explicitly did not rename any files or paths as I was unsure about the best way to do this.

How to Review this PR

Take your time (and many breaks)!

I am sorry.

I am also sorry for the very brief description, looking at words and thinking about them has become hard... I need to not look at words for a while...


As a complete side note, I honestly don't know if config, Config, ctx, or Context are really words at this point.

@rustbot
Copy link
Collaborator

rustbot commented Feb 6, 2024

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@weihanglo
Copy link
Member

Bikeshedding: GlobalContext or Cargo Context which might be better and good for text search?

@ehuss
Copy link
Contributor

ehuss commented Feb 6, 2024

Another bit of bikeshedding, I'm a little concerned that BuildContext and CompileContext might be confusing since in my mind those two words are essentially synonyms of one another. I'm not sure if anyone has other ideas? Maybe Context could be BuildRunner, to emphasize that it handles the actual process of running the build?

@bors
Copy link
Contributor

bors commented Feb 7, 2024

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

@bors bors added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Feb 7, 2024
@Muscraft
Copy link
Member Author

Muscraft commented Feb 8, 2024

Bikeshedding: GlobalContext or Cargo Context which might be better and good for text search?

I am fine with GlobalContext and making the variable gctx if everyone else is.

Another bit of bikeshedding, I'm a little concerned that BuildContext and CompileContext might be confusing since in my mind those two words are essentially synonyms of one another. I'm not sure if anyone has other ideas? Maybe Context could be BuildRunner, to emphasize that it handles the actual process of running the build?

I honestly don't know enough about BuildContext or CompileContext to say what they should be named, but I did think it was odd how similar their names were. I am happy to name things whatever.

@weihanglo
Copy link
Member

It might be a bit odd if cargo tree requires BuildRunner but doesn't really build stuff. I am not sure though.

@ehuss
Copy link
Contributor

ehuss commented Feb 8, 2024

It might be a bit odd if cargo tree requires BuildRunner but doesn't really build stuff. I am not sure though.

Maybe I'm a little confused, I don't think cargo tree uses either of the contexts?

Or maybe my previous message wasn't clear. I was referring to the old Context, not the old Config.

@weihanglo
Copy link
Member

Thanks for the clarification, Eric. Then BuildRunner makes sense to me!

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
tests/testsuite/config.rs Outdated Show resolved Hide resolved
tests/testsuite/config.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Feb 19, 2024

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

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished the review! Thanks for the hard work 👍🏾

There might be some other places need updates, but let's get this merged asap to avoid conflicts.

crates/mdman/src/hbs.rs Outdated Show resolved Hide resolved
crates/resolver-tests/src/lib.rs Outdated Show resolved Hide resolved
crates/xtask-bump-check/src/xtask.rs Outdated Show resolved Hide resolved
src/bin/cargo/commands/clean.rs Outdated Show resolved Hide resolved
src/bin/cargo/commands/publish.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_clean.rs Outdated Show resolved Hide resolved
src/cargo/sources/git/utils.rs Outdated Show resolved Hide resolved
src/cargo/core/features.rs Outdated Show resolved Hide resolved
src/cargo/core/features.rs Outdated Show resolved Hide resolved
src/cargo/core/resolver/mod.rs Outdated Show resolved Hide resolved
@Muscraft Muscraft force-pushed the rename-config branch 4 times, most recently from 563f6fc to 42c645a Compare February 20, 2024 17:47
@Muscraft Muscraft changed the title chore: Rename Config to Context chore: Rename Config to GlobalContext Feb 20, 2024
@epage
Copy link
Contributor

epage commented Feb 20, 2024

If there is anything else, we can patch it up over time

@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2024

📌 Commit 42c645a has been approved by epage

It is now in the queue for this repository.

@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-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Feb 20, 2024
@bors
Copy link
Contributor

bors commented Feb 20, 2024

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout rename-config (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self rename-config --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging src/cargo/util/toml_mut/dependency.rs
Auto-merging src/cargo/sources/git/utils.rs
Auto-merging src/cargo/ops/registry/mod.rs
Auto-merging src/cargo/ops/fix.rs
Auto-merging src/bin/cargo/main.rs
CONFLICT (content): Merge conflict in src/bin/cargo/main.rs
Auto-merging src/bin/cargo/commands/help.rs
CONFLICT (content): Merge conflict in src/bin/cargo/commands/help.rs
Auto-merging src/bin/cargo/cli.rs
CONFLICT (content): Merge conflict in src/bin/cargo/cli.rs
Auto-merging crates/resolver-tests/src/lib.rs
Automatic merge failed; fix conflicts and then commit the result.

@bors bors added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 20, 2024
@epage
Copy link
Contributor

epage commented Feb 20, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2024

📌 Commit 118afd0 has been approved by epage

It is now in the queue for this repository.

@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-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Feb 20, 2024
@bors
Copy link
Contributor

bors commented Feb 20, 2024

⌛ Testing commit 118afd0 with merge 762c7d8...

@bors
Copy link
Contributor

bors commented Feb 20, 2024

☀️ Test successful - checks-actions
Approved by: epage
Pushing 762c7d8 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Feb 20, 2024

☀️ Test successful - checks-actions
Approved by: epage
Pushing 762c7d8 to master...

@bors bors merged commit 762c7d8 into rust-lang:master Feb 20, 2024
21 checks passed
@bors
Copy link
Contributor

bors commented Feb 20, 2024

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

bors added a commit that referenced this pull request Feb 20, 2024
Fix more redundant imports.

The latest nightly has started warning about redundant imports. This removes those warnings.

Presumably these were casualties of merging between #13464 and #13409.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2024
Update cargo

9 commits in 7b7af3077bff8d60b7f124189bc9de227d3063a9..194a60b2952bd5d12ba15dd2577a97eed7d3c587
2024-02-17 14:13:00 +0000 to 2024-02-21 01:53:45 +0000
- fix: remove unused `sysroot_host_libdir` (rust-lang/cargo#13468)
- feat: support `target.<triple>.rustdocflags` officially (rust-lang/cargo#13197)
- Fix unused imports on Windows. (rust-lang/cargo#13469)
- Fix more redundant imports. (rust-lang/cargo#13466)
- test: Remove empty snapshots (rust-lang/cargo#13465)
- chore: Rename `Config` to `GlobalContext` (rust-lang/cargo#13409)
- Fix redundant imports. (rust-lang/cargo#13464)
- feat: respect `rust-version` when generating lockfile (rust-lang/cargo#12861)
- chore(ci): bump CI tools (rust-lang/cargo#13459)

r? ghost
@rustbot rustbot added this to the 1.78.0 milestone Feb 21, 2024
@Muscraft Muscraft deleted the rename-config branch February 22, 2024 17:12
bors added a commit that referenced this pull request Mar 1, 2024
refactor: Clarify more Config -> Context

### What does this PR try to resolve?

This is a follow up to #13409 and #13486

### How should we test and review this PR?

### Additional information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants