Update rustbuild commentary #189
Conversation
build.rs
Outdated
| // need, so include a few more that aren't typically needed by | ||
| // LLVM/Rust. | ||
| sources.extend(&[ | ||
| "ffsdi2.c", |
There was a problem hiding this comment.
How come this was moved up?
There was a problem hiding this comment.
I could leave it where it is, but that invites questions (why is it separate from the rest of the files?).
The comment here suggests an answer, but doesn't make sense given that the file's inclusion is unconditional (the condition was removed in 91eaa85, but I don't know why).
So since the inclusion is unconditional, I figured I'd just move it into the main slice.
There was a problem hiding this comment.
This is currently in a dedicated section because of the comment. This file is not needed to compile Rust but it's needed to compile Rust's dependencies, namely jemalloc.
There was a problem hiding this comment.
OK, so the conditional bit should be restored?
build.rs
Outdated
| @@ -4003,16 +4003,15 @@ mod c { | |||
|
|
|||
| /// Compile intrinsics from the compiler-rt C source code | |||
| pub fn compile(llvm_target: &[&str]) { | |||
| let target_arch = env::var("CARGO_CFG_TARGET_ARCH").unwrap(); | |||
| let target_env = env::var("CARGO_CFG_TARGET_ENV").unwrap(); | |||
| let target_os = env::var("CARGO_CFG_TARGET_OS").unwrap(); | |||
There was a problem hiding this comment.
These all need to stay as environment variables due to cross compilation
Also use `cfg!(feature = "rustbuild")` instead of the environment variable to ease grep-ability.
|
@alexcrichton I think this ready for another look - |
|
@alexcrichton I think #188 was maybe incorrect; should we restore that feature and add tests that exercise it? |
|
Sure! |
|
Hm, it's not exactly clear to me how to meaningfully test that outside of "does rustc still compile", so I'm going to leave it as is. Is this good to go? |
|
@bors: r+ Looks good! |
|
📌 Commit a32e76e has been approved by |
Update rustbuild commentary r? @nikomatsakis This is the reason that rust-lang/rust#44509 doesn't work - the `rustbuild` feature _is_ actually used, it was just incorrectly documented here and I missed it.
|
💔 Test failed - status-travis |
|
@bors: retry
…On Wed, Sep 13, 2017 at 6:12 PM, bors ***@***.***> wrote:
💔 Test failed - status-travis
<https://travis-ci.org/rust-lang-nursery/compiler-builtins/builds/275208001?utm_source=github_status&utm_medium=notification>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#189 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD95JEHi6U7-SlJ6aemvv8e-8Wk0Wjkks5siGFrgaJpZM4PVX4b>
.
|
|
@alexcrichton any idea what that failure is? I can't even make sense of the output. |
Update rustbuild commentary r? @nikomatsakis This is the reason that rust-lang/rust#44509 doesn't work - the `rustbuild` feature _is_ actually used, it was just incorrectly documented here and I missed it.
|
☀️ Test successful - status-appveyor, status-travis |
r? @nikomatsakis
This is the reason that rust-lang/rust#44509 doesn't work - the
rustbuildfeature is actually used, it was just incorrectly documented here and I missed it.