-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
replace llvm-rebuild-trigger with submodule commit hash #59303
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
b2ebcd9
to
60a091d
Compare
In CI, some of the submodules (especially LLVM) are unpacked from plain archives, not git: Lines 57 to 63 in 48e354d
Maybe we can have that Or per #59285 (comment), the rebuild trigger doesn't really matter for CI anyway, so perhaps this git check just needs to fail gracefully. |
Anyone building from rustc-src tarballs will also lack git info. |
@cuviper For those building with rust-src tarballs, is making the git check fail gracefully sufficient? |
The worst thing that can happen if the git commit cannot be fetched is CMake being re-configured and then no-op build being performed (that's about 10 seconds on my machine). |
I agree, it should be fine to just assume a rebuild is needed in that case. |
3c18f0c
to
cdb623b
Compare
Ok, I modified the git check to print a message if there's an error pulling the submodule commit and then proceed with the LLVM build. |
LGTM, but we should wait for #59285 to land, as there will be a merge conflict on the removed file. |
☔ The latest upstream changes (presumably #59285) made this pull request unmergeable. Please resolve the merge conflicts. |
cdb623b
to
e1daa36
Compare
Rebased. |
Let's see, I should have reviewer privilege now -- of course, others are welcome to review too. @bors r+ |
📌 Commit e1daa36 has been approved by |
Not including in rollup => @bors p=1 |
@bors rollup- p=1 |
What would happen here when people change things within the submodule and then rebuild? Change things and rebuild multiple times? |
They would have to make a new commit for the change to be recognized.
…On Thu, Mar 28, 2019, 8:46 AM Simonas Kazlauskas ***@***.***> wrote:
What would happen here when people change things within the submodule and
then rebuild? Do it multiple times?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#59303 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABTxFjpB8DM6KfgcJ4djt55q5-ni5Nwaks5vbLmMgaJpZM4b9VjO>
.
|
We could use If you want it to consider dirty state too, we'll need to use something besides |
replace llvm-rebuild-trigger with submodule commit hash As mentioned in #59285. This PR removes the need to update the `llvm-rebuild-trigger` file. Instead, the latest commit hash of the appropriate LLVM submodule will be stored in the stamp file and used to detect if a build is required. Fixes #42405. Fixes #54959. Fixes #55537.
☀️ Test successful - checks-travis, status-appveyor |
How to rebuild if llvm directory is in dirty state? |
rm build/<target>/llvm/llvm-finished-building |
Thanks!!! That's really helpful for me. |
As mentioned in #59285.
This PR removes the need to update the
llvm-rebuild-trigger
file. Instead, the latest commit hash of the appropriate LLVM submodule will be stored in the stamp file and used to detect if a build is required.Fixes #42405.
Fixes #54959.
Fixes #55537.