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

fix checking git submodules during a commit hook #126255

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

RalfJung
Copy link
Member

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

r? @onur

@rustbot
Copy link
Collaborator

rustbot commented Jun 11, 2024

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 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
Copy link
Collaborator

rustbot commented Jun 11, 2024

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

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

@RalfJung
Copy link
Member Author

r? @onur-ozkan

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 11, 2024 via email

@onur-ozkan
Copy link
Member

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.

@onur-ozkan 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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

onur-ozkan commented Jun 11, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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

onur-ozkan added a commit to onur-ozkan/rust that referenced this pull request Jun 12, 2024
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
Copy link
Member

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

onur-ozkan added a commit to onur-ozkan/rust that referenced this pull request Jun 12, 2024
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 added a commit to onur-ozkan/rust that referenced this pull request Jun 12, 2024
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
Copy link
Member Author

Sure, this is not urgent.

@onur-ozkan 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 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
jhpratt added a commit to jhpratt/rust that referenced this pull request Jun 16, 2024
…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.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2024
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
Copy link
Contributor

bors commented Jun 16, 2024

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

@onur-ozkan
Copy link
Member

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

It is merged.

@onur-ozkan 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
Copy link
Member Author

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
Copy link
Member

onur-ozkan commented Jun 17, 2024

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
Copy link
Member Author

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
Copy link
Member Author

PR updated.

@rustbot ready

@rustbot 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
let checked_out_hash =
output(helpers::git(Some(&absolute_path)).args(["rev-parse", "HEAD"]));
// update_submodules
let submodule_git = || helpers::git(Some(&absolute_path));
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 left this closure in since I think it makes the code more readable to abstract this common call away.

Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 17, 2024

📌 Commit e3e7140 has been approved by onur-ozkan

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 Jun 17, 2024
@RalfJung
Copy link
Member Author

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. 🤷

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 17, 2024
…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
@bors bors merged commit dcda375 into rust-lang:master Jun 17, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 17, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 17, 2024
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`
@RalfJung RalfJung deleted the bootstrap-submodules branch June 19, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

With submodules=true, using ./x.py fmt as a git hook makes bootstrap delete all submodules
5 participants