std: Depend directly on crates.io crates #56092
Merged
+231
−314
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? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @rust-lang/libs, y'all are likely interested in this! |
|
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 |
Does this raise the minimum Rust version required by libc? |
There was a problem hiding this comment.
Overall I think I'm not a fan of this approach because it requires modifying crates.io crates and bumping their minimum version of Rust to 1.31.
Could we perhaps inject the dependency via an unstable Cargo feature? I'm okay with landing the groundwork here for now, though I don't think it's long term viable...
src/libstd/Cargo.toml
Outdated
| profiler_builtins = { path = "../libprofiler_builtins", optional = true } | ||
| unwind = { path = "../libunwind" } | ||
|
|
||
| [dev-dependencies] | ||
| rand = "0.5" | ||
| #rand = "0.5" |
There was a problem hiding this comment.
I suspect this is because uncommenting rand here causes problems down the line?
There was a problem hiding this comment.
Ah sorry forgot to add a comment about this. This is a temporary holdover until rust-random/rand#647 is merged, published, and integerated here.
A good question!
Don't worry, as the maintainer of The libc-specific patch is much different than the patch applied to |
To be clear, we have basically no other candidates for adding as a dependency to the standard library in the near future. Crates added here are often nightly and unstable, and In any case all crates affected here are barely used on crates.io (modulo libc). I would like to land this to pave the way for using crates in the future, even if crates we want to use don't want to require 1.31 yet. |
|
Okay - yeah, the version requirement matters less to me than three general approach of pushing this out into crates.io but I agree that the set of crates is limited enough for now that this is the practical decision. |
alexcrichton
force-pushed
the
no-more-std-subodules
branch
from
56231b3
to
414b26f
Compare
|
@Mark-Simulacrum hm sorry I may be misunderstanding, but do you think we should not pursue a change like this? |
|
I think this is awesome, but I worry about the ergonomics. @alexcrichton IIUC, we only need to add hacks for this to crates in crates.io that we want to use from |
|
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 |
|
@gnzlbg can you elaborate on the ergonomics point? To be clear we have very few dependencies of the standard library, and we need to be extremely careful about picking up dependencies in libstd because those crates are shipped with basically all code Rust builds against. To that end the fact that anything at all is required to change to get used by libstd seems reasonable, and the 4-5 lines in |
|
@alexcrichton I agree with everything you said. With ergonomics I meant that IMO the place to be careful about what goes into libstd is libstd's But this works, adding these lines is not much extra work and I agree that as you said this won't happen often. Finding a better way to do this might not be a problem worth solving, and it should definitely not block this PR. |
|
Heh the Cargo.toml of libstd and other crates in rust-lang/rust are already a comedy of workarounds, so that's sort of the least of my worries. I also feel like none of us can say with a straight face that |
That's true, at least this workaround is consistent for all crates involved. |
|
@alexcrichton I think we should go ahead with this; I was suggesting that there might be a better solution somewhere down the line, I don't think there's any point in waiting though. (Edit: to be clear r=me with the relevant PRs filed/merged in upstream crates) |
alexcrichton
added a commit
to alexcrichton/compiler-builtins
that referenced
this pull request
Nov 21, 2018
This commit prepares to publish the compiler-builtins crate to crates.io in order for the standard library to directly depend on it from crates.io in rust-lang/rust#56092
alexcrichton
added a commit
to alexcrichton/dlmalloc-rs
that referenced
this pull request
Nov 21, 2018
alexcrichton
added a commit
to alexcrichton/libc
that referenced
this pull request
Nov 21, 2018
This commit prepares the `libc` crate to be included directly into the standard library via crates.io. More details about this can be found on rust-lang/rust#56092, but the main idea is that this crate now depends on core/compiler-builtins explicitly (but off-by-default). The main caveat here is that this activates `no_core` when building as part of libstd, which means that it needs to explicitly have an `iter` and `option` module for the expansion of `for` loops to work.
alexcrichton
added a commit
to alexcrichton/libc
that referenced
this pull request
Nov 21, 2018
This commit prepares the `libc` crate to be included directly into the standard library via crates.io. More details about this can be found on rust-lang/rust#56092, but the main idea is that this crate now depends on core/compiler-builtins explicitly (but off-by-default). The main caveat here is that this activates `no_core` when building as part of libstd, which means that it needs to explicitly have an `iter` and `option` module for the expansion of `for` loops to work.
alexcrichton
added a commit
to alexcrichton/libc
that referenced
this pull request
Nov 21, 2018
This commit prepares the `libc` crate to be included directly into the standard library via crates.io. More details about this can be found on rust-lang/rust#56092, but the main idea is that this crate now depends on core/compiler-builtins explicitly (but off-by-default). The main caveat here is that this activates `no_core` when building as part of libstd, which means that it needs to explicitly have an `iter` and `option` module for the expansion of `for` loops to work.
|
Is there a reason stdsimd is still a submodule? |
Why wouldn't it be? Makes it easy to test it, implement new features, reduces testing load (no need to run stdsimd tests in CI, no need to test every single stdsimd PR, etc.) in rust-lang/rust, etc. |
|
All those arguments also apply to all the submodules that were removed by the PR. |
|
Ah, I see what you mean. The |
Merged
|
@jethrogb I suspect we could build |
|
er sorry, just ignore what I said and it's all what @gnzlbg said |
I think we should do that. Some people have expressed interest into using the same run-time feature detection as (I can prepare the |
|
Oh good point about the runtime feature detection, seems plausible to me to game that out and see what it would look like! One of the main points of being on crates.io though is offering a stable interface, and I do fear a bit that providing a stable interface might be hard especially for the stability attributes and all that... Seems fine to try though! |
bors
added a commit
that referenced
this pull request
Dec 14, 2018
Update Clippy Hopefully unbreaks toolstate: #56092 (comment)
This was referenced Dec 14, 2018
alexcrichton
added a commit
to alexcrichton/rust
that referenced
this pull request
Dec 14, 2018
This was an accidental regression from rust-lang#56092, but for `no_std` targets being built and distributed we want to be sure to activate the compiler-builtins `mem` feature which demangles important memory-related intrinsics.
bors
added a commit
that referenced
this pull request
Dec 15, 2018
std: Activate compiler_builtins `mem` feature for no_std targets This was an accidental regression from #56092, but for `no_std` targets being built and distributed we want to be sure to activate the compiler-builtins `mem` feature which demangles important memory-related intrinsics.
14 tasks
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.
Ever since we added a Cargo-based build system for the compiler the
standard library has always been a little special, it's never been able
to depend on crates.io crates for runtime dependencies. This has been a
result of various limitations, namely that Cargo doesn't understand that
crates from crates.io depend on libcore, so Cargo tries to build crates
before libcore is finished.
I had an idea this afternoon, however, which lifts the strategy
from #52919 to directly depend on crates.io crates from the standard
library. After all is said and done this removes a whopping three
submodules that we need to manage!
The basic idea here is that for any crate
stddepends on it adds anoptional dependency on an empty crate on crates.io, in this case named
rustc-std-workspace-core. This crate is overridden via[patch]inthis repository to point to a local crate we write, and that has a
pathdependency on libcore.Note that all
no_stdcrates also depend oncompiler_builtins, but ifwe're not using submodules we can publish
compiler_builtinstocrates.io and all crates can depend on it anyway! The basic strategy
then looks like:
The standard library (or some transitive dep) decides to depend on a
crate
foo.The standard library adds
The crate
foohas an optional dependency onrustc-std-workspace-coreThe crate
foohas an optional dependency oncompiler_builtinsThe crate
foohas a featurerustc-dep-of-stdwhich activates thesecrates and any other necessary infrastructure in the crate.
A sample commit for
dlmallocturns out to be quite simple.After that all
no_stdcrates should largely build "as is" and still bepublishable on crates.io! Notably they should be able to continue to use
stable Rust if necessary, since the
rename-dependencyfeature of Cargois soon stabilizing.
As a proof of concept, this commit removes the
dlmalloc,libcompiler_builtins, andlibcsubmodules from this repository. Longthorns in our side these are now gone for good and we can directly
depend on crates.io! It's hoped that in the long term we can bring in
other crates as necessary, but for now this is largely intended to
simply make it easier to manage these crates and remove submodules.
This should be a transparent non-breaking change for all users, but one
possible stickler is that this almost for sure breaks out-of-tree
std-building tools likexargoandcargo-xbuild. I think it shouldbe relatively easy to get them working, however, as all that's needed is
an entry in the
[patch]section used to build the standard library.Hopefully we can work with these tools to solve this problem!