Revival of 'Compile core crates before std'

[Aaron1011]

UPDATE: My Hashbrown PR has been merged - this PR now depend on hashbrown master. I assume that there will need to be a new hashbrown release before this PR can be merged.

This is a revival of PR #51440. I've squashed the original commits into one commit (with @est31's authorship preserved), and added my changes as new commits.

As in the original PR #51440, this PR compiles the 'core' crate before 'std', allowing libstd to depend on #![no_std] without any other modifications.

Overview of the changes in this pr:

• Changes to rustbuild to support building libcore separately. Here, I tried to stay close to the original PR, while making the necesasry changes to get it compiling with changes that have occured to rustbuild since the original PR.

• The introduction of a new 'compiler_builtins_shim' crate. This crate allows us to have a single 'core' target in rustbuild, which is responsible for building both libcore and compiler_builtints. This crate works as follows:

  compiler_builtins_shim is a no_core crate, which prevents a core dependency from being automatically injected. It depends on compiler_builtins, enabling the rustc-dep-of-std feature. This causes compiler_builtins to depend on the virtual 'rustc-std-workspace-core', which gets patched to point to our local 'libcore'.

  This somewhat complicated setup accomplishes the following:

  • 'libcore' and 'compiler_builtins' are both made available in the sysroot. This means that external no_std crates need not depend on them if we want to use them in 'libstd' - we can use such crates without any additional changes.
  • We can forward the 'compiler-builtins-mem' and 'compiler-builtins-c' from 'rustbuild' into the 'compiler_builtins', based on the configuration for the build.

• A slight modification to 'hashbrown'. Unlike all of libstd's other dependencies, hashbrown actually changes its public API when its 'rustc-dep-of-std' is enabled - it exports some internal types. In order to make hashbrown work with this new setup, I've submitted a separate PR to add a new rustc-internal-api. This feature only enables the export of the private types - it doesn't add any new dependencies.

• Now that 'libcore' and 'compiler_builtins' are always available, several internal rust crates can stop depending on them. In fact, it's required for these crates to no longer depend on compiler_builtins - otherwise, we'll get errors about duplicate lang items. This is a breaking change for any crates in the rust ecosystem which depend on compiler-builtins. However, compiler-builtins is nightly-only, so this should be acceptable.

A few other notes:

• I've temporary disabled a check in tidy to get this PR to build. Once the hashbrown PR is merged, the change can be removed.
• My hashbrown PR is submitted against the master branch, so this required bumping to the as-of-yet-unreleased hashbrown 0.5.0. I've made the necessary change in src/libstd/collections/hash/map.rs - however, I'm not quite sure what the producure for updating hashbrown is. Does this PR need to be blocked on a new release of hashbrown?
• Once this PR is merged, we can remove the 'rustc-dep-of-std' features from 'backtrace', 'cfg-if', and other crates. This can be done at any time - since rustc will no longer use that feature, it doesn't matter whether or not it continuous to exist.

[rust-highfive]

r? @nikomatsakis

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

[Aaron1011]

This PR is now ready for review.

[Aaron1011]

I think it would be useful to get a try run, so that we can quickly identify any linker-related issues that might require changing the design of this PR.

[nikomatsakis]

I'm going to make this

r? @alexcrichton

because I think i'm the wrong choice, but I'm not sure who is the right choice.

[alexcrichton]

I personally do not think we should pursue this change/refactoring of rustbuild. The current setup of rustbuild is bad enough having to invoke Cargo three times for one build of the compiler, and we've always wanted to get that down to one invocation of Cargo. Adding yet another invocation seems like a step backwards.

It's not entirely clear to me what the motivation of this PR is but I'm sort of inferring that the desire is to pull in crates from crates.io into libstd without modifications. While a laudable goal I don't think is really that necessary. We need to be extremely careful about what libstd depends on since it goes into every single Rust program by default. Having a slight speed bump of "add a few lines to Cargo.toml" feels to me like it's not really a showstopper in any way.

Are there other motivations as well though?

[est31]

I can't speak for @Aaron1011 but I've made #51440 because of those additional lines. I consider them to be a blocker for gimli based std backtraces because of its large number of dependencies. See my comment here for a list.

Being careful about what libstd depends on is a great idea, but I think it's a bad idea to do this via extra lines in Cargo.toml of upstream projects:

1. This code is exposed to every user of those crates, it's not even hidden or something. For those people, these flags are just noise.
2. Such stuff is not needed for any of the C dependencies of rustc. I'd be sceptical whether LLVM would accept a specific configure flag that sets some dep-of-rustc specific things. They would probably accept a generic flag that does the equivalent thing e.g. function name prefixing with a user-specified name but I doubt they'd accept a flag hardcoded to the rustc_ prefix.
3. It's bad for new contributors who are accustomed to just using stuff from crates.io and now have to add these weird lines. To me it seems that this is what evolution of rustbuild has been about: making the developer experience as similar to normal Rust as possible.
4. I agree that being careful about what std depends on is important, but for that you can review Cargo.lock changes when PRs are being reviewed. If you don't consider this practical because of Cargo.lock noise, one can think about adding a whitelist of allowed crates and reviewing changes of that.

As for your point about the additional Cargo invocation, I think it's an okay price to pay. However, I'd be okay with other approaches as well that remove the requirement of upstream to add any custom code to their Cargo.toml or anywhere else. E.g. adding a special Cargo flag to libcore, licompiler_builtins, etc. that adds them as dependencies of any crate that they don't depend on themselves. I think @Mark-Simulacrum and I have considered it as well.

Ultimately I hope that one day rustbuild can be removed, Cargo is std-aware and can manage everything on its own, and rustc is just another "ordinary" codebase. I assume that this is similar to your goals @alexcrichton , and for me this PR is definitely a step into that direction. I hope I could convince you that this change is an improvement :).

[Mark-Simulacrum]

I will note that I used to consider something like this more tenable, but no longer think it's worth the additional code in rustbuild. Each Cargo invocation adds pain to rustbuild's maintenance -- at least today -- and the bloat in downstream crates isn't actually all that bad. I don't think adding e.g. gimli would be viable in today's world (or, realistically, with this PR), without reducing the number of dependencies.

[alexcrichton]

Er yeah @Mark-Simulacrum is right, it's not like this is the only thing (or even a thing) blocking gimli in the backtrace crate, the issues there go much deeper. It's best to discuss those on the backtrace crate itself.

This code is exposed to every user of those crates, it's not even hidden or something. For those people, these flags are just noise.

While I agree with this I'm not personally that sympathetic to it. We're talking maybe 10 crates at most in the entire Rust ecosystem (out of the thousands on crates.io).

Such stuff is not needed for any of the C dependencies of rustc

I'm not really sure how this is relevant? We don't pull in C dependencies through Cargo, and the whole point of the integration here is to pull in crates.io crates.

It's bad for new contributors who are accustomed to just using stuff from crates.io and now have to add these weird lines

I again agree with this, but I'm not very sympathetic to it. We're not optimizing for pulling in brand new crates into libstd and enabling anyone to do that. We're optimizing for enabling anything to get pulled from crates.io into std.

Ultimately I hope that one day rustbuild can be removed

While I don't think you're wrong I don't think this will ever really be practical... Using this as a justification for changes now I think is a bit too far fetched.

[est31]

@alexcrichton @Mark-Simulacrum thanks for your explanations! I still think that the current situation should be improved somehow, but I guess you don't want this particular approach...

Such stuff is not needed for any of the C dependencies of rustc

I'm not really sure how this is relevant? We don't pull in C dependencies through Cargo, and the whole point of the integration here is to pull in crates.io crates.

I think I have made the point a bit badly. It was mostly about separation of concerns, aka why should dependencies include stuff custom for one of their dependents? E.g. if servo uses my crate, why should I add a specific flag to my project that doesn't have anything to do with servo for servo to enable? Maybe I can add a flag for something that servo needs, but I don't want it to be specific for servo.

I guess I don't want to further waste your time with discussing this approach, but what do you think about the solution I suggested above? Adding a special Cargo flag to libcore, licompiler_builtins, etc. that adds them as dependencies of any crate that they don't depend on themselves. It seems that the number of these crates is less than the number of crates that std depends on.

[bors]

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

[alexcrichton]

I think what you're basically proposing is very closely related to the "std-aware cargo" proposal which has had been a long-desired feature. Eventually we can get there but it's not something trivial to add to Cargo, and also something I don't think is worth pursuing at this time.

[JohnCSimon]

Ping from triage:
@alexcrichton @est31 @Aaron1011 this PR has sat idle for nearly two weeks, but I'm not sure what to do with this.

[est31]

@JohnCSimon given that there is neither support for the PR by @alexcrichton nor by @Mark-Simulacrum I suppose this PR doesn't have hopes of being merged so I sadly guess that closing is the best way forward.

[JohnCSimon]

@Aaron1011 thanks for your contribution, anyway
closing this PR.