Skip to content

Make LLVM version suffix independent of rustc version on dev channel #70882

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

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Apr 7, 2020

Remove rustc version from LLVM version suffix on dev channel,
avoiding the need for full rebuilds when switching between
branches with different LLVM submodule & rustc version.

Note: To avoid full rebuild, on subsequent LLVM submodule update, copy the
current value of LLVM_VERSION_SUFFIX from build/*/llvm/build/CMakeCache.txt,
to version-suffix in config.toml.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 7, 2020
@Mark-Simulacrum
Copy link
Member

I'm surprised this fixes things. CFG_RELEASE_NUM, as far as I can tell, is changed once every 6 weeks, so presumably when switching branches that's not changing much.

Can you confirm that's the cause of the rebuilds you're seeing, and if so, add a comment noting that this is necessary mostly around bootstrap bump time (when if you're switching between branches and don't rebase them on master, you'll need to rebuild)?

Thinking more on this, though, if you're not rebasing your branches on master (fixing the submodule as a side-effect) during bootstrap compiler bump (every 6 weeks), you're rebuilding all of std/rustc anyway, so I would expect that a LLVM rebuild isn't all that much more work. Maybe I'm wrong about that though?

@tmiasko
Copy link
Contributor Author

tmiasko commented Apr 7, 2020

I agree that this shouldn't be an issue too often, but is there any reason not to fix this anyway?

Remove rustc version from LLVM version suffix on dev channel, avoiding
the need for full rebuilds when moving between commits with different
LLVM submodule & rustc version.
@tmiasko tmiasko force-pushed the llvm-version-suffix branch from 2b8d6f7 to 7c5a4cd Compare April 7, 2020 13:03
@tmiasko
Copy link
Contributor Author

tmiasko commented Apr 7, 2020

I added a comment explaining the motivation behind the current implementation, so one does not have to look up the commit message to find it out.

@Mark-Simulacrum
Copy link
Member

Well, it's not that we can't fix it, more so that I'm not quite sure this is the right fix. I unfortunately don't have the reasoning for including the rustc version in the filename/version in cache, but from what I recall it's because we needed to avoid loading the wrong LLVM when compiling or so (when you're e.g. bootstrapping and have both beta sysroot and the new sysroot in your load path).

But given that this PR only affects the dev channel (for which we never ship artifacts) that shouldn't be a problem, I think. Though it could lead to problems if trying to build rust itself with a dev channel compiler, but that doesn't happen today either to my knowledge.

Let's land this, and if it turns out this breaks things in practice during e.g. bootstrap bump we can revert that decision.

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 7, 2020

📌 Commit 7c5a4cd 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 Apr 7, 2020
@bors
Copy link
Collaborator

bors commented Apr 12, 2020

⌛ Testing commit 7c5a4cd with merge 458ad1c7cf29552f9fc6f6d05c312ad8d9f33130...

@Dylan-DPC-zz
Copy link

@bors retry yield

@bors
Copy link
Collaborator

bors commented Apr 13, 2020

⌛ Testing commit 7c5a4cd with merge d28a464...

@bors
Copy link
Collaborator

bors commented Apr 13, 2020

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing d28a464 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 13, 2020
@bors bors merged commit d28a464 into rust-lang:master Apr 13, 2020
@tmiasko tmiasko deleted the llvm-version-suffix branch April 13, 2020 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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