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

refactor merge base logic and fix x fmt #130161

Merged
merged 7 commits into from
Sep 11, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Sep 9, 2024

When remote upstream is not configured, using get_git_modified_files to find modified files fails because get_rust_lang_rust_remote can not resolve "rust-lang/rust" from the git output. The changes in this PR makes bootstrap to find the latest bors commit, treating it as the "closest upstream commit" so that the change tracker logic can use it to find the diffs.

In addition, skips formatting if there are no modified files.

Fixes #130147

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Compare to `get_git_merge_base`, this doesn't require configuring the upstream remote.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@rustbot
Copy link
Collaborator

rustbot commented Sep 9, 2024

r? @Kobzol

rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 9, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 9, 2024

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rust-log-analyzer

This comment has been minimized.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 9, 2024

Just noting that ./x fmt for me shows the same output (and generates very long strace output), and I have a classic setup with a working remote and no worktrees. So I'm not sure if this is just about (un)configured upstream.

@onur-ozkan
Copy link
Member Author

Just noting that ./x fmt for me shows the same output (and generates very long strace output), and I have a classic setup with a working remote and no worktrees. So I'm not sure if this is just about (un)configured upstream.

Does git remote -v | grep "rust-lang" print anything?

@Kobzol
Copy link
Contributor

Kobzol commented Sep 9, 2024

(venv) /pr/pe/ru/rust [test/u]$ git remote -v | grep "rust-lang"
origin  https://github.com/rust-lang/rust (fetch)
origin  https://github.com/rust-lang/rust (push)

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Sep 9, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 9, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@onur-ozkan
Copy link
Member Author

Oh, so configuring the remote just fixes this:

fmt warning: Something went wrong running git commands:
fmt warning: rust-lang/rust remote not found

so there is another problem (if there is truly no change at all, bootstrap should skip formatting files) to solve.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 9, 2024

I think that the problem is simply that when no files are modified, untracked or ignored, then the override_builder builder is empty, and thus everything gets formatted. I'll send a PR to fix.

@onur-ozkan
Copy link
Member Author

I think that the problem is simply that when no files are modified, untracked or ignored, then the override_builder builder is empty, and thus everything gets formatted. I'll send a PR to fix.

I fixed it already with e392454. It was quite simple to figure out.

@onur-ozkan onur-ozkan changed the title refactor merge base logic refactor merge base logic and x fmt Sep 9, 2024
src/tools/build_helper/src/git.rs Show resolved Hide resolved
@Kobzol
Copy link
Contributor

Kobzol commented Sep 9, 2024

You can r=me with the added comment.

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

In addition, skips formatting if there are modified files.

There's a "no" missing here, right?

@RalfJung
Copy link
Member

RalfJung commented Sep 10, 2024

The changes in this PR makes bootstrap to find the latest bors commit, treating it as the "closest upstream commit" so that the change tracker logic can use it to find the diffs.

Won't that make #101907 worse again when the local branch is a miri (or other tool) sync -- which contains local, non-upstream bors commits, from the "other bors" running in the other repo?

See #113588 and #115409 for some context of why the logic currently works the way it does.

OTOH, things are already not reliably working for these miri syncs, so... it's probably not worth preserving the current half-working state.

@onur-ozkan
Copy link
Member Author

In addition, skips formatting if there are modified files.

There's a "no" missing here, right?

Yes..

Won't that make #101907 worse again when the local branch is a miri (or other tool) sync -- which contains local, non-upstream bors commits, from the "other bors" running in the other repo?

See #113588 and #115409 for some context of why the logic currently works the way it does.

OTOH, things are already not reliably working for these miri syncs, so... it's probably not worth preserving the current half-working state.

I don't see how this could make #101907 worse, as we don't change any behavior for CI-LLVM in this PR. If the most recent bors commit isn't from upstream, then bootstrap will attempt to modify some more files in addition to the ones actually changed.

@rust-log-analyzer

This comment has been minimized.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@RalfJung
Copy link
Member

I don't see how this could make #101907 worse, as we don't change any behavior for CI-LLVM in this PR. If the most recent bors commit isn't from upstream, then bootstrap will attempt to modify some more files in addition to the ones actually changed.

Oh we are using different "upstream base commit" logic for LLVM and formatting? That seems even worse, to be honest...^^

@@ -153,10 +154,9 @@ pub fn prebuilt_llvm_config(builder: &Builder<'_>, target: TargetSelection) -> L
/// This retrieves the LLVM sha we *want* to use, according to git history.
pub(crate) fn detect_llvm_sha(config: &Config, is_git: bool) -> String {
let llvm_sha = if is_git {
helpers::get_closest_merge_base_commit(
get_closest_merge_commit(
Copy link
Member

Choose a reason for hiding this comment

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

This here makes it look like the logic for finding the LLVM commit will change. So I am confused by your statement saying that it will not change.

Copy link
Member Author

Choose a reason for hiding this comment

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

We just moved bootstrap function to build_helper crate.

@onur-ozkan
Copy link
Member Author

I don't see how this could make #101907 worse, as we don't change any behavior for CI-LLVM in this PR. If the most recent bors commit isn't from upstream, then bootstrap will attempt to modify some more files in addition to the ones actually changed.

Oh we are using different "upstream base commit" logic for LLVM and formatting? That seems even worse, to be honest...^^

Previously formatting and LLVM had different "merge base commit" logic, this PR makes formatting to use the LLVM logic.

git.current_dir(git_dir);
}

let merge_base = get_git_merge_base(config, git_dir).unwrap_or_else(|_| "HEAD".into());
Copy link
Member

Choose a reason for hiding this comment

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

Oh, so of upstream is available this will start at the upstream merge base and then find the nearest commit from there. The doc comment of this function doesn't mention that so I got confused.

Copy link
Member Author

Choose a reason for hiding this comment

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

With the "If it fails to find the upstream remote," part, it looks quite clear to me actually 🤔

/// Searches for the nearest merge commit in the repository that also exists upstream.
///
/// If it fails to find the upstream remote, it then looks for the most recent commit made
/// by the merge bot by matching the author's email address with the merge bot's email.
pub fn get_closest_merge_commit(

Copy link
Member

@RalfJung RalfJung Sep 11, 2024

Choose a reason for hiding this comment

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

That only explains what it does when it does not find the upstream remote, it says nothing about what it does when it finds the upstream remote.

The "that also exists upstream" part is the key thing that was missing for me to understand what happens.

@@ -163,7 +163,8 @@ pub fn parse_config(args: Vec<String>) -> Config {
)
.optopt("", "edition", "default Rust edition", "EDITION")
.reqopt("", "git-repository", "name of the git repository", "ORG/REPO")
.reqopt("", "nightly-branch", "name of the git branch for nightly", "BRANCH");
.reqopt("", "nightly-branch", "name of the git branch for nightly", "BRANCH")
.reqopt("", "git-merge-commit-email", "email address used for the merge commits", "EMAIL");
Copy link
Member

Choose a reason for hiding this comment

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

"the" here should be removed.

Also, this is used to find merge commits, not to create them, right? It should specifically be the bors email? That's not clear at all currently, neither here nor in the code.

@@ -107,6 +109,38 @@ pub fn get_git_merge_base(
Ok(output_result(git.arg("merge-base").arg(&updated_master).arg("HEAD"))?.trim().to_owned())
}

/// Searches for the nearest merge commit in the repository.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Searches for the nearest merge commit in the repository.
/// Searches for the nearest merge commit in the repository that also exists upstream.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am fine with adding that comment, but in rare cases it's not correct (like you mentioned at #130161 (comment)).

Copy link
Member

Choose a reason for hiding this comment

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

Even in those cases, the merge commit exists upstream. It's just not the one we wanted...

/// To work correctly, the upstream remote must be properly configured using `git remote add <name> <url>`.
/// In most cases `get_closest_merge_commit` is the function you are looking for as it doesn't require remote
/// to be configured.
fn get_git_merge_base(config: &GitConfig<'_>, git_dir: Option<&Path>) -> Result<String, String> {
Copy link
Member

Choose a reason for hiding this comment

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

maybe "git_upstream_merge_base" would be more clear?

@RalfJung
Copy link
Member

Previously formatting and LLVM had different "merge base commit" logic, this PR makes formatting to use the LLVM logic.

Previously both used get_closest_merge_base_commit, now both use get_closest_merge_commit... seems like they used the same logic before as well?

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Sep 10, 2024

Previously both used get_closest_merge_base_commit, now both use get_closest_merge_commit... seems like they used the same logic before as well?

Formatting logic uses get_git_modified_files to find changed files, which was utilizing "get_git_merge_base" before, and now it uses "get_closest_merge_commit".

@RalfJung
Copy link
Member

Oh sorry, I was looking at the wrong place. oops

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

This should be ready to land if everyone is happy with the recent changes.

@onur-ozkan onur-ozkan changed the title refactor merge base logic and x fmt refactor merge base logic and fix x fmt Sep 11, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Sep 11, 2024

Fine by me.

@RalfJung
Copy link
Member

LGTM, thanks!

@onur-ozkan
Copy link
Member Author

@bors r=Kobzol,RalfJung

@bors
Copy link
Contributor

bors commented Sep 11, 2024

📌 Commit 5f32717 has been approved by Kobzol,RalfJung

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#129260 (Don't suggest adding return type for closures with default return type)
 - rust-lang#129520 (Suggest the correct pattern syntax on usage of unit variant pattern for a struct variant)
 - rust-lang#129866 (Clarify documentation labelling and definitions for std::collections)
 - rust-lang#130123 (Report the `note` when specified in `diagnostic::on_unimplemented`)
 - rust-lang#130161 (refactor merge base logic and fix `x fmt`)
 - rust-lang#130206 (Map `WSAEDQUOT` to `ErrorKind::FilesystemQuotaExceeded`)
 - rust-lang#130207 (Map `ERROR_CANT_RESOLVE_FILENAME` to `ErrorKind::FilesystemLoop`)
 - rust-lang#130219 (Fix false positive with `missing_docs` and `#[test]`)
 - rust-lang#130221 (Make SearchPath::new public)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ff4b3d4 into rust-lang:master Sep 11, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
Rollup merge of rust-lang#130161 - onur-ozkan:fmt-changed-files, r=Kobzol,RalfJung

refactor merge base logic and fix `x fmt`

When remote upstream is not configured, using [get_git_modified_files](https://github.com/rust-lang/rust/blob/38e3a5771cefc9362976a605549f8b04d5707311/src/tools/build_helper/src/git.rs#L114) to find modified files fails because [get_rust_lang_rust_remote](https://github.com/rust-lang/rust/blob/38e3a5771cefc9362976a605549f8b04d5707311/src/tools/build_helper/src/git.rs#L46-L48) can not resolve "rust-lang/rust" from the git output. The changes in this PR makes bootstrap to find the latest bors commit, treating it as the "closest upstream commit" so that the change tracker logic can use it to find the diffs.

In addition, [skips formatting](rust-lang@e392454) if there are no modified files.

Fixes rust-lang#130147
@onur-ozkan onur-ozkan deleted the fmt-changed-files branch September 12, 2024 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x.py fmt: logic for "which files changed" is broken when there are no changes
6 participants