cargo_build_script uses inconsistent vars between compilation and runtime

[UebelAndre]

The way cargo_build_script works today is that a rust_binary target is created and used in another rule, _cargo_build_script a private rule which actually executes the build.rs script. I believe I've found an issue with this pattern where environment/build variables generated for the binary do not match what would be passed at runtime by _cargo_build_script. This is because the cargo variables (like CARGO_MANIFEST_DIR) are set to paths within the sandbox currently used to build the rust_binary but are later set to an expected path within the new sandbox for the CargoBuildScriptRun action (note, these paths may also now be the runfiles of the rust_binary).

I've created a draft to demonstrate the issue:

• Added cargo_build_script integration test. #877

I believe the fix is to merge the creation of the build.rs binary and it's execution into a single rule. Open to other ideas but I think the result should be the pull request linked above passes.

[illicitonion]

See also #661 :)

As far as I know, and can tell, the runtime env vars should be a superset of the compile-time env vars, and the only expected difference in our implementation is CARGO_MANIFEST_DIR. We do go to some effort to normalise CARGO_MANIFEST_DIR where possible - redact_exec_root:

rules_rust/cargo/cargo_build_script_runner/lib.rs

Lines 184 to 186 in 73f228f

	fn redact_exec_root(value: &str, exec_root: &str) -> String {
	value.replace(exec_root, "${pwd}")
	}

tries to make the "official" ways of passing paths up to dependents (e.g. env vars and flags) end up agnostic, though if someone e.g. is generating code containing absolute paths, there's little we can do.

Is there a specific problem you ran into where this is causing problems?

[UebelAndre]

Is there a specific problem you ran into where this is causing problems?

Yeah, the behavior I added in my draft came wasm-bindgen-shared::build.rs. So now, without patching, the new release of wasm-bindgen is not usable.

[UebelAndre]

See also #661 :)

Maybe I'm misinterpreting this but #661 seems to be saying not all the same variables are being set. Where this issue is saying variables that are set have different values at compile time and runtime. I can close this one if the older ticket covers the same issue.

[illicitonion]

Is there a specific problem you ran into where this is causing problems?

Yeah, the behavior I added in my draft came wasm-bindgen-shared::build.rs. So now, without patching, the new release of wasm-bindgen is not usable.

Can you expand the reproduction a bit to actually trying to read the files being problematic? I get that the literal values are different, but looking at wasm-bindgen's build.rs, I'd expect both values to work for their needs - src/lib.rs should be available relative to both of the directories that have the value set, assuming the right runfiles are set up.

[UebelAndre]

Can you expand the reproduction a bit to actually trying to read the files being problematic? I get that the literal values are different, but looking at wasm-bindgen's build.rs, I'd expect both values to work for their needs - src/lib.rs should be available relative to both of the directories that have the value set, assuming the right runfiles are set up.

Because the value of CARGO_MANIFESTS_DIR is compiled into the binary, it ends up being incorrect when the cargo_build_script rule runs it. The runfiles are not the issue (which are all present), the issue is that the path differs from compile time to runtime which results in cases where files cannot be found (full log):

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `"/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/423/execroot/rules_rust/test/cargo_build_script"`,
 right: `"/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1016/execroot/rules_rust/bazel-out/k8-opt-exec-2B5CBBC6/bin/test/cargo_build_script/cargo_build_script_script_.runfiles/rules_rust/test/cargo_build_script"`', test/cargo_build_script/build.rs:7:5

Note both the sandbox directory 423 vs 1016 and the fact that the latter is in a runfiles directory and the former is not.

[illicitonion]

We already try to address this some - the 423 vs 1016 we should already paper over with the exec-root redaction:

rules_rust/cargo/cargo_build_script_runner/lib.rs

Lines 184 to 186 in 73f228f

	fn redact_exec_root(value: &str, exec_root: &str) -> String {
	value.replace(exec_root, "${pwd}")
	}

It feels like we could probably do something similar for the path from the exec-root to the runfiles path? So that we're doing the correct replacement in the rustc_env to make this work?

[UebelAndre]

I think that only impacts runtime arguments, not anything that was compiled into the binary... How would the script runner modify that value in a compiled binary?

[illicitonion]

Oh, I see what you mean - yeah, the way we've made other libraries work with this is by giving them an overridable env var which is used at runtime which defaults to the compile-time value...

[UebelAndre]

Right but the rules should account for this because we can't expect everyone to conform to this restriction and I think it's unreasonable to force users to find workarounds for something that should just work. I suspect if the build.rs script is created in the rule then it'd work? Otherwise we'll need to update the cargo_build_script rule to build and run everything in the same action. Which I suspect is a pretty small update to the script runner.

[illicitonion]

There's always going to be a limit to how much we can cover here - for instance, prost used to embed the absolute path in the binary as a variable, and use that value at binary runtime (rather than build script runtime)... So I guess we can combine the rustc and build-script-run stages into a single action, and it'll probably cover a few more cases, but I'm wary of doing anything too invasive because we'll always be somewhat limited...

[UebelAndre]

I think the rules should aim to be as compatible as possible. Particularly with cargo_build_script But there are some things that just do not translate and will need to be documented how they differ from cargo or why they are not supported.

Can you show me what the specific issue is with prost? Is it related to a similar issue with build.rs scripts?

[illicitonion]

tokio-rs/prost#282

Effectively, an absolute path was being baked into the binary at build.rs time and that would be used at runtime (rather than at build-script-run time)

[UebelAndre]

#842 is another example of a cargo environment variable I don't think is going to work with Bazel so a compromise must be met.

[UebelAndre]

tokio-rs/prost#282

Effectively, an absolute path was being baked into the binary at build.rs time and that would be used at runtime (rather than at build-script-run time)

Sorry, can you break this one down a bit more if you have the context? The issue is that the build.rs script compiled an environment variable into itself to set a value to compile into the prost binary? And this value is required to be an absolute path?

[illicitonion]

The build.rs used to end up effectively running:

println!("cargo:rustc-env=PROTOC={}", std::env::current_dir().unwrap().join("third-party").join("protobuf").join("protoc").display());

and the src/main.rs which depends on the build script would effectively exec env!("PROTOC")

[UebelAndre]

I see now, thanks for the follow-up! Do you know if it'd be possible to allow user defined variables in a BUILD file to supersede variables from a build.rs file? If so I can open a separate issue for that.

[illicitonion]

I think it would be a pretty hard DSL to express... I really think the answer for this kind of case is to make the build script more configurable, and reduce the assumptions it makes about its environment.

[UebelAndre]

I ended up opening rustwasm/wasm-bindgen#2657 to try and fix the compatibility issues with wasm_bindgen. It'd still be great if we had a way to keep CARGO_MANIFEST_DIR consistent at compilation time and runtime of a Cargo build script.

[UebelAndre]

Closing this out as I don't think there's a real solution for the issue. cargo_build_script needs to compile a rust binary to run on the exec platform so it can produce files for the target platform. I do not know of a mechanism to target different configurations within the same rule. So the binary will always be compiled separately and end up having a different sandbox path (since it's a different action). In other words CARGO_MANIFEST_DIR will always be slightly different in each action as it includes the sandbox path.