bootstrap: handle worktrees in warn_old_master_branch #130121

lolbinarycat · 2024-09-08T22:25:01Z

rustbot · 2024-09-08T22:25:08Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Kobzol (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

lolbinarycat · 2024-09-08T22:29:34Z

r? @albertlarsan68

fixes rust-lang#130111

RalfJung · 2024-09-09T10:26:53Z

src/tools/build_helper/src/git.rs

+) -> Result<bool, Box<dyn std::error::Error>> {
+    use std::time::Duration;
+    *updated_master = updated_master_branch(config, Some(git_dir))?;
+    let branch_path = git_dir.join(".git/refs/remotes").join(&updated_master);


Isn't there a way to avoid directly looking at git's data format? Breaking the abstraction like this should ideally be avoided. E.g. you could look at the date of the last commit in that branch.

If you must use the .git folder, please use git rev-parse --git-dir to determine its location, rather than assuming that it is .git. That will then also work with worktrees.

Ah, I see you already found --git-common-dir... which is indeed the right location, --git-dir is wrong. Let me quickly fix some faulty logic I recently wrote elsewhere, then... ;)

i suppose i overestimated:

how many people use worktrees

how much people dislike incorrect warnings

waiting on a second opinion, i'll rewrite the code if that's the consensus preference.

The incorrect warning already led to incorrect diagnosis of an unrelated issue in #130144. So it's not just about dislike, it's about the real costs of an incorrect warning.

yeah, i see that pretty clearly now, there's a reason i started writing this PR as soon as i was made aware of the issue.

i know the current PR works, and my main worry is that if i need to totally rewrite it, that might introduce even more bugs (this situation has happened to me many times), and this would be even worse with something that is so tedious for me to test (spinning up worktrees is not fast on a hdd system)

the worktree bug seems pretty severe, so i think we should prioritize getting a fix merged.

i started writing this PR as soon as i was made aware of the issue.

Thanks for spinning up a fix so quickly!

RalfJung · 2024-09-09T10:29:04Z

src/tools/build_helper/src/git.rs

+    let meta = match std::fs::metadata(&branch_path) {
+        Ok(meta) => meta,
+        Err(err) => {
+            let gcd = git_common_dir(&git_dir)?;


Why use this only as a fallback instead of an error? There's a single unique way to find this path correctly for all situations, that should IMO be used consistently.

i'm trying not to have much of a performance impact over such a niche lint. i'm stuck on an hdd system for the time being, so git can take almost half a second to query basic information if it isn't in the I/O cache.. compared to a single stat syscall, which is always nearly instant.

yes, this does give slightly worse performance in the worktree case, but worktrees are already nearly unusable on a slow hdd system, so i'm not too concerned about that.

I'm concerned about maintainability of this codebase and how much we're willing to carry difficult-to-test fallback paths in code that is proven to be error-prone and not covered by CI. There's a global cost to every hack like this, and at some point making a slow system a bit slower is an okay way to keep the rest of the project productive -- the time spent figuring out why my worktrees now show this warning is time not spent working on rustc itself, after all.

In the end this will be up to the bootstrap team to decide though, these are just my thoughts.

albertlarsan68 · 2024-09-13T20:45:12Z

I am r+'ing the code because it is good and because it is a fix for a rather important issue, but I'd like to see only Git being used (isn't there a way to ask it the timestamp of the latest commit of a branch directly and compare that instead of an mtime?)
@bors r+ p=1

bors · 2024-09-13T20:45:15Z

📌 Commit 5a9b9a3 has been approved by albertlarsan68

It is now in the queue for this repository.

bors · 2024-09-13T22:02:38Z

⌛ Testing commit 5a9b9a3 with merge 507c05b...

bors · 2024-09-14T00:24:56Z

☀️ Test successful - checks-actions
Approved by: albertlarsan68
Pushing 507c05b to master...

rust-timer · 2024-09-14T01:56:08Z

Finished benchmarking commit (507c05b): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.3%, secondary 1.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Cycles

Results (secondary -8.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-8.5%	[-9.1%, -7.4%]	3
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 759.315s -> 756.859s (-0.32%)
Artifact size: 341.16 MiB -> 341.11 MiB (-0.02%)

lolbinarycat · 2024-09-14T15:30:02Z

@albertlarsan68 i was hoping that the timestamp would be updated even when a git pull does nothing, but it seems that is not the case.

…old-upstream-worktree, r=albertlarsan68" This reverts commit 507c05b, reversing changes made to 0609062.

Revert "warn_old_master_branch" check See rust-lang#131296 (comment). Reverts rust-lang#130121 and rust-lang#129584. Fixes rust-lang#131296 and rust-lang#131324.

Rollup merge of rust-lang#131331 - onur-ozkan:131296, r=Kobzol Revert "warn_old_master_branch" check See rust-lang#131296 (comment). Reverts rust-lang#130121 and rust-lang#129584. Fixes rust-lang#131296 and rust-lang#131324.

rustbot assigned Kobzol Sep 8, 2024

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 8, 2024

rustbot assigned albertlarsan68 and unassigned Kobzol Sep 8, 2024

This comment has been minimized.

Sign in to view

bootstrap: handle worktrees in warn_old_master_branch

5a9b9a3

fixes rust-lang#130111

lolbinarycat force-pushed the bootstrap-warn-old-upstream-worktree branch from 64fa312 to 5a9b9a3 Compare September 8, 2024 22:37

RalfJung reviewed Sep 9, 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 14, 2024

bors merged commit 507c05b into rust-lang:master Sep 14, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Sep 14, 2024

onur-ozkan mentioned this pull request Oct 6, 2024

Bootstrap shows unexpected warning: unable to check if origin/master is old due to error #131296

Closed

onur-ozkan added a commit to onur-ozkan/rust that referenced this pull request Oct 6, 2024

Revert "Auto merge of rust-lang#130121 - lolbinarycat:bootstrap-warn-…

dd0bcf5

…old-upstream-worktree, r=albertlarsan68" This reverts commit 507c05b, reversing changes made to 0609062.

onur-ozkan mentioned this pull request Oct 6, 2024

Revert "warn_old_master_branch" check #131331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootstrap: handle worktrees in warn_old_master_branch #130121

bootstrap: handle worktrees in warn_old_master_branch #130121

lolbinarycat commented Sep 8, 2024

rustbot commented Sep 8, 2024

lolbinarycat commented Sep 8, 2024

This comment has been minimized.

RalfJung Sep 9, 2024 •

edited

Loading

RalfJung Sep 9, 2024

lolbinarycat Sep 9, 2024

RalfJung Sep 9, 2024

lolbinarycat Sep 9, 2024 •

edited

Loading

RalfJung Sep 9, 2024

RalfJung Sep 9, 2024

lolbinarycat Sep 9, 2024

RalfJung Sep 9, 2024

albertlarsan68 commented Sep 13, 2024

bors commented Sep 13, 2024

bors commented Sep 13, 2024

bors commented Sep 14, 2024

rust-timer commented Sep 14, 2024

lolbinarycat commented Sep 14, 2024

bootstrap: handle worktrees in warn_old_master_branch #130121

bootstrap: handle worktrees in warn_old_master_branch #130121

Conversation

lolbinarycat commented Sep 8, 2024

rustbot commented Sep 8, 2024

lolbinarycat commented Sep 8, 2024

This comment has been minimized.

RalfJung Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolbinarycat Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertlarsan68 commented Sep 13, 2024

bors commented Sep 13, 2024

bors commented Sep 13, 2024

bors commented Sep 14, 2024

rust-timer commented Sep 14, 2024

Overall result: ❌✅ regressions and improvements - no action needed

lolbinarycat commented Sep 14, 2024

RalfJung Sep 9, 2024 •

edited

Loading

lolbinarycat Sep 9, 2024 •

edited

Loading