bootstrap: consider filtering `compiler` in the `git log` command for download-rustc

[jyn514]

Right now, download-rustc picks a commit to download as follows:

1. Choose the last commit authored by bors as the commit we are comparing to:

   rust/src/bootstrap/config.rs

   Lines 1921 to 1927 in 85c4ea0

   	let merge_base = output(
   	self.git()
   	.arg("rev-list")
   	.arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email))
   	.args(&["-n1", "--first-parent", "HEAD"]),
   	);
   	let commit = merge_base.trim_end();

2. Check if there are changes to compiler/ since that commit:

   rust/src/bootstrap/config.rs

   Lines 1936 to 1941 in 85c4ea0

   	// Warn if there were changes to the compiler or standard library since the ancestor commit.
   	let has_changes = !t!(self
   	.git()
   	.args(&["diff-index", "--quiet", &commit, "--", &compiler, &library])
   	.status())
   	.success();

This is slightly different than how download-ci-llvm works:

1. Choose the last commit author by bors that modified src/llvm-project:

   rust/src/bootstrap/llvm.rs

   Lines 130 to 144 in 68d458b

   	let llvm_sha = if is_git {
   	let mut rev_list = config.git();
   	rev_list.args(&[
   	PathBuf::from("rev-list"),
   	format!("--author={}", config.stage0_metadata.config.git_merge_commit_email).into(),
   	"-n1".into(),
   	"--first-parent".into(),
   	"HEAD".into(),
   	"--".into(),
   	config.src.join("src/llvm-project"),
   	config.src.join("src/bootstrap/download-ci-llvm-stamp"),
   	// the LLVM shared object file is named `LLVM-12-rust-{version}-nightly`
   	config.src.join("src/version"),
   	]);
   	output(&mut rev_list).trim().to_owned()

This results in some differences in the commit we pick. In particular, download-rustc always chooses the most recent commit authored by bors, even if that commit didn't modify compiler/:

; git log --author=bors -n1 --first-parent 2d0aa57684e10f7b3d3fe740ee18d431181583ad -c --stat --oneline
2d0aa57684e Auto merge of #112645 - Kobzol:ci-mingw-merge, r=pietroalbini

 .github/workflows/ci.yml     | 22 ++++------------------
 src/bootstrap/mk/Makefile.in | 14 ++++++--------
 src/ci/github-actions/ci.yml | 30 ++++--------------------------

So we download both more and less often than we need to: more often because we always download the latest bors commit, less often because we only consider compiler/ and library/, so if someone has modified the way bootstrap builds rustc locally, we won't notice.

In #112143 (comment) I tried this changing this approach, but it didn't actually work, because --author interacts strangely with -- compiler/ when used with rollups:

; git log -n1 --author=bors f217411bacbe943ead9dfca93a91dff0753c2a96 -c --stat --oneline
f217411bacb Auto merge of #112774 - compiler-errors:rollup-z8oof6r, r=compiler-errors

 compiler/rustc_hir_typeck/src/demand.rs                          |  7 +--
 compiler/rustc_hir_typeck/src/method/confirm.rs                  | 26 +++++++-
 compiler/rustc_hir_typeck/src/method/mod.rs                      | 21 +++++++
 compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs        | 85 ++++++++------------------
 compiler/rustc_resolve/src/imports.rs                            | 93 +++++++++++++----------------
 src/bootstrap/mk/Makefile.in                                     |  7 ++-
 src/bootstrap/test.rs                                            |  3 +-
 src/bootstrap/util.rs                                            |  2 -
 src/ci/docker/run.sh                                             |  2 -
 src/tools/build_helper/src/ci.rs                                 |  6 +-
 tests/ui/typeck/dont-record-adjustments-when-pointing-at-arg.rs  | 29 +++++++++
 .../typeck/dont-record-adjustments-when-pointing-at-arg.stderr   | 17 ++++++
 12 files changed, 167 insertions(+), 131 deletions(-)
; git log -n1 --author=bors f217411bacbe943ead9dfca93a91dff0753c2a96 -c --stat --oneline -- compiler/
939786223f2 Auto merge of #112636 - clubby789:no-capture-array-ref, r=cjgillot

 compiler/rustc_hir_typeck/src/expr_use_visitor.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

It would be nice to use the same approach for LLVM and ci-rustc, but we'd have to work around this git limitation somehow. Here is an idea i came up with that may or may not work

Details

        //   1. Find the last commit that modified compiler/: CHANGE
        //   2. Find the last commit authored by bors (if in CI, excluding the current commit): BORS
        //   3. Confirm that CHANGE is in the history of BORS

note that if we start doing this, we'll need to introduce a download-ci-rustc-stamp file (like download-ci-llvm-stamp) in case we change the way we build rustc; otherwise we might download artifacts that wouldn't match the artifacts you'd get if you build locally.

[jyn514]

note that if we start doing this, we'll need to introduce a download-ci-rustc-stamp file (like download-ci-llvm-stamp) in case we change the way we build rustc; otherwise we might download artifacts that wouldn't match the artifacts you'd get if you build locally.

i want to do this rather than include src/ci and src/bootstrap because i worry we won't get many cache hits if we count all changes as invalidating the cache. @Kobzol helpfully put together statistics for changes in the last year:

• Merge PRs that touched {compiler | library | bootstrap | ci}: 2146/2429 88.35%
• Merge PRs that touched {compiler | library}: 1978/2429 81.43%
• Merge PRs that touched {compiler | bootstrap | ci}: 1905/2429 78.43%
• Merge PRs that touched {compiler}: 1720/2429 70.81%

[jyn514]

In #112143 (comment) I tried this changing this approach, but it didn't actually work, because --author interacts strangely with -- compiler/ when used with rollups:

note that this implies that the logic we use to pick a commit for LLVM is extremely fragile and only happens to work because all llvm updates are marked rollup=never.

[Mark-Simulacrum]

In #112143 (comment) I tried this changing this approach, but it didn't actually work, because --author interacts strangely with -- compiler/ when used with rollups:

note that this implies that the logic we use to pick a commit for LLVM is extremely fragile and only happens to work because all llvm updates are marked rollup=never.

FWIW, passing --show-pulls fixes that particular case to show the f217411 commit again. The description of that looks plausible, though I'm not sure why we're skipping the auto merge without it (seems to go against documentation to my understanding).

(git log -n1 --author=bors f217411bacbe943ead9dfca93a91dff0753c2a96 -c --stat --oneline --show-pulls -- compiler/)

[Noratrieb]

Where exactly are we saving time and bandwidth by not just including all commits? Given that 70% of PRs touch compiler, basically every rebase over master (since you usually rebase over several merges) will contain changes to the compiler, causing the need for new downloads.

[jyn514]

this is a fair point! if the only outcome here is that we add --show-pulls to the llvm command i will be happy with that

[RalfJung]

This is slightly different than how download-ci-llvm works:

Choose the last commit author by bors that modified src/llvm-project:

FWIW this got changed (#113588). Now it is:

• find the merge-base of the current commit and origin/master
• then find the last commit authored by bors above that

However the new approach still sometimes fails because "commit authored by bors" could be a Miri/clippy bors commit that was moved over via a subtree sync. Really we'd want to check for "commit authored by the bors of this repo"; sadly, bors does not put that information into the commit.

(The "that modified src/llvm-project" part does not seem to be effective; at least I have seen the LLVM logic pick up Miri bors commits even though those can never modify src/llvm-project.)

[RalfJung]

@onur-ozkan you've changed this logic quite a bit recently... is this still an issue?

[onur-ozkan]

Here

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

Lines 2771 to 2784 in 4b198d6

	let mut files_to_track = vec!["compiler", "src/version", "src/stage0", "src/ci/channel"];
	// In CI, disable ci-rustc if there are changes in the library tree. But for non-CI, ignore
	// these changes to speed up the build process for library developers. This provides consistent
	// functionality for library developers between `download-rustc=true` and `download-rustc="if-unchanged"`
	// options.
	if CiEnv::is_ci() {
	files_to_track.push("library");
	}
	// Look for a version to compare to based on the current commit.
	// Only commits merged by bors will have CI artifacts.
	let commit =
	match self.last_modified_commit(&files_to_track, "download-rustc", if_unchanged) {

we only pick the most recent bors commit that modified one of files_to_track items, so the issue seems outdated today.

Thanks for the ping.