The new `per-package-target` feature does not work with `-Zbuild-std`

[phil-opp]

Problem

I just tried to use the new per-package-target feature implemented in #9030 (tracking issue) together with -Zbuild-std, but it results in an error: -Zbuild-std requires --target.

Steps

1. Add a default-target or forced-target key in Cargo.toml.
2. Try to compile by running cargo build -Zbuild-std=core.

Possible Solution(s)

I think the issue is that the target check already happens here:

cargo/src/cargo/ops/cargo_compile.rs

Lines 394 to 399 in db741ac

	if build_config.requested_kinds[0].is_host() {
	// TODO: This should eventually be fixed. Unfortunately it is not
	// easy to get the host triple in BuildConfig. Consider changing
	// requested_target to an enum, or some other approach.
	anyhow::bail!("-Zbuild-std requires --target");
	}

But the default-target/forced-target fields are only considered later in generate_targets:

cargo/src/cargo/ops/cargo_compile.rs

Lines 925 to 943 in db741ac

	// If `--target` has not been specified, then the unit
	// graph is built almost like if `--target $HOST` was
	// specified. See `rebuild_unit_graph_shared` for more on
	// why this is done. However, if the package has its own
	// `package.target` key, then this gets used instead of
	// `$HOST`
	let explicit_kinds = if let Some(k) = pkg.manifest().forced_kind() {
	vec![k]
	} else {
	requested_kinds
	.iter()
	.map(|kind| match kind {
	CompileKind::Host => {
	pkg.manifest().default_kind().unwrap_or(explicit_host_kind)
	}
	CompileKind::Target(t) => CompileKind::Target(*t),
	})
	.collect()
	};

The generate_targets function is invoked here:

cargo/src/cargo/ops/cargo_compile.rs

Lines 470 to 488 in db741ac

	// Passing `build_config.requested_kinds` instead of
	// `explicit_host_kinds` here so that `generate_targets` can do
	// its own special handling of `CompileKind::Host`. It will
	// internally replace the host kind by the `explicit_host_kind`
	// before setting as a unit.
	let mut units = generate_targets(
	ws,
	&to_builds,
	filter,
	&build_config.requested_kinds,
	explicit_host_kind,
	build_config.mode,
	&resolve,
	&workspace_resolve,
	&resolved_features,
	&pkg_set,
	&profiles,
	interner,
	)?;

Notes

Output of cargo version:

cargo 1.53.0-nightly (4369396ce 2021-04-27)
release: 1.53.0
commit-hash: 4369396ce7d270972955d876eaa4954bea56bcd9
commit-date: 2021-04-27

cc @Ekleog

[kennystrawnmusic]

Doing some further testing and debugging revealed that the problem has nothing to do with the way -Zbuild-std handles targets and everything to do with the way per-package-target handles targets. To test this, I attempted to compile this with the manifest file set to use the per-package-target specific Cargo.toml options and had forced-target set to the custom .json file ― yet for some reason, regardless of where this check was in the code, they still weren't being honored. Turns out, in a nutshell, that merely moving this check down further didn't solve the problem.

So I tried instead to see what would happen if I commented out the check in question and then tested the compiler that way. You'd expect forced-target to set the target to that custom one, but it didn't. Instead, the compiler panicked. So I tried using the --target=x86_64-unknown-none.json flag instead to set the target manually from the command line. This time, my fork with the check commented out didn't panic and instead compiled successfully. If Phil's reasoning was correct, then you'd expected the commented fork not to panic when not overridden by the command line, but that apparently isn't the case.

[kennystrawnmusic]

Looks like another possible lead on Line 955:

  // Custom build units are added in `build_unit_dependencies`.
  assert!(!target.is_custom_build());

This might be interfering with the ability to pass custom target files as arguments to the per-package-target feature's default-target and forced-target options.

[kennystrawnmusic]

For the record, here's what happens when I attempt a build with both the problematic test commented out and while using Cargo.toml instead of the command line to specify the per-package-target-specific targets:

$ ../cargo/target/debug/cargo build -Zbuild-std="core,compiler_builtins"
warning: unused manifest key: unstable
thread 'main' panicked at 'no entry found for key', src/cargo/core/compiler/unit_dependencies.rs:151:25
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

If @phil-opp was correct about this check being the problem, this wouldn't be happening. This panic message, however, does at least give a better hint as to what the actual problem is: "no entry found for key" ― this means that per-package-target isn't setting the target at all. Which suggests, of course, a fundamental flaw in the way that per-package-target handles JSON files defining custom bare-metal targets ― perhaps if someone put a built-in bare metal target (like an ARM one for example) in that Cargo.toml field it might actually work. If so, that would mean that JSON is completely unsupported by per-package-target itself and would need to be written into the per-package-target code. Update: tried testing this by putting forced-target = "thumbv7em-none-eabihf" in the Cargo.toml file and it too resulted in the error that Phil describes, which means the problem is something other than JSON but still something entirely to do with per-package-target and nothing to do with code arrangement regardless given the above panic message.

FWIW this might be the exact same issue as #9521, given that the lack of information denoted in the panic message is something you'd expect if per-package-target is misidentifying the target specified.

[kennystrawnmusic]

Opening src/cargo/core/compiler/unit_dependencies.rs at line 151 (the panic line) revealed something astonishing:

let mut found = false;
    for (unit, deps) in state.unit_dependencies.iter_mut() {
        if !unit.kind.is_host() && !unit.mode.is_run_custom_build() {
            deps.extend(std_roots[&unit.kind].iter().map(|unit| UnitDep {
                unit: unit.clone(),
                unit_for: UnitFor::new_normal(),
                extern_crate_name: unit.pkg.name(),
                // TODO: Does this `public` make sense?
                public: true,
                noprelude: true,
            }));
            found = true;
        }
    }

Line 151 in this case is the deps.extend() call. Immediately above it is this very peculiar check that may explain everything:

if !unit.kind.is_host() && !unit.mode.is_run_custom_build() {

It appears that per-package-target isn't writing to unit.kind the way it should. Checking therefore to see if changing this && to a || makes a difference. Update: nope.

[kennystrawnmusic]

Looks like I finally found the real culprit at lines 450-454:

    // If `--target` has not been specified, then the unit graph is built
    // assuming `--target $HOST` was specified. See
    // `rebuild_unit_graph_shared` for more on why this is done.
    let explicit_host_kind = CompileKind::Target(CompileTarget::new(&target_data.rustc.host)?);

The explicit_host_kind value here is hard-coded to assume that the target is the host, before any attempt to parse Cargo.toml is made to check to see if a target is specified in there. What's needed, therefore, is another check for pkg.manifest().default_kind() || pkg.manifest().forced_kind() here. Maybe change the definition of explicit_host_kind from a let to an if let? Except that in that case I don't think there's any manifest info defined yet either until later in the file. Which is a problem because you can't know whether or not "default_kind()" or "forced_kind()" are non-null until you first parse the manifest.

[kennystrawnmusic]

Yet another update: turns out that wasn't the issue either. Even this change wasn't enough to solve the problem:

...
    // Use Vec to store build manifests
    let mut to_build_manifests = Vec::new();

    // Use a series of local bools to determine whether or not we have per-package-target enabled (#9451)
    let mut is_per_package_target = false;
    let mut is_default_target = false;
    let mut is_forced_target = false;

    // The ordering here affects some error messages coming out of cargo, so
    // let's be test and CLI friendly by always printing in the same order if
    // there's an error.
    to_builds.sort_by_key(|p| p.package_id());

    for pkg in to_builds.iter() {
        ...
        to_build_manifests.push(pkg.manifest()); //added into already existing loop
        ...
    }

    // Set is_per_package_target to true if any per-package-target specific feature flags are detected (#9451)
    for m in to_build_manifests.iter() {
        match m.forced_kind() {
            Some(_) => {
                is_per_package_target = true;
                is_forced_target = true;
            },
            None => {},
        }
        match m.default_kind() {
            Some(_) => {
                is_per_package_target = true;
                is_default_target = true;
            },
            None => {},
        }
    }
    ...

    // Redefined as mutable
    let mut explicit_host_kind = CompileKind::Target(CompileTarget::new(&target_data.rustc.host)?);

    // #9451
    if is_per_package_target == true && is_forced_target == true {
        // TODO: find a better way to do this than overwriting multiple times with an iterator
        for m in to_build_manifests.iter() {
            for t in m.targets().iter() {

                // clobber explicit_host_kind with what should be the per-package-target
                let forced_target = match CompileTarget::new(t.name())?;
                explicit_host_kind = CompileKind::Target(forced_target);
            }
        }
    } else if is_per_package_target == true && is_default_target == true {
        // TODO: find a better way to do this than overwriting multiple times with an iterator
        for m in to_build_manifests.iter() {
            for t in m.targets().iter() {

                // clobber explicit_host_kind with what should be the per-package-target
                let default_target CompileTarget::new(t.name())?;
                explicit_host_kind = CompileKind::Target(default_target);
            }
        }
    }

Once again this narrows it down further and brings us closer to suspecting that #9521 and this issue are tightly interconnected.

Update: Same panic message but from a different source file:

$ ../cargo/target/debug/cargo build -Zbuild-std="core,compiler_builtins"
thread 'main' panicked at 'no entry found for key', src/cargo/core/compiler/build_context/target_info.rs:810:40

This is part of this function:

/// Gets the target configuration for a particular host or target.
    pub fn target_config(&self, kind: CompileKind) -> &TargetConfig {
        match kind {
            CompileKind::Host => &self.host_config,
            CompileKind::Target(s) => &self.target_config[&s], //panics here
        }
    }

That it's still panicking with the same error (albeit in a different file) after this change shows that it's probably a TOML parsing issue, for 2 main reasons:

1. The fact that a completely different line of a completely different file is involved means that the Boolean values were indeed set when calls to m.forced_kind() and m.default_kind() were made in the first loop suggests that said values were correctly read from their respective files
2. Despite this, it seems that the target property itself remained unchanged.

So what needs to happen is that someone on the per-package-target team needs to find a way to override this current target hardcoding problem. If Rust had a mechanism for searching TOML files for certain key names, that would help things out immensely.

[kennystrawnmusic]

Trying this again with pull-request #10108 which is a partial fix ― needs some further work to merge though; hopefully by opening it up to the open-source community at large, it'll help get the problem solved faster.

The new enum PerPackageTargetMode and struct PerPackageTarget objects that I introduce in this new PR should, along with the constructor PerPackageTarget::new(f: &str) that takes a manifest filename and brute-forces it for either default-target or forced-target using a Regex should, at the very least, drastically reduce the number of lines of code necessary to debug this.

[Ekleog]

@kennystrawnmusic Thank you very much for your investigation! Here are some comments about your findings:

FWIW this might be the exact same issue as #9521, given that the lack of information denoted in the panic message is something you'd expect if per-package-target is misidentifying the target specified.

I think this is not the case: #9521 is not about misidentifying the target specified, but about compiling crates with too many features. Basically, the core of the issue is that cargo assumes additive features, so when having a workspace with two dependencies on foo with different features it compiles it just once with all the features. The issue of #9521 is that it still does this if the two dependencies on foo are with different architectures, which in practice leads to broken code because not all features are available on all architectures and so features aren't actually really additive.

The fix to #9521 would be to instead of having one crate build graph with all the features, have one crate build graph per architecture… either that, or have one crate build graph per crate instead of per workspace, which to me would make the most sense so that cargo build behaves the same in the workspace and in the crates themselves.

The explicit_host_kind value here is hard-coded to assume that the target is the host, before any attempt to parse Cargo.toml is made to check to see if a target is specified in there.

Yes, explicit_host_kind is a variable that is used to define the kind of the host of the compiler. Honestly, I'm not sure why it is used, but I saw it while trying to do the changes needed for per-package-target so I didn't touch it. per-package-target hooks at another place in the build process. I'm not 100% sure I remember correctly, but I seem to remember that is by setting some parameter on each root crate of the build graph, and cargo then computes the correct build graph by taking this into account via pre-existing mechanisms.

So what needs to happen is that someone on the per-package-target team needs to find a way to override this current target hardcoding problem

AFAICT there's no per-package-target team, I just wrote the initial implementation and then didn't have enough time to get back to this :( that said, if you have a small reproducer with an unpatched cargo, would you be able to share it with me as eg. a tarball or git repo along with the exact commands to be used to reproduce? I may have a bit of time in the upcoming weeks, and can try looking at this :)

[Ekleog]

Thinking some more about it: what would be the semantics of -Zbuild-std with per-package-target? In particular, how should it behave when being used in a workspace that has different crates with different targets? (which is the main “reason to be” of per-package-target, as if there's a single crate there's not much point in having a default/forced target defined as it can just as well be passed on the command line)

I'm halfway thinking -Zbuild-std should be changed to take eg. -Zbuild-std-for=[target], which could then be passed any number of times. That said I'm not used to build-std, as I never compiled for an architecture for which I couldn't get the prebuilt std yet. Would that make sense to y'all?

[kennystrawnmusic]

@kennystrawnmusic Thank you very much for your investigation! Here are some comments about your findings:

FWIW this might be the exact same issue as #9521, given that the lack of information denoted in the panic message is something you'd expect if per-package-target is misidentifying the target specified.

I think this is not the case: #9521 is not about misidentifying the target specified, but about compiling crates with too many features. Basically, the core of the issue is that cargo assumes additive features, so when having a workspace with two dependencies on foo with different features it compiles it just once with all the features. The issue of #9521 is that it still does this if the two dependencies on foo are with different architectures, which in practice leads to broken code because not all features are available on all architectures and so features aren't actually really additive.

The fix to #9521 would be to instead of having one crate build graph with all the features, have one crate build graph per architecture… either that, or have one crate build graph per crate instead of per workspace, which to me would make the most sense so that cargo build behaves the same in the workspace and in the crates themselves.

The explicit_host_kind value here is hard-coded to assume that the target is the host, before any attempt to parse Cargo.toml is made to check to see if a target is specified in there.

Yes, explicit_host_kind is a variable that is used to define the kind of the host of the compiler. Honestly, I'm not sure why it is used, but I saw it while trying to do the changes needed for per-package-target so I didn't touch it. per-package-target hooks at another place in the build process. I'm not 100% sure I remember correctly, but I seem to remember that is by setting some parameter on each root crate of the build graph, and cargo then computes the correct build graph by taking this into account via pre-existing mechanisms.

So what needs to happen is that someone on the per-package-target team needs to find a way to override this current target hardcoding problem

AFAICT there's no per-package-target team, I just wrote the initial implementation and then didn't have enough time to get back to this :( that said, if you have a small reproducer with an unpatched cargo, would you be able to share it with me as eg. a tarball or git repo along with the exact commands to be used to reproduce? I may have a bit of time in the upcoming weeks, and can try looking at this :)

Sorry for the late response — been very busy myself because it is almost Christmas after all.

One already-existing set of instructions to reproduce this bug is the set of instructions that @phil-opp posted to his blog. Whenever the “-Zbuild-std” option is invoked, it fails to check if “per-package-target” is specified in the package manifest file before throwing an error. That’s why I attempted to contribute that PerPackageTarget enum through my fork, although I agree that there has to be a better way to find out if it’s specified than by re-parsing Cargo.toml all over again. I did try to use crate::core::manifest::Manifest and attempt to read from the Manifest::default_kind and/or Manifest::forced_kind variables prior to creating that structure, but that didn’t work.

Obviously it is possible to work around this bug by simply specifying the custom target .json file with --target for every package built individually but that defeats the whole purpose of this.

As Phil also suggested here, exposing -Zbuild-std options to Cargo.toml to allow them to be set there instead of needing them to be specified from the CLI every time would also help and could potentially be a more viable solution because that would make it possible to source both unstable.build-std and default-target/forced-target from the same place instead of relying on the command line for the former and Cargo.toml for the latter. You’d in that case just need to add those as extra entries to the Manifest structure mentioned previously and then use those values as the components of the standard library to build instead of relying on the command line as much as Cargo does. That in fact could be part of the problem: perhaps the default-target and forced-target values are set in the background from the Cargo.toml file but something between the configuration file and the command line is keeping -Zbuild-std from seeing them.