Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`std` Aware Cargo #2663

Open
wants to merge 14 commits into
base: master
from

Conversation

@jamesmunns
Copy link
Member

jamesmunns commented Mar 16, 2019

Rendered.

Please see Pre-RFC discussion here.

Note: I'm happy to squash/rebase out the multiple commits below. I have included them as they were part of the Pre-RFC discussion.

@jamesmunns

This comment has been minimized.

Copy link
Member Author

jamesmunns commented Mar 16, 2019

@comex

This comment has been minimized.

Copy link

comex commented Mar 16, 2019

Regarding "stable features"... I think that might be better thought of as bringing Cargo features to std/core, which are pretty much an entirely different concept from #[feature] features, other than the name. In fact, what needs to be implemented is the opposite: a way to mark Cargo features as unstable.

See also: rust-lang-nursery/api-guidelines#95 ("Determine how crates should expose 'unstable' APIs")

Anyway, that's just a matter of semantics.

## Should we allow configurable `core` and `std`
If we are to uphold stability guarantees for all configurations of `core` and `std`, this could require testing 2^(n+m) versions of Rust, where `n` is the number of `core` features, and `m` is the number of `std` features. This would have a negative impact on CI times.

This comment has been minimized.

Copy link
@Centril

Centril Mar 17, 2019

Contributor

Our CI times are already quite stretched; I think bors is the bottleneck overall in our development processes. This would also require 2^(n+m) for reasonably many platforms, not just for one.

If core & std (and probably alloc) are to be configurable, it should, for the foreseeable future be exclusively limited to removing things from the standard library, rather than changing algorithms and whatnot.

This comment has been minimized.

Copy link
@aep

aep Mar 17, 2019

if rust opens to embedded, we can contribute plenty CI machines.

This comment has been minimized.

Copy link
@Centril

This comment has been minimized.

Copy link
@kennytm

kennytm Mar 17, 2019

Member

We're certainly not going to test all 2m+n combinations on the CI, this is like mindlessly aiming for 100% code coverage. Having one test for all features disabled and one for all features enabled is more than enough.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Mar 20, 2019

Contributor

Having one test for all features disabled and one for all features enabled is more than enough.

In my experience that's rarely the case - features interact with each other in weird ways.

IMO 100% coverage is impossible, but that should not be the goal. The goal should be to get as close to 100% coverage as possible while using a tiny fraction of resources.

I don't think we can achieve that goal by using fixed rules (like testing everything, or testing just A and B). Achieving the goal is going to require investing time into evaluating which features and features groups makes sense to tests where.

For example, it might make sense to test more combinations on the x86_64 tier 1 targets only, and it might also make sense to set up a weekly cron job that tests even more combinations, random combinations, etc. But which features should be tested in isolation and which ones can be grouped in the tests with other features is something that we should evaluate and constantly re-evaluate as new features get added on a 1:1 basis.

This comment has been minimized.

Copy link
@kennytm

kennytm Mar 20, 2019

Member

@gnzlbg Cargo features are supposed to be additive, features interacting within libstd is already a bug IMO.

For sure if it turns out A+C+F has weird behavior compared with no features and A+B+C+D+E+F+G features, we could create a special run-make test to guarantee that special behavior in A+C+F.

Another option in this area is to force the use of profile overrides, as specified by [RFC2822](https://github.com/rust-lang/rfcs/blob/master/text/2282-profile-dependencies.md).
## Should providing a custom `core` or `std` require a nightly compiler?

This comment has been minimized.

Copy link
@Centril

Centril Mar 17, 2019

Contributor

Yes, I think this should be considered a requirement. We cannot reasonably have stability and at the same time allow people to do this with with a stable compiler.

This comment has been minimized.

Copy link
@phil-opp

phil-opp Mar 17, 2019

But isn't this already possible on stable? Just specify a core/std dependency in your Cargo.toml to override the default. Rust even automatically imports the prelude of the custom std.

We use this approach in our stm32f7-discorvery crate to augment the core library with the required Future implementations for using async/await on no_std.

This comment has been minimized.

Copy link
@Centril

Centril Mar 17, 2019

Contributor

Ugh... @alexcrichton ^-- Was this intended and who intended it?

(the Future link is dead)

This comment has been minimized.

Copy link
@phil-opp

phil-opp Mar 17, 2019

Ah sorry, fixed it. Basically we provide our own implementation for the parts of std::future that currently use thread local storage (including the await macro).

This comment has been minimized.

Copy link
@Nemo157

Nemo157 Mar 17, 2019

Contributor

Also, part of the discussion on the pre-RFC was that overriding std/core on a stable compiler should be fine because actually implementing std/core will require using unstable features that will require a nightly compiler.

EDIT: and if in the future it’s possible to provide an implementation of std/core without using any unstable features, why should the act of switching your dependencies to it require a nightly compiler?

This comment has been minimized.

Copy link
@jamesmunns

jamesmunns Mar 17, 2019

Author Member

EDIT: and if in the future it’s possible to provide an implementation of std/core without using any unstable features, why should the act of switching your dependencies to it require a nightly compiler?

@Nemo157 I did mention that later on in the RFC, that I am of the opinion that IF you can develop your own core and/or std without "magic" features (which are required for things like box and format!), you should be able to use a stable compiler.

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Mar 17, 2019

Contributor

@Centril What exactly are you worried about? std and core are just names that happen to usually be bound to libraries that use unstable features. If you want to override them with other features then you don't have to pay for those features. I can imagine that std-aware cargo is itself an unstable feature. But then the instability of overriding std/core is not a rustc concern, but a cargo concern: the instability is not the std and core are special and shouldn't be overridden, but that the way cargo adds implicit std/core dependencies is unstable.

In particular, I can imagine a situation where customary std and core still use unstable features, but it is possible to make a binary without using those unstable features. If the Cargo implicit stdlib deps mechanism is stable by then, then I think someone should be able to swap std and core out for stripped-down all stable code, even though building customized versions regular std still requires a nightly rustc.

This comment has been minimized.

Copy link
@phil-opp

phil-opp Mar 17, 2019

@Centril

EDIT: and if in the future it’s possible to provide an implementation of std/core without using any unstable features, why should the act of switching your dependencies to it require a nightly compiler?

When that happens and you don't need to set any lang items then that changes the equation entirely.

Creating a stripped-down std library on top of core and alloc might be possible on stable Rust soon (when alloc is stabilized). It would only work with no_main binaries and wouldn't contain all std features, but I don't think that it's problem that some crates would not build with it. In fact, it might be even preferable for environments like WASM, where e.g. threading is unavailable and accidentally using a crate that uses threads throws a runtime error today.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Mar 22, 2019

Member

FWIW My thinking here is that Cargo simply won't do anything special in the long run when you're overriding core/std with a custom source. If it's compiling the sysroot std/core Cargo would enable unstable features manually for stable compilers (aka set the RUSTC_BOOTSTRAP env var). Otherwise if you crates can finagle some way to compile on stable but still override libstd, then more power to them! (aka you link to the real std and maybe wrap some of its functionality).

So in that sense I think Cargo, when compiling a custom core/std, just wouldn't do anything. If you use a bunch of unstable features that'll naturally require a nightly compiler, and if not then it's all stable anyway!

This comment has been minimized.

Copy link
@jethrogb

jethrogb Mar 22, 2019

Contributor

Otherwise if you crates can finagle some way to compile on stable but still override libstd, then more power to them! (aka you link to the real std and maybe wrap some of its functionality).

Should cargo provide a way to use the non-patched dependency from the patched one?


When compiling for a non-standard target, users may specify their target using a target specification file, rather than a pre-defined target.

> NOTE: The current target specification is described in JSON, and contains some

This comment has been minimized.

Copy link
@Centril

Centril Mar 17, 2019

Contributor

In jamesmunns#1 (comment) you noted that we should be "blind" to this format and what it contains and that stabilizing could be done later.. My question is then: how can we reasonably change anything if people start relying on the format on stable? Stability is not just about what you say, it must also be practical.

With my language team hat on (but speaking for myself only) it is important that we be able to feasibly use other backends than LLVM. It should not just be a theoretical possibility to use Cranelift or some other backend, but practically possible in cargo for e.g. debug builds or whatnot.

I would suggest embracing the otherwise incremental approach of this RFC where we start with other things first and let custom target specs be unstable until we are confident that stabilizing won't cause headaches wrt. backends.

Meanwhile we can also do what should be a straightforward switch to TOML.

This comment has been minimized.

Copy link
@IsaacWoods

IsaacWoods Mar 17, 2019

+1 for the switch to TOML as a first step.

However, I am very hesitant of the idea of stabilising the current custom target format. While there is an RFC describing the format, it feels very much like an unstable implementation detail at the moment, and I've personally put up with many problems with it because it's not meant to be a permanent solution and is not a priority at the moment; I wonder if this reflects other people's thoughts.


On alternative backends: if we do end up stabilising this, the minimum change I think we'd need to make would be to split some options into a backend-options map (like has been done with pre-link-args, for example). For example, you'd be required to write (in the current JSON format):

"backend-options": {
    "llvm": {
        "data-layout": "e-m:e-i64:64-f80:128-n8:16:32:64-S128",
        "target": "x86_64-unknown-none",
    }

    {{other backends could then be added with backwards compatibility here}}
}

You could then only build a target with a specific backend if it has a corresponding entry in the map.

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Mar 17, 2019

Contributor

@Centril being blind to something is just parametricity :). Only rustc cares about the format, cargo need not care about the format at all. What it does need to do however is be able to tell if two machine configs are equal, since Cargo controls caching. So we need a:

abstract type MachineConfig: Eq;

Cargo can forward the contents as raw bytes to rustc (as a CLI arg, or temp file Cargo creates to avoid races, or many other things). As to equality, Cargo can take the hash of the config and use that as a cache key. Sure, this is never complete (concrete formats usually have a more flexible notion of equality), but it is always sound. That's just fine for now.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Mar 17, 2019

This one was a bit tricky to assign teams to... but:

  • T-cargo -- the cargo aspect of this.
  • T-compiler -- re. support in the compiler itself and target specs.
  • T-lang -- re. the custom target stuff since that has implications wrt. what backends are feasibly possible and not and re. stability... I've added this team in an interim capacity; may change later.

(that's a lot of teams but I expect one team to take the lead and others to review... it may also change...)

@ehuss
Copy link
Contributor

ehuss left a comment

(Implementation detail) I suspect there will need to be special considerations for proc macros and build scripts. My guess is that proc macros will be sensitive to having a different libstd, and may need to only be built with the libstd binaries from the rustc sysroot. This might be quite expensive for some projects.

Show resolved Hide resolved text/0000-std-aware-cargo.md Outdated
Currently, `compiler-builtins` contains components implemented in the C programming language. While these dependencies have been highly optimized, the use of them would require the builder of the root crate to also have a working compilation environment for compilation in C.
This RFC proposes instead to use the [pure rust implementation] when compiling for a custom target, removing the need for a C compiler.

This comment has been minimized.

Copy link
@ehuss

ehuss Mar 18, 2019

Contributor

Sorry if this is a dumb question, would backtrace also require building a C library?

This comment has been minimized.

Copy link
@jamesmunns

jamesmunns Mar 18, 2019

Author Member

Definitely not a dumb question, I hadn't thought of this. @alexcrichton is there a pure Rust version of libbacktrace? Or are there any other C-dependencies you are aware of that would also need to be listed here?

This comment has been minimized.

Copy link
@kennytm

kennytm Mar 18, 2019

Member

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Mar 18, 2019

Member

It's correct yeah that libbacktrace requires a C compiler right now, and while all the pieces exist in basic forms (e.g. gimli, addr2line exmples for gimli, etc) in only-Rust they haven't been integrated in such a way yet to get pulled into the standard library. (the issue @kennytm pointed out is tracking that)

## Should profile changes always prompt a rebuild of `core`/`std`?
For example, if a user sets their debug build to use `opt-level = 'z'`, should this rebuild `core`/`std` to use that opt level? Or should an additional flag, such as `apply-to-sysroot` be required to opt-in to this behavior, unless otherwise needed?

This comment has been minimized.

Copy link
@ehuss

ehuss Mar 18, 2019

Contributor

I'm a little confused on this point. Would it rebuild core/std even if they are not listed in [dependencies] or [patch]? I fear that always rebuilding when using non-default profile settings would be too much disruption. It would also be a little confusing, because there are multiple profiles, none of which match the settings used in the precompiled libraries. Perhaps core/std should only be rebuilt when they are explicitly listed in Cargo.toml?

Another potential issue is that the features to enable for libstd currently are driven by bootstrap's config.toml, so it may not be obvious to the user that they need to enable things like "backtrace" to have feature parity with the defaults (which change per platform). How do you switch to a custom-built std that retains feature parity with the default distribution per platform?

This comment has been minimized.

Copy link
@jamesmunns

jamesmunns Mar 18, 2019

Author Member

That is exactly the open question here. On one hand: if the user states they want "opt-level = 's'", and we can now give them that for libstd too, it would make sense to keep the total program size down.

On the other hand, this could surprise some users, as it could drastically increase a clean build time by rebuilding core and standard for them.

Good call regarding config.toml, I was not aware of how this mechanism worked. In xargo, you can set some (or all?) of these flags using a feature = [ ... ] syntax. I would guess we would need to expose these features as "Cargo Features" rather than config.toml features so the users may configure them, with default-features matching what the CI builds of libstd and libcore currently are.

This comment has been minimized.

Copy link
@ehuss

ehuss Mar 18, 2019

Contributor

I think they are "Cargo Features", config.toml just drives the defaults. This is done here.

One thing I'm aware of that is not driven via features, and that is sanitizer support. That is done here. It looks like sanitizers are used on the linux builds, but I don't really know anything about them. Looks like it requires llvm?

Maybe the defaults could be captured somewhere when the distribution is made, and Cargo could read those (maybe in the "src" component? maybe rewrite the Cargo.toml?)? EDIT: 🤔 Except that won't work for non-host targets.

This comment has been minimized.

Copy link
@Ericson2314

Ericson2314 Mar 18, 2019

Contributor

It's a general question whether any dependency needs to be rebuilt for these non-abi-affecting switches, so as usual I hope for a Cargo solution that doesn't special-case core and std.

Show resolved Hide resolved text/0000-std-aware-cargo.md Outdated
Show resolved Hide resolved text/0000-std-aware-cargo.md Outdated
Show resolved Hide resolved text/0000-std-aware-cargo.md Outdated
Show resolved Hide resolved text/0000-std-aware-cargo.md Outdated

shepmaster and others added some commits Mar 18, 2019

Apply suggestions from code review
Co-Authored-By: jamesmunns <james@onevariable.com>
Update text/0000-std-aware-cargo.md
Co-Authored-By: jamesmunns <james@onevariable.com>
Apply suggestions from code review
Co-Authored-By: jamesmunns <james@onevariable.com>
@jethrogb

This comment has been minimized.

Copy link
Contributor

jethrogb commented Mar 18, 2019

Is it necessary to introduce the concept of "stable features" in this RFC? Can this be in a separate RFC from the building/source specification?

I brought this up in the pre-RFC: I think the RFC should say something about the current [patch.crates-io] section in Rust's Cargo.toml.

What about the test crate?

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Mar 18, 2019

@ehuss

(Implementation detail) I suspect there will need to be special considerations for proc macros and build scripts. My guess is that proc macros will be sensitive to having a different libstd, and may need to only be built with the libstd binaries from the rustc sysroot. This might be quite expensive for some projects.

That's just a cross compiling concern. I'm strongly of the opinion that one should always be cross compiling, and native compiling is just cross compiling where the platform you are building for and the platform you're building on happen to be the same. Under this philosophy, it should be possible to do [path.host.crates-io] and only affect regular dependencies, not build.rs or proc macro dependencies.

(I'm strongly of this opinion having previously refactored two existing build tools to adopt this philosophy, even though there initially didn't at all. It's not too late for Cargo either.)

In today's Rust environment, `core` and `std` are shipped as precompiled objects. This was done for a number of reasons, including faster compile times, and a more consistent experience for users of these dependencies. This design has served the bulk of users fairly well. However there are a number of less common uses of Rust, that are not well served by this approach. Examples include:

* Supporting new/arbitrary targets, such as those defined by a custom target (".json") file
* Modifying `core` or `std` through use of feature flags

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Mar 20, 2019

Contributor

Cargo feature flags? or compiler feature flags (-C target-cpu, -C target-feature, --cfg foo, etc. ?) Or both?

It is necessary to use unstable features to build `core`. To allow users of a stable compiler to build `core`, we would set the `RUSTC_BOOTSTRAP` environment variable **ONLY** for the compilation of `core`.
This should be considered sound, as stable users may not change the source used to build `core`, or the features used to build `core`.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Mar 20, 2019

Contributor

Why wouldn't this also be sound for crates on crates.io (or for alloc and libstd) ?

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Mar 20, 2019

Contributor

Also maybe mention that even when compiled with RUSTC_BOOTSTRAP unstable features from core are not available to stable users.

In general, the same restrictions for building `core` will apply to building `std`. These include:
* Users of the stable compiler must use the source used to build the current Rust compiler
* Only compile time features considered `stable` may be used outside of nightly. Initially the list of `stable` features would be empty, and stabilizing these features would require a PR/RFC to `libstd`.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Mar 20, 2019

Contributor

We would have to support these stable features forever, so each feature should go through the RFC process. A PR + mini FCP is not an option for this in my opinion.

By using stable feature flags for `std`, we could say that `std` as a crate with `default-features = false` would essentially be `no_core`, or with `features = ["core"]`, we would be the same as `no_std`.
This abstraction may not map to the actual implementation of `libcore` or `libstd`, but instead be an abstraction layer for the end developer.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Mar 20, 2019

Contributor

Note that we would still need to allow using [dependency.core] forever and somehow map that to an unified version of the library.

Or in other words: removing the concept of core and std (e.g. into an unified std that uses the portability lint) would be a breaking change if this RFC was merged as is.

I don't how hard would it be to have an unified library, that's provided as a "split" one simultaneously (or in an edition dependent way) for backwards compatibility.

This comment has been minimized.

Copy link
@Nemo157

Nemo157 Mar 20, 2019

Contributor

It seems like this could easily be dealt with by making core (and alloc) a facade over std with a limited set of default active features. As long as the current core -> std dependency order is not observable somehow.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 22, 2019

One of the things I'm worried about is the trigger that causes Cargo to build libstd/libcore/etc. The RFC currently says that only the root crate can do it and it happens when a custom target is used, feature flags are modified, profile settings change, or [patch.sysroot] is present. Some questions I'd have are:

  • What happens though when crates on crates.io change feature flags of core/std?
  • How does Cargo know what to build? Does it always try to build everything up to std? When does it only build core (and maybe alloc)?
  • I agree with @ehuss that changing any profile setting causing a build of std/core may be a bit heavy handed because there's lots of tweaking of profiles right now that probably don't want to build std/core (although there's surely some that do want a recompile)

I'm a little uneasy about how we're going to handle compiler_builtins, but that crate is the embodiment of "just keep everything unstable and somehow get everything to work", so I think it's fine to leave basically all details related to compiler_builtins to the implementation. It may cause issues but they're likely not too hard to overcome.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Mar 23, 2019

How does Cargo know what to build? Does it always try to build everything up to std? When does it only build core (and maybe alloc)?

Hmm there should be some language in the RFC saying by default crates are assumed to depend on std and core, but if you explicitly list one of those as a dependency, you get no other implicit dependencies. That way, explicitly depending on core (or core + alloc) means no implicit dependency on std.

Then Cargo simply builds the dependencies which are needed per the normal rules.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Mar 23, 2019

What happens though when crates on crates.io change feature flags of core/std?

If creates there are allowed to [patch.*] today, then I have no problem allowing them to patch core and std. Likewise for non-root crates regardless of source/registry. Making this stuff unstable Cargo features is an independent concern.

@newpavlov

This comment has been minimized.

Copy link

newpavlov commented Mar 25, 2019

I personally prefer several user facing crates (core, alloc, collections, etc.) over a single std crate with a lot of features. And I think it will be nice to eventually stop shipping pre-compiled std and core, as it will be quite useful for issues like this one: rust-lang/rust#51713

Also I think we should keep in mind PAL proposal: https://internals.rust-lang.org/t/4301

cuviper added a commit to cuviper/rust that referenced this pull request Mar 30, 2019

manifest: only include miri on the nightly channel
miri needs to build std with xargo, which doesn't allow stable/beta:
<japaric/xargo#204 (comment)>

Therefore, at this time there's no point in making miri available on any
but the nightly channel.  If we get a stable way to build `std`, like
[RFC 2663], then we can re-evaluate whether to start including miri,
perhaps still as `miri-preview`.

[RFC 2663]: rust-lang/rfcs#2663

Centril added a commit to Centril/rust that referenced this pull request Mar 30, 2019

Rollup merge of rust-lang#59544 - cuviper:miri-nightly, r=Centril
manifest: only include miri on the nightly channel

miri needs to build std with xargo, which doesn't allow stable/beta:
<japaric/xargo#204 (comment)>

Therefore, at this time there's no point in making miri available on any
but the nightly channel.  If we get a stable way to build `std`, like
[RFC 2663], then we can re-evaluate whether to start including miri,
perhaps still as `miri-preview`.

[RFC 2663]: rust-lang/rfcs#2663
@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Apr 19, 2019

What is the status of this?

@jamesmunns

This comment has been minimized.

Copy link
Member Author

jamesmunns commented Apr 19, 2019

@mark-i-m this is probably due an update to the RFC to take the feedback from the comments, and summarize the open discussion points. Unfortunately I will not have time to address this until early May, but I do plan to continue pushing for this then.

As far as I know, there have not been any critical issues raised, though there are a number of open points that need to be addressed or discussed further.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Apr 19, 2019

I have been lax in responding to some points made in this RFC; I hope to change that soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.