fix checking git submodules during a commit hook #126255

RalfJung · 2024-06-11T06:48:27Z

Fixes #125954
The one thing that still puzzles me is why this only is a problem with worktrees.

rustbot · 2024-06-11T06:48:34Z

Failed to set assignee to onur: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

rustbot · 2024-06-11T06:48:36Z

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

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

RalfJung · 2024-06-11T06:48:39Z

r? @onur-ozkan

RalfJung · 2024-06-11T07:42:11Z

Hm... Any idea why that command would fail...?

onur-ozkan · 2024-06-11T12:22:35Z

To reproduce the problem, you can enter the container in interactive mode with ./src/ci/docker/run.sh x86_64-gnu-llvm-17 --dev.

Running the failed command from #126255 (comment) gives the following error message:

nimda@b536d58a7363:~$ git --git-dir /checkout/.git rev-list --author=bors@rust-lang.org -n1 --first-parent HEAD -- /checkout/src/llvm-project /checkout/src/bootstrap/download-ci-llvm-stamp /checkout/src/version
fatal: /checkout/src/llvm-project: '/checkout/src/llvm-project' is outside repository at '/checkout/obj'

As far as I understand you cannot access another repository (or submodule) while explicitly setting --git-dir.

RalfJung · 2024-06-11T12:30:24Z

That can't be quite right, git --git-dir .git rev-list -n1 --first-parent HEAD -- src/tools/cargo works just fine here.

onur-ozkan · 2024-06-11T12:43:28Z

To reproduce the problem, you can enter the container in interactive mode with ./src/ci/docker/run.sh x86_64-gnu-llvm-17 --dev.

Running the failed command from #126255 (comment) gives the following error message:
nimda@b536d58a7363:~$ git --git-dir /checkout/.git rev-list --author=bors@rust-lang.org -n1 --first-parent HEAD -- /checkout/src/llvm-project /checkout/src/bootstrap/download-ci-llvm-stamp /checkout/src/version
fatal: /checkout/src/llvm-project: '/checkout/src/llvm-project' is outside repository at '/checkout/obj'
As far as I understand you cannot access another repository (or submodule) while explicitly setting --git-dir.

fwiw changing current directory to /checkout from /checkout/obj fixed the problem on my side.

RalfJung · 2024-06-11T12:51:43Z

I don't understand why changing the current directory makes a difference when --git-dir is set... but that's exactly what my most recent commit also does.

RalfJung · 2024-06-11T12:54:08Z

I am definitely not understanding --git-dir properly. I played around with this

git --git-dir ~/src/rust/rustc/.git rev-list -n1 --first-parent HEAD -- ~/src/rust/rustc/src/tools/cargo

and depending on my working directory, this sometimes shows a commit and sometimes not. And when my working dir is a different git checkout it just fails.

So do we have to do both, change the working dir and set --git-dir?!?

RalfJung · 2024-06-11T12:55:57Z

Ah, this explains it, partially. So we definitely need the working dir, or set -C.

But then also setting --git-dir seems like a hack. Maybe I should figure out which env var is the problem and just unset that.

onur-ozkan · 2024-06-11T13:05:27Z

Maybe I should figure out which env var is the problem and just unset that.

This could be the best solution. Adding --git-dir individually on multiple places makes things more complicated than they should be.

RalfJung · 2024-06-11T15:07:19Z

I found something that works, but it still behaves a bit strange -- when I actually have an intended submodule change and git add && git commit that, then the x.py invocation in the commit hook will first check out the old cargo and then, when it runs a second time, it checks out the commit I set it to...

Maybe we should just skip all submodule processing when GIT_DIR is set? That happens during a git invocation and doing advanced git surgery in the middle of that might just not be a good idea.

onur-ozkan · 2024-06-11T16:36:20Z

Would setting --git-dir next to every current_dir usage solve this problem? If this works, it would be much easier to maintain as it only adds one extra argument to the git command without significantly increasing complexity.

onur-ozkan · 2024-06-11T16:39:55Z

Would setting --git-dir next to every current_dir usage solve this problem? If this works, it would be much easier to maintain as it only adds one extra argument to the git command without significantly increasing complexity.

I think you already did that before the latest change, right? The main issue was that we are manually constructing git commands all over the bootstrap codebase (which makes it hard to ensure setting --git-dir properly). If we manage all the git commands from a single interface, we could maintain this much more easily.

RalfJung · 2024-06-11T20:19:45Z

I tried that, yeah -- but it's still not great in a commit hook: when you want to actually change the submodule, and then commit, weird stuff happens. It first un-does your change and then un-does the un-doing, at least with the cargo subrepo which is currently checked twice each time (first by the loop that always initializes cargo, then by the "check all existing submodules" check).

RalfJung · 2024-06-11T20:21:26Z

Also to be clear I have not systematically searched for git invocations in the codebase, I just refactored the one I stumbled into.

Due to rust-lang#125954, we had to modify git invocations with certain flags in rust-lang#126255. However, because there are so many instances of `Command::new("git")` in bootstrap, it is difficult to apply these solutions to all of them. This PR creates a helper function that unifies the git usage in bootstrap. Meaning, whenever special flags or hacks are needed, we can apply them to this single function which makes things much simpler for the bootstrap team. Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan · 2024-06-12T08:48:27Z

Can we wait until we ship #126309 ? It should help significantly in making this work cleaner.

Due to rust-lang#125954, we had to modify git invocations with certain flags in rust-lang#126255. However, because there are so many instances of `Command::new("git")` in bootstrap, it is difficult to apply these solutions to all of them. This PR creates a helper function that unifies the git usage in bootstrap. Meaning, whenever special flags or hacks are needed, we can apply them to this single function which makes things much simpler for the bootstrap team. Signed-off-by: onur-ozkan <work@onurozkan.dev>

RalfJung · 2024-06-12T09:25:12Z

Sure, this is not urgent.

…r=Kobzol unify git command preperation Due to rust-lang#125954, we had to modify git invocations with certain flags in rust-lang#126255. However, because there are so many instances of `Command::new("git")` in bootstrap, it is difficult to apply these solutions to all of them. This PR creates a helper function that unifies the git usage in bootstrap. Meaning, whenever special flags or hacks are needed, we can apply them to this single function which makes things much simpler for the bootstrap team.

Rollup merge of rust-lang#126309 - onur-ozkan:unify-git-utilization, r=Kobzol unify git command preperation Due to rust-lang#125954, we had to modify git invocations with certain flags in rust-lang#126255. However, because there are so many instances of `Command::new("git")` in bootstrap, it is difficult to apply these solutions to all of them. This PR creates a helper function that unifies the git usage in bootstrap. Meaning, whenever special flags or hacks are needed, we can apply them to this single function which makes things much simpler for the bootstrap team.

bors · 2024-06-16T10:47:22Z

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

onur-ozkan · 2024-06-16T10:48:48Z

Can we wait until we ship #126309 ? It should help significantly in making this work cleaner.

It is merged.

RalfJung · 2024-06-17T10:36:13Z

So which option would you prefer -- unsetting GIT_DIR and GIT_INDEX_FILE, or disabling the submodule handling entirely when they are set?
Even when the env vars are unset, some odd things are happening when one intentionally wants to commit a submodule change and has x.py fmt as a commit hook -- but presumably the people doing submodule updates will have x.py submodule handling disabled. OTOH someone may run x.py as part of an automated bisect (git bisect run) or as part of the exec flag in a git rebase, and then I guess they do want x.py handling.

onur-ozkan · 2024-06-17T13:00:34Z

We had this workaround previously, so if we unset GIT_DIR in bootstrap (in the centrealized git function in helper module), that should fix the problem. I am not sure why that undoing->un-undoing thing happens, but it doesn't break anything in the process as far as I understand..

RalfJung · 2024-06-17T13:37:33Z

Un-setting GIT_DIR is not enough, I tried that. One has to also unset GIT_INDEX_FILE.

I am not sure why that undoing->un-undoing thing happens, but it doesn't break anything in the process as far as I understand..

I think it would break something for submodules that are only updated once, i.e. all the ones which are not forcefully checked out. (Ideally bootstrap would be patched to not update anything twice...)

RalfJung · 2024-06-17T14:17:17Z

PR updated.

@rustbot ready

RalfJung · 2024-06-17T14:18:07Z

src/bootstrap/src/lib.rs

-        let checked_out_hash =
-            output(helpers::git(Some(&absolute_path)).args(["rev-parse", "HEAD"]));
-        // update_submodules
+        let submodule_git = || helpers::git(Some(&absolute_path));


I left this closure in since I think it makes the code more readable to abstract this common call away.

onur-ozkan

Thanks for the fix!

Ideally bootstrap would be patched to not update anything twice...

We are planning to track and cache git certain invocations, I think by using the same interface we can achieve this.

onur-ozkan · 2024-06-17T17:17:28Z

@bors r+ rollup

bors · 2024-06-17T17:17:31Z

📌 Commit e3e7140 has been approved by onur-ozkan

It is now in the queue for this repository.

RalfJung · 2024-06-17T17:24:14Z

We are planning to track and cache git certain invocations, I think by using the same interface we can achieve this.

For this particular case, it seems easier to make update_existing_submodules return a list of submodules (instead of calling update_submodule directly), add ["src/tools/cargo", "src/doc/book", "library/backtrace", "library/stdarch"] if they aren't in there already, and then iterate the result. 🤷

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#126255 (fix checking git submodules during a commit hook) - rust-lang#126355 (Pass target to some run-make tests) - rust-lang#126567 (Rename `InstanceDef` -> `InstanceKind`) - rust-lang#126579 (Fix broken documentation link) - rust-lang#126596 (Add tracking issue number to async_drop APIs) - rust-lang#126603 (Update books) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#126255 - RalfJung:bootstrap-submodules, r=onur-ozkan fix checking git submodules during a commit hook Fixes rust-lang#125954 The one thing that still puzzles me is why this only is a problem with worktrees. r? `@onur`

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 Jun 11, 2024

rustbot assigned onur-ozkan Jun 11, 2024

This comment has been minimized.

Sign in to view

onur-ozkan 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 Jun 11, 2024

RalfJung force-pushed the bootstrap-submodules branch from 9fe297e to f768fc8 Compare June 11, 2024 15:11

onur-ozkan mentioned this pull request Jun 12, 2024

unify git command preperation #126309

Merged

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

onur-ozkan added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jun 12, 2024

onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jun 16, 2024

RalfJung force-pushed the bootstrap-submodules branch from f768fc8 to 91c9f03 Compare June 17, 2024 14:16

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2024

RalfJung commented Jun 17, 2024

View reviewed changes

fix checking git submodules during a commit hook

e3e7140

RalfJung force-pushed the bootstrap-submodules branch from 91c9f03 to e3e7140 Compare June 17, 2024 14:19

onur-ozkan approved these changes Jun 17, 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 Jun 17, 2024

matthiaskrgr mentioned this pull request Jun 17, 2024

Rollup of 6 pull requests #126605

Merged

bors merged commit dcda375 into rust-lang:master Jun 17, 2024
6 checks passed

rustbot added this to the 1.81.0 milestone Jun 17, 2024

RalfJung deleted the bootstrap-submodules branch June 19, 2024 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix checking git submodules during a commit hook #126255

fix checking git submodules during a commit hook #126255

RalfJung commented Jun 11, 2024

rustbot commented Jun 11, 2024

rustbot commented Jun 11, 2024

RalfJung commented Jun 11, 2024

This comment has been minimized.

RalfJung commented Jun 11, 2024 via email

onur-ozkan commented Jun 11, 2024

RalfJung commented Jun 11, 2024

onur-ozkan commented Jun 11, 2024

RalfJung commented Jun 11, 2024

RalfJung commented Jun 11, 2024

RalfJung commented Jun 11, 2024

onur-ozkan commented Jun 11, 2024

RalfJung commented Jun 11, 2024

onur-ozkan commented Jun 11, 2024

onur-ozkan commented Jun 11, 2024 •

edited

Loading

RalfJung commented Jun 11, 2024

RalfJung commented Jun 11, 2024

onur-ozkan commented Jun 12, 2024

RalfJung commented Jun 12, 2024

bors commented Jun 16, 2024

onur-ozkan commented Jun 16, 2024

RalfJung commented Jun 17, 2024

onur-ozkan commented Jun 17, 2024 •

edited

Loading

RalfJung commented Jun 17, 2024

RalfJung commented Jun 17, 2024

RalfJung Jun 17, 2024

onur-ozkan left a comment

onur-ozkan commented Jun 17, 2024

bors commented Jun 17, 2024

RalfJung commented Jun 17, 2024

fix checking git submodules during a commit hook #126255

fix checking git submodules during a commit hook #126255

Conversation

RalfJung commented Jun 11, 2024

rustbot commented Jun 11, 2024

rustbot commented Jun 11, 2024

RalfJung commented Jun 11, 2024

This comment has been minimized.

RalfJung commented Jun 11, 2024 via email

onur-ozkan commented Jun 11, 2024

RalfJung commented Jun 11, 2024

onur-ozkan commented Jun 11, 2024

RalfJung commented Jun 11, 2024

RalfJung commented Jun 11, 2024

RalfJung commented Jun 11, 2024

onur-ozkan commented Jun 11, 2024

RalfJung commented Jun 11, 2024

onur-ozkan commented Jun 11, 2024

onur-ozkan commented Jun 11, 2024 • edited Loading

RalfJung commented Jun 11, 2024

RalfJung commented Jun 11, 2024

onur-ozkan commented Jun 12, 2024

RalfJung commented Jun 12, 2024

bors commented Jun 16, 2024

onur-ozkan commented Jun 16, 2024

RalfJung commented Jun 17, 2024

onur-ozkan commented Jun 17, 2024 • edited Loading

RalfJung commented Jun 17, 2024

RalfJung commented Jun 17, 2024

RalfJung Jun 17, 2024

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

onur-ozkan commented Jun 17, 2024

bors commented Jun 17, 2024

RalfJung commented Jun 17, 2024

onur-ozkan commented Jun 11, 2024 •

edited

Loading

onur-ozkan commented Jun 17, 2024 •

edited

Loading