rustc: Link LLVM directly into rustc again #65703
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @zackmdavis (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 |
21e13c1 to
6bbede3
Compare
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
This pretty much looks great to me -- less steps in bootstrap is always an improvement! @bors r+ (CI failure appears to be spurious) |
|
📌 Commit 6bbede35569584aef12cfde9cb9093cc21483c0b has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
|
☔ The latest upstream changes (presumably #65733) made this pull request unmergeable. Please resolve the merge conflicts. |
6bbede3 to
5ac40ea
Compare
|
@bors: r=Mark-Simulacrum |
|
📌 Commit 5ac40ea197bf4b7d2ff5bd98f0ec7fcec1dc8735 has been approved by |
|
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 |
bjorn3
reviewed
Oct 25, 2019
| @@ -131,6 +131,7 @@ const WHITELIST: &[Crate<'_>] = &[ | |||
| Crate("polonius-engine"), | |||
| Crate("ppv-lite86"), | |||
| Crate("proc-macro2"), | |||
| Crate("punycode"), | |||
There was a problem hiding this comment.
We didn't check it before? It's presumably only depended on by librustc_codegen_lvm
| @@ -272,7 +270,7 @@ pub fn get_codegen_backend(sess: &Session) -> Box<dyn CodegenBackend> { | |||
| filename if filename.contains(".") => { | |||
| load_backend_from_dylib(filename.as_ref()) | |||
| } | |||
There was a problem hiding this comment.
Maybe move this to get_builtin_codegen_backend and rename that function to get_codegen_backend, while renaming this function to get_codegen_backend_for_sess or even inline this function.
There was a problem hiding this comment.
I'm not sure I understand? What would the purpose of that movement be?
There was a problem hiding this comment.
The purpose of that change is that the version which doesn't take a Session as input will accept /path/to/backend.so. I am fine with keeping it the way it is right now.
| @@ -379,9 +379,6 @@ | |||
| # and currently the only standard option supported is `"llvm"` | |||
| #codegen-backends = ["llvm"] | |||
There was a problem hiding this comment.
Just to confirm, this one still works? Or do I need to use llvm.enabled instead?
There was a problem hiding this comment.
It would also be nice if this would automatically pass a feature flag for every item, even unknown ones, so adding a codegen backend is as easy as adding a few crate features, adding optional deps, and changing rustc_interface to recognize the -Zcodegen-backend value for it.
There was a problem hiding this comment.
It should be the case that setting this to the empty array should work. I'll leave it to future PRs though to continue to tweak how multiple codegen backends work.
| @@ -501,6 +501,9 @@ impl Build { | |||
| if self.config.jemalloc { | |||
| features.push_str("jemalloc"); | |||
| } | |||
| if self.config.llvm_enabled() { | |||
| features.push_str(" llvm"); | |||
There was a problem hiding this comment.
Perhaps? It's all internal anyway and can change at any time, I sort of prefer just llvm but feel free to send a PR after this if you feel differently.
5ac40ea to
7786647
Compare
|
@bors: r=Mark-Simulacrum |
|
📌 Commit 7786647845faa968fa7e2d4dec131c2c4c8cec31 has been approved by |
|
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 |
|
⌛ Testing commit 7786647845faa968fa7e2d4dec131c2c4c8cec31 with merge a14b9ca89b008c74000ba5f31f91e27025095808... |
|
@bors: r- |
|
Ping from triage |
|
Sorry for the delay, I haven't forgotten about this, just been busy. I've got some builds going in the background to help debug the failure here. |
|
@bors: r=Mark-Simulacrum Ok I updated the test and confirmed that it doesn't actually result in any more build time. |
|
📌 Commit e7f4fb3 has been approved by |
|
⌛ Testing commit e7f4fb3 with merge c979d3f0999329d39d30213f961d7fec9dd09498... |
|
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 |
|
💔 Test failed - checks-azure |
|
☔ The latest upstream changes (presumably #66610) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping from triage: |
|
Unfortunately I don't think I have the time to work through this, so I'm going to close it. |
|
@alexcrichton: I'd be interested in working on finishing up this PR |
|
Sure thing, please feel free to do so! |
bors
added a commit
that referenced
this pull request
Dec 6, 2019
rustc: Link LLVM directly into rustc again (take two) This is a continuation of PR #65703
bors
added a commit
that referenced
this pull request
Dec 13, 2019
rustc: Link LLVM directly into rustc again (take two) This is a continuation of PR #65703
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit builds on #65501 continue to simplify the build system and
compiler now that we no longer have multiple LLVM backends to ship by
default. Here this switches the compiler back to what it once was long
long ago, which is linking LLVM directly to the compiler rather than
dynamically loading it at runtime. The
codegen-backendsdirectory ofthe sysroot no longer exists and all relevant support in the build
system is removed. Note that
rustcstill supports a dynamically loadedcodegen backend as it did previously, it just no longer supports
dynamically loaded codegen backends in its own sysroot.
Additionally as part of this the
librustc_codegen_llvmcrate now onceagain explicitly depends on all of its crates instead of implicitly
loading them through the sysroot. This involved filling out its
Cargo.tomland deleting all the now-unnecessaryextern crateannotations in the header of the crate. (this in turn required adding a
number of imports for names of macros too).
The end results of this change are:
the codegen backend" checks can be easily removed.
rustc_codegen_llvmis much simpler since it's simplyanother compiler crate.
rustc_codegen_llvmis much simpler sinceit's "just another
Cargo.tomlto edit"parallelism in the main rustc build step rather than splitting
librustc_codegen_llvmout to its own step.codegen backend does not need to be dynamically loaded.
multiple codegen backends is still supported, and dynamic loading of a
codegen backend is still supported.