Support compiling rustc without LLVM (try 2) #42932

bjorn3 · 2017-06-27T16:21:46Z

Now doesn't change rustc_driver.

Supersedes #42752

rust-highfive · 2017-06-27T16:21:50Z

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

eddyb · 2017-06-27T17:51:35Z

cc @rust-lang/compiler
I am personally not in favor of the approach taken here and would rather make rustc_trans LLVM-only and extract all the other functionality out of it. There are just too many #[cfg(llvm)].

Keruspe · 2017-06-30T08:02:12Z

src/bootstrap/config.toml.example

@@ -14,6 +14,9 @@
 # =============================================================================
 [llvm]

+# Indicates whether rustc will support compilation with LLVM (currently broken)


The comment seems to imply that the build with llvm is broken, not the build without it. Should be the other way round, right?

bors · 2017-07-04T18:29:51Z

☔ The latest upstream changes (presumably #43051) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2017-07-12T11:02:13Z

I am personally not in favor of the approach taken here and would rather make rustc_trans LLVM-only and extract all the other functionality out of it. There are just too many #[cfg(llvm)].

So llvm would just be a cargo feature of rustc_driver either including rustc_trans_llvm or not, depending on whether the feature is active or not?

eddyb · 2017-07-12T11:57:03Z

@oli-obk More or less, yes.

carols10cents · 2017-07-17T12:21:50Z

friendly ping to keep this on your radar, @bjorn3!

nikomatsakis · 2017-07-17T13:45:45Z

Without looking in detail, I definitely agree with @eddyb that we should strive to factor things better and avoid quite so many #[cfg] gates. @bjorn3, do you need more detailed guidance on how to achieve that?

bjorn3 · 2017-07-20T13:16:55Z

@eddyb is this a bit like you mean. If so I will work it out further in a few days.

bors · 2017-07-22T20:59:13Z

☔ The latest upstream changes (presumably #43059) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2017-07-27T23:21:44Z

Looks like this may be ready for another round of review @eddyb!

@bjorn3 it also looks like there may be rebase conflicts here now.

eddyb · 2017-07-28T05:24:04Z

src/librustc_driver/Cargo.toml

+ar = "0.3.0"
+
+[features]
+llvm = ["rustc_trans"]


Missing newline at the end of file.

eddyb

Now this looks much more streamlined!

eddyb · 2017-07-28T05:28:26Z

@bjorn3 Ahh how I wish I had a way to un-dismiss github notifications... I keep losing track of some I've gone through on my phone 😞. @alexcrichton Thanks for the ping!

bjorn3 · 2017-07-31T08:19:35Z

Rebased

eddyb · 2017-07-31T08:21:19Z

@bjorn3 Can you squash the commits to remove the back-and-forth (X followed by revert X)?

eddyb · 2017-07-31T08:23:35Z

src/librustc_driver/lib.rs

+
+#[cfg(not(feature="llvm"))]
+impl MetadataLoader for NoLLvmMetadataLoader {
+    fn get_rlib_metadata(&self, _: &Target, filename: &Path) -> Result<ErasedBoxRef<[u8]>, String> {


Was this copied wholesale from rustc_trans? Does that mean ar::Archive is independent of LLVM?
That could make rustc_metadata target-agnostic in the non-dynamic-library case!
cc @alexcrichton @rkruppe

The ar crate is a rust only .a reader and writer.

Then it should be used directly by rustc_metadata and get_rlib_metadata shouldn't exist as a pluggable API (but rather the code live in rustc_metadata) which means loader: Box<MetadataLoader> in rustc_metadata becomes just:

get_dylib_metadata: fn(&Target, &Path) -> Result<ErasedBoxRef<[u8]>, String>

When not available, one can just pass in |_, _| Err(String::from("unsupported")).

bjorn3 · 2017-07-31T12:16:07Z

Squashed

bjorn3 · 2017-08-05T15:35:10Z

Doesn't fail locally (mac os)

alexcrichton · 2017-08-10T19:00:12Z

Hm so looking over this it looks like this is changing how we're reading rlib metadata? The current construction is done specifically for speed as LLVM's ar parsing/reading is much speedier than many other solutions. Are we sure the ar crate is sufficiently fast enough? It initially looks that it is not, as one big difference is using memory maps vs actually reading the file into memory, which can make a huge difference here.

Also from a maintainability perspective, this seems like the kind of patch that'd bitrot almost instantly? Is there something we can do to avoid the #[cfg] jungle that will be very difficult to maintain over time?

eddyb · 2017-08-10T19:21:20Z

@alexcrichton OH! Uhm, when I asked about it it sounded like we already moved away from using the LLVM support - but if that's not true, then the "Move rlib metadata reading code to rustc_metadata and replace MetadataLoader with a function pointer" commit was a mistake...

bjorn3 · 2017-08-11T10:57:39Z

Travis passes.

eddyb · 2017-08-11T11:17:37Z

@bjorn3 Please squash the original commits with the reverts.

bjorn3 · 2017-08-11T13:26:56Z

Squashed and travis passes

eddyb · 2017-08-11T13:36:46Z

@bors r+

bors · 2017-08-11T13:36:47Z

📌 Commit e539996 has been approved by eddyb

bors · 2017-08-11T15:45:16Z

⌛ Testing commit e539996 with merge edd82ee...

Support compiling rustc without LLVM (try 2) Now doesn't change rustc_driver. Supersedes #42752

bors · 2017-08-11T18:37:24Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing edd82ee to master...

alexcrichton · 2017-08-11T18:48:26Z

@eddyb and @bjorn3 did y'all have no comments about the maintainability aspect above? Specifically:

Also from a maintainability perspective, this seems like the kind of patch that'd bitrot almost instantly? Is there something we can do to avoid the #[cfg] jungle that will be very difficult to maintain over time?

eddyb · 2017-08-11T18:55:46Z

@alexcrichton So I took a look at the diff to count them, remembering only 5-6... and now there's a lot more. And they're mostly in b7314c7 "Actually make rustc_driver compile without llvm".

eddyb · 2017-08-11T19:09:03Z

So that is definitely my fault, and if @alexcrichton wants this reverted then I'm on board.
@bjorn3 What are you willing to do here? I know I've asked for a lot during this PR.

bjorn3 · 2017-08-12T08:50:17Z

Im writing a pr to remove most of the cfg's. It will likely be ready tomorrow.

alexcrichton · 2017-08-12T15:10:05Z

Thanks @bjorn3! Feel free to cc or r? me on that

@alexcrichton

Cleanup for "Support compiling rustc without LLVM (try 2)" This includes a small patch to allow running tests without llvm. Also check if you are not trying to compile a dylib. cc #42932 r? @alexcrichton

rust-highfive assigned eddyb Jun 27, 2017

bjorn3 mentioned this pull request Jun 27, 2017

Support compiling rustc without LLVM #42752

Closed

2 tasks

arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 27, 2017

Keruspe reviewed Jun 30, 2017

View reviewed changes

bjorn3 force-pushed the no_llvm_try2 branch from 7b9617e to 36a3bf3 Compare July 20, 2017 13:15

alexcrichton added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 27, 2017

eddyb reviewed Jul 28, 2017

View reviewed changes

eddyb approved these changes Jul 28, 2017

View reviewed changes

bjorn3 force-pushed the no_llvm_try2 branch 2 times, most recently from 4c5839c to 7f191bc Compare July 31, 2017 08:18

eddyb reviewed Jul 31, 2017

View reviewed changes

bjorn3 force-pushed the no_llvm_try2 branch from 7f191bc to af8838d Compare July 31, 2017 12:15

bjorn3 force-pushed the no_llvm_try2 branch 3 times, most recently from 7b64996 to 47cd964 Compare August 1, 2017 13:26

bjorn3 force-pushed the no_llvm_try2 branch from 5c1bd88 to e20cdba Compare August 11, 2017 08:36

bjorn3 added 2 commits August 11, 2017 10:38

Build support for no llvm

e152a16

Make librustc_driver work without librustc_trans

b43c02b

bjorn3 force-pushed the no_llvm_try2 branch from e20cdba to 7e6afac Compare August 11, 2017 09:08

bjorn3 added 3 commits August 11, 2017 14:00

Actually make rustc_driver compile without llvm

b7314c7

It now completely compiles without LLVM!!!

b8d5c74

Remove some unused dependencies from rustc_trans_utils

e539996

bjorn3 force-pushed the no_llvm_try2 branch from 7e6afac to e539996 Compare August 11, 2017 12:00

bors added a commit that referenced this pull request Aug 11, 2017

Auto merge of #42932 - bjorn3:no_llvm_try2, r=eddyb

edd82ee

Support compiling rustc without LLVM (try 2) Now doesn't change rustc_driver. Supersedes #42752

bors merged commit e539996 into rust-lang:master Aug 11, 2017

This was referenced Aug 11, 2017

Support dynamically-linked and/or native musl targets #40113

Merged

WIP implement a machine readable print-type-info API #43761

Closed

bjorn3 mentioned this pull request Aug 13, 2017

Cleanup for "Support compiling rustc without LLVM (try 2)" #43842

Merged

bjorn3 deleted the no_llvm_try2 branch August 14, 2017 09:44

Support compiling rustc without LLVM (try 2) #42932

Support compiling rustc without LLVM (try 2) #42932

Conversation

bjorn3 commented Jun 27, 2017

rust-highfive commented Jun 27, 2017

eddyb commented Jun 27, 2017

Keruspe Jun 30, 2017

Choose a reason for hiding this comment

bjorn3 Jun 30, 2017

Choose a reason for hiding this comment

bors commented Jul 4, 2017

oli-obk commented Jul 12, 2017 • edited Loading

eddyb commented Jul 12, 2017

carols10cents commented Jul 17, 2017

nikomatsakis commented Jul 17, 2017

bjorn3 commented Jul 20, 2017

bors commented Jul 22, 2017

alexcrichton commented Jul 27, 2017

eddyb Jul 28, 2017

Choose a reason for hiding this comment

bjorn3 Jul 31, 2017

Choose a reason for hiding this comment

eddyb left a comment

Choose a reason for hiding this comment

eddyb commented Jul 28, 2017

bjorn3 commented Jul 31, 2017

eddyb commented Jul 31, 2017

eddyb Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

bjorn3 Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

eddyb Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

bjorn3 commented Jul 31, 2017

bjorn3 commented Aug 5, 2017

alexcrichton commented Aug 10, 2017

eddyb commented Aug 10, 2017

bjorn3 commented Aug 11, 2017

eddyb commented Aug 11, 2017

bjorn3 commented Aug 11, 2017

eddyb commented Aug 11, 2017

bors commented Aug 11, 2017

bors commented Aug 11, 2017

bors commented Aug 11, 2017

alexcrichton commented Aug 11, 2017

eddyb commented Aug 11, 2017

eddyb commented Aug 11, 2017

bjorn3 commented Aug 12, 2017

alexcrichton commented Aug 12, 2017

oli-obk commented Jul 12, 2017 •

edited

Loading

eddyb Jul 31, 2017 •

edited

Loading

bjorn3 Jul 31, 2017 •

edited

Loading

eddyb Jul 31, 2017 •

edited

Loading