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

Linker and links are ignored with target-applies-to-host and no --target flag #14195

Open
gmorenz opened this issue Jul 5, 2024 · 7 comments
Labels
C-bug Category: bug S-triage Status: This issue is waiting on initial triage. Z-target-applies-to-host Nightly: target-applies-to-host

Comments

@gmorenz
Copy link
Contributor

gmorenz commented Jul 5, 2024

Problem

This is the exact same bug as #10744 except with regards to linker and links instead of rustflags and rustdocflags. When building with target-applies-to-host=false and not passing a --target flag linker and links are ignored because the host-values of those are set to the default and then the artifact units end up picking up the host values (see "How it is implemented" in #13900 for an explanation of how that happens).

Steps

Running on x86_64-unknown-linux-gnu (if you have a different host substitute in the correct architecture)

cargo new demo
cd demo
cargo build -Z target-applies-to-host --config target-applies-to-host=false  --config target.x86_64-unknown-linux-gnu.linker="\"nope\""

Expected behavior: error: linker `nope` not found

Actual behavior: It builds without error

cargo new demo
cd demo
echo "int myfunc() {}" | gcc -xc -shared -o libmylib.so -
sed -i '/\[package\]/a links="mylib"' Cargo.toml
echo 'fn main() { panic!("build.rs run") }' > build.rs
cargo build \
    --config 'target.x86_64-unknown-linux-gnu.mylib.rustc-link-lib=["mylib"]' \
    --config 'target.x86_64-unknown-linux-gnu.mylib.rustc-link-search=["native=."]' \
    -Z target-applies-to-host \
    --config target-applies-to-host=false

Expected behavior: It builds without error

Expected bug: It fails to find mylib

Actual behavior:

    Creating binary (application) `demo` package
note: see more `Cargo.toml` keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
thread 'main' panicked at src/cargo/core/compiler/custom_build.rs:256:10:
running a script not depending on an actual script
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I.e. cargo crashes as a whole instead of just failing to apply the flags. This only happens in the case where target-applies-to-host=false and no --target flag is passed so it's likely a side effect of failing to apply the flags, but that should be confirmed. I am confident it does fail to apply the flags as well from having been working on the code.

Possible Solution(s)

Extend the fix in #13900 to linker and link overrides. That should fix the expected bugs, but the actual behavior of link overrides causing cargo to panic is a surprise to me and might need something further.

@gmorenz gmorenz added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Jul 5, 2024
@gmorenz
Copy link
Contributor Author

gmorenz commented Jul 5, 2024

@rustbot label: Z-target-applies-to-host

@weihanglo weihanglo added the Z-target-applies-to-host Nightly: target-applies-to-host label Jul 5, 2024
@weihanglo
Copy link
Member

I am not @rustbot :P

@gmorenz
Copy link
Contributor Author

gmorenz commented Jul 5, 2024

I should say I intend to fix this since it's a natural continuation of what I already fixed, but thought that I should at least create an issue before doing so instead of completely ignoring process.

@gmorenz
Copy link
Contributor Author

gmorenz commented Jul 5, 2024

Alright, went ahead and tracked down the root cause of the panic for the links-override half of this issue, tl;dr it is a side effect of failing to properly apply the link overrides flags.

compute_deps_custom_build gets called through compile_ws -> create_bcx (before rebuild_unit_depdendencies) -> build_unit_dependencies --<main path>--> deps_of -> compute_deps -> compute_deps_custom_build, sees that script_override(links, unit.kind) returns Some, and returns that there are no dependencies because it is overridden.

Shortly-therafter rebuild_unit_graph_shared is run, the units kind is changed from Target(host_architecture) to Host.

Much later in the build process build_map gets called through compile_ws -> BuildRunner::compile -> build_map, calls script_override(links, unit.kind) again, and this time gets that there is no override because we haven't overriden the build scripts for host deps. This means it doesn't fill in the build_script_output from the config.

Finally we get around to running the custom build script (compile_ws -> BuildRunner::compile -> compiler::compile -> custom_build::prepare), check if there is a build_script_outputs already, and since there isn't try and run the build-script that we didn't prepare dependencies for. Crashing.

The proposed fix should completely fix this error case, by storing link overrides in the unit instead of looking it up via kind when needed it should no longer be possible to get inconsistent results on whether or not overrides exist.

@weihanglo
Copy link
Member

weihanglo commented Jul 10, 2024

Just re-read this issue. I guess this table is what we are going to fix?
(change in bold text)

target_applies_to_host=true target_applies_to_host=false
no --target flag Flag passed to bin
Flag passed to shared dep from bin
Linker passed to bin

Flag passed to proc macro
Flag passed to shared dep from proc macro
Linker passed to proc macro

Built with 5 invocations
Without rustflags, built with 5 invocations
Flag passed to bin
Flag passed to shared dep from bin
Linker passed to bin (wasn't before)

Flag not passed to proc macro
Flag not passed to shared dep from proc macro
Linker not passed to proc macro

Built with 6 invocations
Without rustflags, built with 5 invocations

@weihanglo
Copy link
Member

If that is the case, then we don't really need to ping the entire team.

@gmorenz
Copy link
Contributor Author

gmorenz commented Jul 10, 2024

Yes, exactly that, only modifying behaviour in the top right box.

For completeness that table is missing links-overrides, but it follows the same pattern as linker (or would except that in the top right box it actually just panics cargo right now).

bors added a commit that referenced this issue Jul 18, 2024
Fix passing of links-overrides with target-applies-to-host and an implicit target

### What does this PR try to resolve?

This fixes the link-overrides half of #14195, both the panic, and the fact that the field is being discarded, the latter of which caused the former as discussed in [the issue](#14195 (comment)).

It does so following the blueprint laid out in #13900 - which is also in my opinion the current best summary of the broader context.

### How should we test and review this PR?

For reviewing, comparing to the changes in #13900 might be useful.

### Additional information

I'm pushing a PR for the other half of #14195 simultaneously. I thought it better to keep the PRs small since they're independent, though if merged simultaneously there will be a conflict over the ordering of fields in `Unit`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug S-triage Status: This issue is waiting on initial triage. Z-target-applies-to-host Nightly: target-applies-to-host
Projects
None yet
Development

No branches or pull requests

2 participants