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

Sync LLVM submodule if it has been initialized #78153

Merged
merged 1 commit into from
Oct 23, 2020

Conversation

est31
Copy link
Member

@est31 est31 commented Oct 20, 2020

Since having enabled the download-ci-llvm option,
and having rebased on top of #76864,
I've noticed that I had to update the llvm-project
submodule manually if it was checked out.
Orignally, the submodule update logic was
introduced to reduce the friction for contributors
to manage the submodules, or in other words, to prevent
getting PRs that have unwanted submodule rollbacks
because the contributors didn't run git submodule update.

This commit adds logic to ensure there is no inadvertent
LLVM submodule rollback in a PR if download-ci-llvm
(or llvm-config) is enabled. It will detect whether the
llvm-project submodule is initialized, and if so, update
it in any case. If it is not initialized, behaviour is
kept to not do any update/initialization.

An alternative to the chosen implementation would
be to not pass the --init command line arg to
git submodule update for the src/llvm-project
submodule. This would show a confusing error message
however on all builds with an uninitialized repo.
We could pass the --silent param, but we still want
it to print something if it is initialized and has
to update something.
So we just do a manual check for whether the
submodule is initialized.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2020
@est31 est31 marked this pull request as draft October 20, 2020 17:09
@est31 est31 marked this pull request as ready for review October 20, 2020 17:17
@Mark-Simulacrum
Copy link
Member

It seems like we should sync the submodule with llvm-config specified too, I don't see why the two options should be different here.

Since having enabled the download-ci-llvm option,
and having rebased on top of f05b47c,
I've noticed that I had to update the llvm-project
submodule manually if it was checked out.
Orignally, the submodule update logic was
introduced to reduce the friction for contributors
to manage the submodules, or in other words, to prevent
getting PRs that have unwanted submodule rollbacks
because the contributors didn't run git submodule update.

This commit adds logic to ensure there is no inadvertent
LLVM submodule rollback in a PR if download-ci-llvm
(or llvm-config) is enabled. It will detect whether the
llvm-project submodule is initialized, and if so, update
it in any case. If it is not initialized, behaviour is
kept to not do any update/initialization.

An alternative to the chosen implementation would
be to not pass the --init command line arg to
`git submodule update` for the src/llvm-project
submodule. This would show a confusing error message
however on all builds with an uninitialized repo.
We could pass the --silent param, but we still want
it to print something if it is initialized and has
to update something.
So we just do a manual check for whether the
submodule is initialized.
@Mark-Simulacrum
Copy link
Member

I think CI failure here is unrelated so going to go ahead and r+, but if not we'll want to fix that.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2020

📌 Commit 5948e62 has been approved by Mark-Simulacrum

@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 Oct 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 22, 2020
…=Mark-Simulacrum

Sync LLVM submodule if it has been initialized

Since having enabled the download-ci-llvm option,
and having rebased on top of rust-lang#76864,
I've noticed that I had to update the llvm-project
submodule manually if it was checked out.
Orignally, the submodule update logic was
introduced to reduce the friction for contributors
to manage the submodules, or in other words, to prevent
getting PRs that have unwanted submodule rollbacks
because the contributors didn't run git submodule update.

This commit adds logic to ensure there is no inadvertent
LLVM submodule rollback in a PR if download-ci-llvm
(or llvm-config) is enabled. It will detect whether the
llvm-project submodule is initialized, and if so, update
it in any case. If it is not initialized, behaviour is
kept to not do any update/initialization.

An alternative to the chosen implementation would
be to not pass the --init command line arg to
`git submodule update` for the src/llvm-project
submodule. This would show a confusing error message
however on all builds with an uninitialized repo.
We could pass the --silent param, but we still want
it to print something if it is initialized and has
to update something.
So we just do a manual check for whether the
submodule is initialized.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 22, 2020
…=Mark-Simulacrum

Sync LLVM submodule if it has been initialized

Since having enabled the download-ci-llvm option,
and having rebased on top of rust-lang#76864,
I've noticed that I had to update the llvm-project
submodule manually if it was checked out.
Orignally, the submodule update logic was
introduced to reduce the friction for contributors
to manage the submodules, or in other words, to prevent
getting PRs that have unwanted submodule rollbacks
because the contributors didn't run git submodule update.

This commit adds logic to ensure there is no inadvertent
LLVM submodule rollback in a PR if download-ci-llvm
(or llvm-config) is enabled. It will detect whether the
llvm-project submodule is initialized, and if so, update
it in any case. If it is not initialized, behaviour is
kept to not do any update/initialization.

An alternative to the chosen implementation would
be to not pass the --init command line arg to
`git submodule update` for the src/llvm-project
submodule. This would show a confusing error message
however on all builds with an uninitialized repo.
We could pass the --silent param, but we still want
it to print something if it is initialized and has
to update something.
So we just do a manual check for whether the
submodule is initialized.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2020
Rollup of 17 pull requests

Successful merges:

 - rust-lang#77268 (Link to "Contributing to Rust" rather than "Getting Started".)
 - rust-lang#77339 (Implement TryFrom between NonZero types.)
 - rust-lang#77488 (Mark `repr128` as `incomplete_features`)
 - rust-lang#77890 (Fixing escaping to ensure generation of welformed json.)
 - rust-lang#77918 (Cleanup network tests)
 - rust-lang#77920 (Avoid extraneous space between visibility kw and ident for statics)
 - rust-lang#77969 (Doc formating consistency between slice sort and sort_unstable, and big O notation consistency)
 - rust-lang#78098 (Clean up and improve some docs)
 - rust-lang#78116 (Make inline const work in range patterns)
 - rust-lang#78153 (Sync LLVM submodule if it has been initialized)
 - rust-lang#78163 (Clean up lib docs)
 - rust-lang#78169 (Update cargo)
 - rust-lang#78231 (Make closures inherit the parent function's target features)
 - rust-lang#78235 (Explain where the closure return type was inferred)
 - rust-lang#78255 (Reduce diagram mess in 'match arms have incompatible types' error)
 - rust-lang#78263 (Add regression test of issue-77668)
 - rust-lang#78265 (Add some inference-related regression tests about incorrect diagnostics)

Failed merges:

r? `@ghost`
@bors bors merged commit 025481a into rust-lang:master Oct 23, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 23, 2020
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants