Refactor change detection for rustdoc and download-rustc by liwagu · Pull Request #131043 · rust-lang/rust

liwagu · 2024-09-29T22:19:10Z

This pull request refactors the change detection logic in the build process by consolidating redundant code into a new helper method. The key changes include the removal of duplicate logic for checking changes in directories and the addition of a new method to handle this functionality.

Refactoring and code simplification:

src/bootstrap/src/core/build_steps/tool.rs: Removed redundant change detection logic and replaced it with a call to the new check_for_changes method.
src/bootstrap/src/core/config/config.rs: Added a new method check_for_changes to centralize the logic for detecting changes in specified directories since a given commit.
src/bootstrap/src/core/config/config.rs: Updated the existing change detection code to use the new check_for_changes method.

Cleanup:

src/bootstrap/src/core/build_steps/tool.rs: Removed the unused import git from the helpers module.

r? @albertlarsan68

rustbot · 2024-09-29T22:19:19Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @albertlarsan68 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2024-09-29T22:19:20Z

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

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

rustbot · 2024-09-29T22:19:22Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

100ac12

bors · 2024-09-30T17:11:58Z

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

onur-ozkan · 2024-10-03T19:49:07Z

src/bootstrap/src/core/config/config.rs

+    /// Check for changes in specified directories since a given commit.
+    /// Returns Some(true) if changes exist, Some(false) if no changes, None if check should be ignored.
+    pub fn check_for_changes(
+        &self,
+        dirs: &[PathBuf],
+        commit: &str,
+        option_name: &str,
+        if_unchanged: bool,
+    ) -> Option<bool> {


I think this should have only one purpose: check whether there are changes or not. There is no need to add if_unchanged argument and an optional result type.

onur-ozkan · 2024-10-03T19:50:31Z

src/bootstrap/src/core/config/config.rs

+        if_unchanged: bool,
+    ) -> Option<bool> {
+        let mut git = helpers::git(Some(&self.src));
+        git.args(["diff-index", "--quiet", commit, "--"]);


You can add "--" separately with if !dirs.is_empty() check above the for dir in dirs line.

onur-ozkan · 2024-10-03T19:53:16Z

src/bootstrap/src/core/config/config.rs

+    pub fn check_for_changes(
+        &self,
+        dirs: &[PathBuf],
+        commit: &str,


How about removing this argument and moving commit finding logic into the function?

commit: &str
Is this the argument？

Yes, it is.

I'm not entirely sure if this is appropriate; the original annotations in config.rs may need to be changed.

if commit.is_empty() { println!("ERROR: could not find commit hash for downloading rustc"); println!("HELP: maybe your repository history is too shallow?"); println!("HELP: consider disabling `download-rustc`"); println!("HELP: or fetch enough history to include one upstream commit"); crate::exit!(1); }

That check is needed for any usage not only for config.rs module. If commit.is_empty() we could return an error from check_for_changes.

onur-ozkan · 2024-10-05T18:15:57Z

@rustbot author

src/bootstrap/src/core/config/config.rs

onur-ozkan

LGTM. Could you please squash your commits?

liwagu · 2024-10-07T18:49:39Z

LGTM. Could you please squash your commits?

Yes. I refer this https://rustc-dev-guide.rust-lang.org/git.html#squash-your-commits and did git rebase --interactive HEAD~5

onur-ozkan · 2024-10-08T15:07:54Z

src/bootstrap/src/core/config/config.rs

+    pub fn check_for_changes(&self, dirs: &[PathBuf], commit: &str) -> bool {
+        let mut git = helpers::git(Some(&self.src));
+        git.args(["diff-index", "--quiet", commit]);
+        if !dirs.is_empty() {
+            git.arg("--");
+            for dir in dirs {
+                git.arg(self.src.join(dir));


I missed this part in the previous reviews, sorry about that. Could you please accept the directory paths as they are and join them outside? Right now it's quite unclear and there's a high chance that some contributors might provide paths that are already joined to config.src.

Where does contributors provide the path from?

In the previous code, I only saw hardcoded: .args([self.src.join("compiler"), self.src.join("library")])

Just don't modify the given paths in check_for_changes function and from the caller side pass them like [self.src.join("compiler"), self.src.join("library")].

albertlarsan68 · 2024-10-17T06:41:53Z

Thanks for the PR!
@bors r+

bors · 2024-10-17T06:41:55Z

📌 Commit 46d98ba has been approved by albertlarsan68

It is now in the queue for this repository.

onur-ozkan · 2024-10-17T06:53:32Z

This shouldn't be merged yet (see #131043 (comment)), that's why it was tagged as "waiting-on-author".

@bors r-

src/bootstrap/src/core/config/config.rs

src/bootstrap/src/core/build_steps/tool.rs

onur-ozkan · 2024-10-23T05:00:56Z

LGTM once #131043 (comment) and #131043 (comment) are fixed and commits are squashed.

rustbot · 2024-10-23T05:48:53Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

32197a7

onur-ozkan · 2024-10-23T08:37:22Z

src/bootstrap/src/core/build_steps/tool.rs

+            let files_to_track = &["src/librustdoc", "src/tools/rustdoc"];
+
+            // Check if unchanged
+            if builder.config.last_modified_commit(files_to_track, "rustdoc", true).is_some() {


I missed this "rustdoc" value here sorry; it should be "download-rustc" instead.

why it shoud be that

This logic is gated from "download-rustc" option and there is no option such "rustdoc" in config.toml.

~~I noticed option_name appears as logs~~

Yes, and these:

rust/src/bootstrap/src/core/config/config.rs

Line 2878 in 9abfcb4

println!("help: consider disabling `{option_name}`");

rust/src/bootstrap/src/core/config/config.rs

Lines 2898 to 2901 in 9abfcb4

println!(

"warning: saw changes to one of {modified_paths:?} since {commit}; \

ignoring `{option_name}`"

);

rust/src/bootstrap/src/core/config/config.rs

Lines 2905 to 2907 in 9abfcb4

println!(

"warning: `{option_name}` is enabled, but there are changes to one of {modified_paths:?}"

);

don't make any sense with invalid option name.

This logic is gated from "download-rustc" option and there is no option such "rustdoc" in config.toml.

Ah, I haven't thought that far ahead; not thought about connecting these code changes to config.toml yet. To be honest, I'm not sure how they're linked.

but I will change it for now

onur-ozkan · 2024-10-23T12:10:04Z

Thanks!

@bors r=albertlarsan68,onur-ozkan

bors · 2024-10-23T12:10:07Z

📌 Commit 1b59216 has been approved by albertlarsan68,onur-ozkan

It is now in the queue for this repository.

…r-ozkan Refactor change detection for rustdoc and download-rustc This pull request refactors the change detection logic in the build process by consolidating redundant code into a new helper method. The key changes include the removal of duplicate logic for checking changes in directories and the addition of a new method to handle this functionality. Refactoring and code simplification: * [`src/bootstrap/src/core/build_steps/tool.rs`](diffhunk://#diff-dc86e288bcf7b3ca3f8c127d3568fbafc785704883bc7fc336bd185910aed5daL588-R593): Removed redundant change detection logic and replaced it with a call to the new `check_for_changes` method. * [`src/bootstrap/src/core/config/config.rs`](diffhunk://#diff-5f5330cfcdb0a89b85ac3547b761c3a45c2534a85c4aaae8fea88c711a7a65b2R2837-R2872): Added a new method `check_for_changes` to centralize the logic for detecting changes in specified directories since a given commit. * [`src/bootstrap/src/core/config/config.rs`](diffhunk://#diff-5f5330cfcdb0a89b85ac3547b761c3a45c2534a85c4aaae8fea88c711a7a65b2L2728-R2740): Updated the existing change detection code to use the new `check_for_changes` method. Cleanup: * [`src/bootstrap/src/core/build_steps/tool.rs`](diffhunk://#diff-dc86e288bcf7b3ca3f8c127d3568fbafc785704883bc7fc336bd185910aed5daL13-R13): Removed the unused import `git` from the helpers module. r? `@AlbertLarsan68`

Rollup of 5 pull requests Successful merges: - rust-lang#131043 (Refactor change detection for rustdoc and download-rustc) - rust-lang#131181 (Compiletest: Custom differ) - rust-lang#131487 (Add wasm32v1-none target (compiler-team/rust-lang#791)) - rust-lang#132054 (do not remove `.cargo` directory) - rust-lang#132058 (CI: rfl: use rust-next temporary commit) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#131043 - liwagu:unify, r=albertlarsan68,onur-ozkan Refactor change detection for rustdoc and download-rustc This pull request refactors the change detection logic in the build process by consolidating redundant code into a new helper method. The key changes include the removal of duplicate logic for checking changes in directories and the addition of a new method to handle this functionality. Refactoring and code simplification: * [`src/bootstrap/src/core/build_steps/tool.rs`](diffhunk://#diff-dc86e288bcf7b3ca3f8c127d3568fbafc785704883bc7fc336bd185910aed5daL588-R593): Removed redundant change detection logic and replaced it with a call to the new `check_for_changes` method. * [`src/bootstrap/src/core/config/config.rs`](diffhunk://#diff-5f5330cfcdb0a89b85ac3547b761c3a45c2534a85c4aaae8fea88c711a7a65b2R2837-R2872): Added a new method `check_for_changes` to centralize the logic for detecting changes in specified directories since a given commit. * [`src/bootstrap/src/core/config/config.rs`](diffhunk://#diff-5f5330cfcdb0a89b85ac3547b761c3a45c2534a85c4aaae8fea88c711a7a65b2L2728-R2740): Updated the existing change detection code to use the new `check_for_changes` method. Cleanup: * [`src/bootstrap/src/core/build_steps/tool.rs`](diffhunk://#diff-dc86e288bcf7b3ca3f8c127d3568fbafc785704883bc7fc336bd185910aed5daL13-R13): Removed the unused import `git` from the helpers module. r? ``@AlbertLarsan68``

rustbot assigned albertlarsan68 Sep 29, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2024

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the unify branch from 100ac12 to da6754f Compare September 29, 2024 22:33

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 29, 2024

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the unify branch from ed198f7 to 174e36f Compare October 1, 2024 05:12

onur-ozkan reviewed Oct 3, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot 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 Oct 5, 2024

liwagu commented Oct 7, 2024

View reviewed changes

src/bootstrap/src/core/config/config.rs Show resolved Hide resolved

onur-ozkan approved these changes Oct 7, 2024

View reviewed changes

onur-ozkan assigned onur-ozkan and unassigned albertlarsan68 Oct 7, 2024

rust-cloud-vms bot force-pushed the unify branch from cfc54ea to 57c439e Compare October 7, 2024 18:48

rust-cloud-vms bot force-pushed the unify branch from 57c439e to ac8a7d1 Compare October 8, 2024 13:56

onur-ozkan reviewed Oct 8, 2024

View reviewed changes

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: This is awaiting some action (such as code changes or more information) from the author. labels Oct 17, 2024

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 17, 2024

rust-cloud-vms bot force-pushed the unify branch from 46d98ba to 9133a8e Compare October 22, 2024 04:12

liwagu commented Oct 23, 2024

View reviewed changes

src/bootstrap/src/core/config/config.rs Outdated Show resolved Hide resolved

rust-cloud-vms bot force-pushed the unify branch from 9133a8e to c5cc78d Compare October 23, 2024 02:12

onur-ozkan reviewed Oct 23, 2024

View reviewed changes

src/bootstrap/src/core/build_steps/tool.rs Outdated Show resolved Hide resolved

rustbot added the has-merge-commits PR has merge commits, merge with caution. label Oct 23, 2024

rust-cloud-vms bot force-pushed the unify branch from 32197a7 to df09ae2 Compare October 23, 2024 06:14

rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Oct 23, 2024

onur-ozkan reviewed Oct 23, 2024

View reviewed changes

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 23, 2024

Refactor change detection for rustdoc and download-rustc

1b59216

rust-cloud-vms bot force-pushed the unify branch from df09ae2 to 1b59216 Compare October 23, 2024 09:30

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 23, 2024

This was referenced Oct 23, 2024

Rollup of 8 pull requests #132069

Closed

Rollup of 5 pull requests #132070

Merged

bors merged commit 4caa60c into rust-lang:master Oct 23, 2024

rustbot added this to the 1.84.0 milestone Oct 23, 2024

	println!(
	"warning: saw changes to one of {modified_paths:?} since {commit}; \
	ignoring `{option_name}`"
	);

	println!(
	"warning: `{option_name}` is enabled, but there are changes to one of {modified_paths:?}"
	);

Uh oh!

Conversation

liwagu commented Sep 29, 2024

Uh oh!

rustbot commented Sep 29, 2024

Uh oh!

rustbot commented Sep 29, 2024

Uh oh!

rustbot commented Sep 29, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

bors commented Sep 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liwagu Oct 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

onur-ozkan commented Oct 5, 2024

Uh oh!

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

liwagu commented Oct 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liwagu Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertlarsan68 commented Oct 17, 2024

Uh oh!

bors commented Oct 17, 2024

Uh oh!

onur-ozkan commented Oct 17, 2024

Uh oh!

Uh oh!

Uh oh!

onur-ozkan commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liwagu Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan Oct 3, 2024 •

edited

Loading

liwagu Oct 5, 2024 •

edited

Loading

liwagu Oct 8, 2024 •

edited

Loading

onur-ozkan Oct 8, 2024 •

edited

Loading

onur-ozkan commented Oct 23, 2024 •

edited

Loading

liwagu Oct 23, 2024 •

edited

Loading

liwagu Oct 23, 2024 •

edited

Loading