Remove rustc_bitflags; use the bitflags crate #44441
Conversation
|
Why is this adding a dependency on the github bitflags? Are there some unreleased changes we need? |
|
@Mark-Simulacrum bitflags/bitflags#24 is unreleased. |
|
In the past, github dependencies caused problems for vendored builds, #42719. |
|
I'm going to suggest we hold off on merging this until we release bitflags. I don't see a reason to rush; and I'd expect that the release would happen fairly soon (though I guess it would be a breaking change). |
|
@cuviper @Mark-Simulacrum holding off seems good to me. In the meantime, there's a compilation issue here; perhaps you guys could offer some guidance (cc @alexcrichton): the individual crates touched by this change don't compile in the presence of the pre-existing However, removing all the |
|
I've just published bitflags 1.0.0 |
|
@alexcrichton I think I've figured out the linking issue here - it's a vestige of the experimental support for no_std in bitflags. I'm going to open a PR - I think we'll need a 1.0.1. |
carols10cents
added
the
S-waiting-on-author
|
@tamird any luck with the investigation here? |
|
I haven't had a chance to come back to this, but given recent experience in #44515 I'm surprised we don't see the same failure here. In that PR, we saw a failure to locate |
|
This crate isn't depended on before the standard library, so it doesn't need the same treatment as compiler-builtins and liblibc, as to why there's any error at all though I'm not sure. |
|
It appears this has something to do with bits of rustc being built as dylibs vs the bits that are built as rlibs and linked statically. Empirically, I observe this same failure much earlier (during the compilation of Well, with that, I'm not exactly sure how to proceed. |
tamird
force-pushed
the
cargo-bitflags
branch
from
c2cdb55 to
bb94c9f
Compare
|
Ah, I think bitflags is being statically linked into several dylibs, which is against the rules. Seems like a bug in the selection algorithm in librustc/middle/selection_algorithm.rs, since we should be able to build bitflags as a dylib, but we aren't. |
|
Looks like this is exactly #34909 (comment). |
tamird
force-pushed
the
cargo-bitflags
branch
from
bb94c9f to
1524302
Compare
|
To fix this what you want to do is:
For now you can probably just do that with |
|
I've done what you suggested*, but I still get the same error.
* I couldn't "move" bitflags to this crate; each crate that includes
`extern crate bitflags` still needs to depend on bitflags explicitly.
* I haven't made all rustc crates depend on it, only those that depend on
bitflags (for now).
…On Sat, Sep 16, 2017 at 10:13 AM, Alex Crichton ***@***.***> wrote:
To fix this what you want to do is:
- Add a new crate, librustc_cratesio_shim
- Make this crate compile as a dylib
- Move all rustc crates.io dependencies to this crate
- Make sure all rustc crates transitively depend on this crate.
For now you can probably just do that with bitflags rather than all
crates.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#44441 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPPwh3fDnS-jpVkpnoyEji0et4ASBks5si9d8gaJpZM4PRx53>
.
|
|
Er what I mean is that there's a crate |
|
Ah, I forgot to link to link to I understand that this workaround is meant to get rustc to use bitflags as a dylib, but why is it necessary? all the consumers of bitflags here are already crate-type=dylib, so what does this shim do beyond that? I would have expected the shim dylib to include bitflags statically, which would have produced the same problem we're seeing without the shim. What's more, why doesn't this problem manifest with other crates.io dependencies like |
tamird
force-pushed
the
cargo-bitflags
branch
2 times, most recently
from
13a132d to
924eb4e
Compare
|
The reasoning here is... complicated. The tl;dr; is that bitflags iteslf isn't a dylib, it's just an rlib (that's the problem). The compiler only wants one copy of a library in all final outputs, so the only way to do that is to get it into a dylib and then have everything else use it through that dylib. |
tamird
force-pushed
the
cargo-bitflags
branch
from
924eb4e to
5c83018
Compare
|
☔ The latest upstream changes (presumably #44634) made this pull request unmergeable. Please resolve the merge conflicts. |
tamird
force-pushed
the
cargo-bitflags
branch
from
5c83018 to
96a3d27
Compare
|
Thanks for your help! I think this is good to go. |
| # the distribution. This doesn't work normally because: | ||
| # | ||
| # - Cargo always builds dependencies as rlibs: | ||
| # https://github.com/rust-lang/cargo/issues/629 |
There was a problem hiding this comment.
There's not really a bug here, the manifests say they want to get built as rlibs.
There was a problem hiding this comment.
I didn't say it was a bug, but also I'm pretty sure bitflags' manifest doesn't specify one way or the other. https://github.com/rust-lang-nursery/bitflags/blob/master/Cargo.toml
| // except according to those terms. | ||
|
|
||
| #![allow(unused_extern_crates)] | ||
|
|
There was a problem hiding this comment.
Can you add a comment here pointing to the Cargo.toml?
src/librustc/lib.rs
Outdated
| @@ -40,7 +40,11 @@ | |||
|
|
|||
| #![recursion_limit="256"] | |||
|
|
|||
| #[allow(unused_extern_crates)] | |||
There was a problem hiding this comment.
Can you add a comment here pointing to the Cargo.toml of rustc_cratesio_shim?
src/librustc_apfloat/lib.rs
Outdated
| @@ -53,34 +53,30 @@ | |||
| #![cfg_attr(not(stage0), feature(const_min_value))] | |||
| #![cfg_attr(not(stage0), feature(const_max_value))] | |||
|
|
|||
| #[allow(unused_extern_crates)] | |||
| extern crate rustc_cratesio_shim; | |||
There was a problem hiding this comment.
Can you add a comment here pointing to the Cargo.toml of rustc_cratesio_shim?
src/librustc_llvm/lib.rs
Outdated
| @@ -24,10 +24,12 @@ | |||
| #![feature(link_args)] | |||
| #![feature(static_nobundle)] | |||
|
|
|||
| extern crate libc; | |||
| #[allow(unused_extern_crates)] | |||
There was a problem hiding this comment.
Can you add a comment here pointing to the Cargo.toml of rustc_cratesio_shim?
src/librustc_mir/lib.rs
Outdated
| @@ -24,16 +24,18 @@ Rust MIR: a lowered representation of Rust. Also: an experiment! | |||
| #![feature(collection_placement)] | |||
| #![feature(nonzero)] | |||
|
|
|||
| #[allow(unused_extern_crates)] | |||
There was a problem hiding this comment.
Can you add a comment here pointing to the Cargo.toml of rustc_cratesio_shim?
src/librustc_trans/lib.rs
Outdated
| @@ -37,6 +37,11 @@ | |||
| use rustc::dep_graph::WorkProduct; | |||
| use syntax_pos::symbol::Symbol; | |||
|
|
|||
| #[allow(unused_extern_crates)] | |||
There was a problem hiding this comment.
Can you add a comment here pointing to the Cargo.toml of rustc_cratesio_shim?
src/librustc_trans/Cargo.toml
Outdated
| rustc_const_math = { path = "../librustc_const_math" } | ||
| rustc_cratesio_shim = { path = "../librustc_cratesio_shim" } |
There was a problem hiding this comment.
Since this (and a number of other crates) transitively depend on syntax you don't need to link this crate in
There was a problem hiding this comment.
True, but that feels fragile to me. What's the downside to always including this?
There was a problem hiding this comment.
Let's go ahead and remove this b/c otherwise it's just slightly confusing why it's here.
There was a problem hiding this comment.
Seems more confusing to me to omit it; as a naive reader I'd expect this to be present anywhere that bitflags is used.
There was a problem hiding this comment.
Let's omit it. This is just a "hack" and should be as contained as possible.
Fix some lints while I'm here.
tamird
force-pushed
the
cargo-bitflags
branch
from
96a3d27 to
edbe8bb
Compare
src/librustc/Cargo.toml
Outdated
| rustc_const_math = { path = "../librustc_const_math" } | ||
| rustc_cratesio_shim = { path = "../librustc_cratesio_shim" } |
There was a problem hiding this comment.
I think because this depends on libsyntax this also isn't needed
There was a problem hiding this comment.
(copied from the other comment) True, but that feels fragile to me. What's the downside to always including this?
src/librustc_mir/Cargo.toml
Outdated
| graphviz = { path = "../libgraphviz" } | ||
| log = "0.3" | ||
| rustc = { path = "../librustc" } | ||
| rustc_const_eval = { path = "../librustc_const_eval" } | ||
| rustc_const_math = { path = "../librustc_const_math" } | ||
| rustc_cratesio_shim = { path = "../librustc_cratesio_shim" } |
There was a problem hiding this comment.
This depends on libsyntax, so this isn't needed
There was a problem hiding this comment.
(copied from the other comment) True, but that feels fragile to me. What's the downside to always including this?
tamird
force-pushed
the
cargo-bitflags
branch
2 times, most recently
from
985f69c to
0eb4df1
Compare
tamird
force-pushed
the
cargo-bitflags
branch
from
0eb4df1 to
231d9e7
Compare
|
@alexcrichton I think this is ready to go. |
|
@bors: r+ |
|
📌 Commit 231d9e7 has been approved by |
|
⌛ Testing commit 231d9e7 with merge 8696486bcad2a94eeb25271b361a264acff0ac56... |
|
💔 Test failed - status-travis |
|
Spurious musl error. |
Remove rustc_bitflags; use the bitflags crate r? @alexcrichton
|
☀️ Test successful - status-appveyor, status-travis |
r? @alexcrichton