Move build-std to Cargo.toml

[fee1-dead]

Resolves #9451. Moves the unstable feature build-std to Cargo.toml per the Rust Internals discussion

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks for the PR, but that internals discussion was generally meant as we're interested in moving some things rather than an enumerated list of "please move everything to Cargo.toml", and build-std is one of those features which is pretty unlikely to move to Cargo.toml for a number of reasons. I also don't really understand how simply moving the configuration for this would solve #9451, as I'm at least personally under the belief that this is a much deeper issue to solve within Cargo.

Can you describe a bit more about why you don't think it's appropriate to configure this in .cargo/config.toml and why you want to move it to Cargo.toml?

[kennystrawnmusic]

Thanks for the PR, but that internals discussion was generally meant as we're interested in moving some things rather than an enumerated list of "please move everything to Cargo.toml", and build-std is one of those features which is pretty unlikely to move to Cargo.toml for a number of reasons. I also don't really understand how simply moving the configuration for this would solve #9451, as I'm at least personally under the belief that this is a much deeper issue to solve within Cargo.

Can you describe a bit more about why you don't think it's appropriate to configure this in .cargo/config.toml and why you want to move it to Cargo.toml?

I can think of one major reason: .cargo/config.toml is a global config file while Cargo.toml is local. That means that if the user has multiple projects with different build requirements (say, is developing an OS and a user-level app at the same time) then he or she will have to edit config.toml every single time he or she switches projects. Forcing the user to either specify command line flags or edit a configuration file that affects all projects simultaneously each time he or she wants to change the build target creates a lot of unnecessary steps for developers, and in a world where IDE development is the norm, not the exception, that’s not a good look.

As for how it fixes #9451 I’ve done enough poking around to know that the issue there lies in the differing methods in which target specs and build-std are handled. Moving per-package-target and build-std to the same place means that they’re detected the same way — which in turn means that if they’re either both in Cargo.toml or both on the command line, they’ll both see each other. If on the other hand one is on the command line and the other is stored in a file, that’s where the discrepancies that cause #9451 arise. Putting both in Cargo.toml is therefore the better option because it unifies and therefore simplifies the build experience — which is exactly what @phil-opp has been on about this entire time because his use case definitely applies here. Update: complex changes to files refutes this

[kennystrawnmusic]

@alexcrichton I’ve looked at @fee1-dead’s changes more closely and they also reveal one other important change that backs up his claim as far as fixing #9451 is concerned: using a HashMap<CompileKind, (HashSet<&str>, HashSet<&str>)> instead of a referenced array of strings in Line 38 of standard_lib.rs exposes the CompileKind specified for each package directly to build_std itself, which in turn makes it much easier for the compiler to know based on per-package-target what exactly the standard library is being built for in those cases.

[kennystrawnmusic]

This PR, if merged, would take 3 items off the phil-opp/blog_os#1063 checklist in one fell swoop, which is a dramatic step closer to getting the third edition published.

See in particular this comment which basically echoes the whole point of this PR.

[kennystrawnmusic]

I've now tested @fee1-dead's changes using the following project file layout and can confirm that I they do indeed fix #9451:

build-std-test/Cargo.toml

cargo-features=[
    "per-package-target",
    "build-std"
]

[package]
name = "build-std-test"
version = "0.1.0"
edition = "2021"
build-std = ["core", "compiler_builtins"]
default-target = "custom.json"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

[profile.dev]
panic = "abort"

[profile.release]
panic = "abort"

build-std-test/custom.json

{
    "llvm-target": "x86_64-unknown-none",
    "data-layout": "e-m:e-i64:64-f80:128-n8:16:32:64-S128",
    "arch": "x86_64",
    "target-endian": "little",
    "target-pointer-width": "64",
    "target-c-int-width": "32",
    "os": "none",
    "executables": true,
    "linker-flavor": "ld.lld",
    "linker": "rust-lld",
    "panic-strategy": "abort",
    "disable-redzone": true,
    "features": "-mmx,-sse,+soft-float"
}

build-std-test/src/main.rs

#![no_std]
#![no_main]

use core::panic::PanicInfo;

#[panic_handler]
fn panic(_info: &PanicInfo) -> ! {
    loop {}
}

#[no_mangle]
pub extern "C" fn _start() {
    loop {}
}

Command line output

[realkstrawn93@realkstrawn93-miner build-std-test]$ ~/cargo/target/release/cargo build
   Compiling compiler_builtins v0.1.66
   Compiling core v0.0.0 (/opt/rust/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core)
   Compiling rustc-std-workspace-core v1.99.0 (/opt/rust/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/rustc-std-workspace-core)
   Compiling build-std-test v0.1.0 (/home/realkstrawn93/Desktop/build-std-test)
    Finished dev [unoptimized + debuginfo] target(s) in 9.30s
[realkstrawn93@realkstrawn93-miner build-std-test]$

If this isn't proof that this PR fixes the problem, I don't know what is.

[kennystrawnmusic]

I do kind of agree that moving everything to Cargo.toml wouldn’t be a good idea. Only those features which are critical to #![no_std] bare metal development should be moved into it:

• build-std (done if this gets merged) — removes the extra step of adding a command line argument to cargo build, saving precious coding time
• build-std-features (done if this gets merged) — removes the extra step of adding a command line argument to cargo build, saving precious coding time
• target.runner — makes it possible to use a simple cargo run to open QEMU and launch a project in it instead of needing to rely on shell scripts to pass all the necessary arguments for QEMU to work (or, worse still, needing to type that long qemu-system-x86_64 string manually each and every time you want to test your code).

This PR proves that exposing these mission-critical features to Cargo.toml is indeed possible.

[fee1-dead]

build-std is one of those features which is pretty unlikely to move to Cargo.toml for a number of reasons.

Can you please show me the previous discussion or why you personally think it shouldn't be moved to Cargo.toml?

I also don't really understand how simply moving the configuration for this would solve #9451, as I'm at least personally under the belief that this is a much deeper issue to solve within Cargo.

Of course it was more complicated than "just move it to Cargo.toml". Before this PR, build-std simply resolved the standard library for the entire build, and then attached the dependency for all transitive dependencies including the root package. After this PR, we will first scan the relevant packages and see if build-std was set. We need to build a HashMap<CompileKind, (HashSet<&str>, HashSet<&str>)>, where the two hashsets are the crates requested and the features requested respectively, then we move on to resolving different target-triples of libstd individually. Finally, when building unit dependencies, we check whether a unit has build-std and attach the std dependencies to that unit and all its transitive dependencies.

Can you describe a bit more about why you don't think it's appropriate to configure this in .cargo/config.toml and why you want to move it to Cargo.toml?

.cargo/config.toml would be fine, if I don't have to set the working directory to a subpackage in my workspace, which I find annoying. I think it is unlikely cargo will support per-package-.cargo/config before another RFC, which led to this PR.

[kennystrawnmusic]

Three days and a lot of tinkering later, it turns out there is an issue with the Cargo.toml approach to getting the build-std option enabled: dependencies don’t honor it. Added a couple of dependencies to my workspace that in turn managed to pull the bit_field and bitflags crates in as dependencies of their own using the method introduced by these changes, and the result was a cascading series of can’t find crate for errors because the build-std option wasn’t propagating through the dependency tree properly. In order to work around this I had to redundantly re-specify the build-std lines on a per-dependency basis:

[dependencies.bit_field]
version = "^0.10.0"
build-std = [
    "core",
    "compiler_builtins",
]

[dependencies.bitflags]
version = "^1.3.0"
build-std = [
    "core",
    "compiler_builtins",
]

[dependencies.volatile]
version = "^0.4.0"
build-std = [
    "core",
    "compiler_builtins",
]

which generated a series of “unused manifest key” warnings despite compiling successfully. Perhaps this is why it why @alexcrichton wants it to remain on the CLI.

With this in mind, however, this PR does introduce other changes that fix #9451 even if the changes that put build-std in Cargo.toml are excluded.

[kennystrawnmusic]

Another problem I encountered: these changes, interestingly enough, actually break the bootloader crate, which was published by the reporter of #9451 (@phil-opp): when an attempt to run the builder alias from the bootloader source tree to create images that can be run from QEMU is made using the fork associated with this PR, a cascading series of unknown -Z flag: build-std errors occurs.

[alexcrichton]

@fee1-dead afaik there aren't discussions to point to, it's mostly been discussion in meetings from long ago. That being said my impression was the deletion of support of -Zbuild-std from .cargo/config.toml, which I also think this PR does. I do think there's a reasonable path forward to supporting both Cargo.toml and config.toml since there are separate use-cases, however. I have other reasons I can list out for why config.toml was done first instead of Cargo.toml but it doesn't really matter if this PR adds support for Cargo.toml instead of also deleting support in config.toml.

Of course it was more complicated than "just move it to Cargo.toml".

Can you split those changes out of this PR? The changes to support the targets/etc in build-std better I think are significant enough to warrant a separate review.

To be clear, you posted this PR with one major commit, little-to-no description, and referenced an internals discussion which didn't actually specifically talk about this setting but rather a general sense of it's ok to move some things on a case-by-case basis. Without reading and understanding your entire PR these sorts of details get lost very quickly. It's extremely helpful to have descriptive messages for commits, descriptive PR text, etc. It makes conveying what your PR does to reviewers much more efficient and effective.

---

I would have to dig in more to understand the problems that @kennystrawnmusic is seeing, but I think it would be best to split this PR into two parts. One part would add build-std support to Cargo.toml and the other would add support for correctly handling the target compilation in more scenarios.

[kennystrawnmusic]

I would have to dig in more to understand the problems that @kennystrawnmusic is seeing, but I think it would be best to split this PR into two parts. One part would add build-std support to Cargo.toml and the other would add support for correctly handling the target compilation in more scenarios.

Looking at the changes makes me inclined to think that would be a very tricky move because many of the #9451-fixing changes have a dependency on the very manifest().build_std() introduced in the changes that move build_std into Cargo.toml. So you’d need to refactor these changes somehow in order to make them compatible with the existing cli_unstable approach.

[alexcrichton]

r? @alexcrichton

[bors]

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

[kennystrawnmusic]

A viable alternative to this (and one that would definitely satisfy @phil-opp's use case in particular) would be to allow developers to add something like this to .cargo/config.toml:

[build.target]
exclude-dirs=[
    "boot",
    ...
]

This is because the problem with the current implementation is that any overrides (e.g. custom targets, build-std) in .cargo/config.toml apply recursively to all subdirectories and therefore all sub-crates within the workspace by default. So if you need to use a helper crate within the workspace, for example, in order to run a cargo alias defined in the source tree of a dependency (like bootloader) it will try to apply the custom target and custom -Zbuild-std configuration to the helper crate as well as the project root, rendering the helper crate useless by so doing.

[kennystrawnmusic]

With the above said I do have a question for @phil-opp: won't adding something like the following override the parent directory's .cargo/config?

# in boot/Cargo.toml
forced-target = "x86_64-unknown-linux-gnu"

The only downside to this approach of course is that it would necessitate the creation of multiple per-host-OS helper crates because obviously a forced-target for Linux wouldn't work on Windows, for example.

Update: nope, this panics.

[alexcrichton]

Thanks for the PR, but I'm going to be stepping down from the Cargo team so I'm going to un-assign myself from this. The Cargo team will help review this when they get a chance.

[fee1-dead]

r? @ehuss

[ehuss]

I'm going to close as this is likely not the approach we want to go with for now. Supporting build-std is a global decision that I think should remain as a config setting for now. The UX for how this will work is tracked in rust-lang/wg-cargo-std-aware#43.

As for getting build-std to specifically work with per-package-target, it looks like there is #10330 as a start to work on that alone. However, I suspect there is more work to get it fully supported. I think there is overlap with artifact-dependencies, and #10405 contains an outline of some of the work that needs to be done.

[jaedson-barbosa]

This PR was exactly what I needed for my current project, too bad it wasn't accepted and I won't be able to use just one repository anymore :(