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

Check submodule commit provenance in CI #75109

Open
jethrogb opened this issue Aug 3, 2020 · 4 comments
Open

Check submodule commit provenance in CI #75109

jethrogb opened this issue Aug 3, 2020 · 4 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@jethrogb
Copy link
Contributor

jethrogb commented Aug 3, 2020

Currently, when any changes are made to a submodule in the Rust repository, rustbot adds a warning comment but it's up to the reviewer to check all the changes.

One of the things that should always be true of a submodule commit (at merge time in the Rust repo) is that it should already be merged in an upstream branch. This can be checked in an automated fashion during CI by checking the commit against a set of allowed remote branches.

If this is not checked, something that might happen is that an author simultaneously makes an upstream and Rust PR with one commit. Later, the author (force) pushes another commit to the same upstream PR but forgets to update the Rust submodule. The original commit will at some point be garbage-collected by GitHub and merging the Rust PR would be bad because the complete source code would not be accessible in git in the future. This happened in #75009.

This check should not be done for try builds, so that development can continue unimpeded.

@rustbot modify labels: +C-enhancement +T-infra

@rustbot rustbot added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Aug 3, 2020
@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2020

FWIW, we have used this technique of merging not-yet-upstream-merged submodule updates into rustc master in the past in Miri. That was quite helpful when disentangling a mess of mutual dependencies. We were careful to make sure the Miri branch gets merged into Miri master later without any force-pushing. But we haven't done that in a while so I think these days we could live without being able to do that.

Cc @rust-lang/miri

@jethrogb
Copy link
Contributor Author

It doesn't necessarily need to be merged, the submodule commit just needs to be in the set of allowed remote branches for the upstream project. Potentially each project could designate their own set.

@jethrogb
Copy link
Contributor Author

Close call: #82102

@nagisa
Copy link
Member

nagisa commented Feb 18, 2021

What I think happened in the referenced PR was this:

I fetched the llvm-project with git fetch origin and wanting to make changes on top of the rustc/11.0-2021-01-05 I ran git checkout rustc/11.0-2021-01-05, made my changes, committed them, and submitted the PR.

Later when the PR was already merged, I took care to git fetch origin and git checkout -f rustc/11.0-2021-01-05 (note the missing origin/ in front of rustc/) which made git to check out the branch that I had implicitly created locally in the previous step, rather than the rustc/11.0-2021-01-05 as it appears in the origin remote.

There're definitely a fair amount of potential footguns here.

Amanieu added a commit to Amanieu/rust that referenced this issue Jan 18, 2022
PR rust-lang#93016 was merged with the stdarch submodule pointing to a commit in
a PR branch and not in master. This was due to a circular dependency
between the rust and stdarch changes which would cause the other to fail
to build.

cc rust-lang#75109
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 18, 2022
Fix stdarch submodule pointing to commit outside tree

PR rust-lang#93016 was merged with the stdarch submodule pointing to a commit in
a PR branch and not in master. This was due to a circular dependency
between the rust and stdarch changes which would cause the other to fail
to build.

cc rust-lang#75109
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants