Symlink in the exec-root so that relative paths will work, unchanged. #1781

freeformstu · 2023-01-17T20:30:51Z

illicitonion

This is a pretty cute solution, thanks for putting it together!

Could you add add a test-case to https:/bazelbuild/rules_rust/tree/main/test/cargo_build_script which shows that this works, so that we don't break it in the future?

illicitonion · 2023-01-18T13:22:30Z

cargo/cargo_build_script_runner/bin.rs

+ )
+ })?;
+
+ // Only symlink directories.


Why not symlink files too?

There's actually no reason not to symlink in files as well. I did not need it for my use case so I ignored it. I'll get file symlinks added in.

illicitonion

LGTM, just some minor comments - thanks!

illicitonion · 2023-01-24T10:16:00Z

cargo/cargo_build_script_runner/bin.rs

+ .ok_or_else(|| "Failed while getting file name".to_string())?
+ .to_str()
+ .ok_or_else(|| "Failed while converting file name to string".to_string())?;


Suggested change

.ok_or_else(|| "Failed while getting file name".to_string())?

.to_str()

.ok_or_else(|| "Failed while converting file name to string".to_string())?;

.ok_or_else(|| "Failed while getting file name".to_string())?;

You can use an OsString to Path::join

illicitonion · 2023-01-24T10:31:29Z

test/cargo_build_script/BUILD.bazel

+ name = "root_file",
+ out = "root_file.txt",


Suggested change

name = "root_file",

out = "root_file.txt",

name = "cargo_manifest_dir_file",

out = "cargo_manifest_dir_file.txt",

My first read of this in the build script assumed "root" meant "root of repo"

illicitonion · 2023-01-24T10:32:05Z

test/cargo_build_script/test_exec_root_access.build.rs

+ "Build script must be in the current directory"
+ );
+ assert!(
+ root_files.take("root_file.txt").is_some(),


Let's assert on the contents too

freeformstu · 2023-02-01T19:36:34Z

@illicitonion Apologies for the delay on this PR. I just addressed your comments - I'm still working through some windows test failures in this PR. I'll try to get those addressed by the end of this week.

Do you mind if I squash and rebase?

illicitonion · 2023-02-03T10:45:13Z

Thanks! Feel free to manage the branch however you want (rewrite history or add commits on top), and I'll squash + rebase into main whenever it's done :)

xortive · 2023-02-23T22:20:33Z

Hey @freeformstu @illicitonion! #1762 is a hard blocker for us to start using a custom toolchain with our bazel build. I've been using this PR's changes in a fork successfully and I'd love to see it merged. I'm willing to offer help in resolving the windows build problems if that makes things easier for y'all

UebelAndre

I think the failure here is caused by the Windows not actually having support for sandboxing. I don't have a windows box so it's kinda hard to confirm but reading https://bazel.build/versions/6.1.0/docs/sandboxing it seems to explicitly mention Linux and MacOS but not Windows which, in addition to the failure here, makes me think there's just no sandboxing. This is potentially a good find for the feature as we should make sure it works correctly with and without sandboxing.

UebelAndre · 2023-04-27T13:38:01Z

cargo/cargo_build_script_runner/bin.rs

@@ -53,6 +53,23 @@ fn run_buildrs() -> Result<(), String> {
 create_dir_all(&out_dir_abs)
 .unwrap_or_else(|_| panic!("Failed to make output directory: {:?}", out_dir_abs));

+ // Symlink the execroot to the manifest_dir so that we can use relative paths in the arguments.
+ let exec_root_paths = std::fs::read_dir(&exec_root)


Ok, I think the issue here is that when running outside of a sandbox there will actually be more files in this directory than expected. Instead of reading from the execroot, is there a MANIFEST file or something that tracks all inputs to an action? I forget if that's created for actions or just tests. The idea would be you can link only the things the build script has as an explicitly registered as an input.

Looks like MANIFEST is only found in runfiles

Thanks @UebelAndre! I can reproduce the issue locally with this diff now:

diff --git a/cargo/private/cargo_build_script.bzl b/cargo/private/cargo_build_script.bzl index e567c1c5..3d1f4858 100644 --- a/cargo/private/cargo_build_script.bzl +++ b/cargo/private/cargo_build_script.bzl @@ -242,6 +242,9 @@ def _cargo_build_script_impl(ctx): mnemonic = "CargoBuildScriptRun", progress_message = "Running Cargo build script {}".format(pkg_name), env = env, + execution_requirements = { + "local": "", + } ) return [

What do you think the behavior should be in this case? Should I just disable all of this logic if you're building outside of a bazel sandbox since that's the whole point of this change?

My most recent change updates the test to only assert that the execroot is empty if the test is running in a sandbox. This resolves almost all of the issues but now I have 3 remaining. One is definitely symlink related and the other two look unrelated but may be

Questions:

Would it be acceptable to make this a feature which does not exist when running on Windows so that it can entirely avoid the windows symlinking issues?

If that's not acceptable, would it be acceptable to make this feature opt-in (perhaps through an environment variable) so that consumers of this change can toggle it on where they see fit? That would give folks the ability to enable it for windows in the environments where they have symlinks enabled.

I think making this a feature would be good. It can be disabled by default and enabled globally via --features in user's .bazelrc files. My experience is doing anything on windows is a bad idea 😅 but we should probably aim to keep the behavior consistent across operating systems.

UebelAndre · 2023-05-03T15:06:52Z

cargo/cargo_build_script_runner/bin.rs

@@ -53,6 +53,23 @@ fn run_buildrs() -> Result<(), String> {
 create_dir_all(&out_dir_abs)
 .unwrap_or_else(|_| panic!("Failed to make output directory: {:?}", out_dir_abs));

+ // Symlink the execroot to the manifest_dir so that we can use relative paths in the arguments.
+ let exec_root_paths = std::fs::read_dir(&exec_root)


I think making this a feature would be good. It can be disabled by default and enabled globally via --features in user's .bazelrc files. My experience is doing anything on windows is a bad idea 😅 but we should probably aim to keep the behavior consistent across operating systems.

freeformstu · 2023-05-17T18:04:36Z

~~Now I just have ios and mac build issues which look like they're just generally broken (not due to my changes). @UebelAndre any recommendations here?~~

It's working now after rebasing on the latest changes.

UebelAndre

Awesome work! Just a few questions 😅

UebelAndre · 2023-05-19T18:26:18Z

.bazelci/presubmit.yml

@@ -27,6 +27,7 @@ default_windows_targets: &default_windows_targets
 - "-//bindgen/..."
 - "-//test/proto/..."
 - "-//test/unit/pipelined_compilation/..."
+ - "-//test/cargo_build_script:test_exec_root_access.build.feature_enabled"


Can you delete this in favor of a target_compatible_with flag on the target should you need it?

Yes definitely!

UebelAndre · 2023-05-19T18:28:32Z

cargo/private/cargo_build_script.bzl

@@ -3,6 +3,7 @@
 load("@bazel_skylib//lib:paths.bzl", "paths")
 load("@bazel_tools//tools/build_defs/cc:action_names.bzl", "CPP_COMPILE_ACTION_NAME", "C_COMPILE_ACTION_NAME")
 load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")
+load("//cargo:features.bzl", "SYMLINK_EXEC_ROOT_FEATURE", "feature_enabled")


Why would this need to be a part of the public api vs living in //cargo/private?

I can move it in there. I was thinking it would be useful to be able to load the constant in downstream repositories. But even in our repository we're just setting this feature in the .bazelrc so it doesn't add any value there.

I'll move it under //cargo/private.

That sounds reasonable, but could you then move the feature_enabled logic into //cargo/private? Surely that doesn't seem necessary, right?

UebelAndre

Amazing work! Thank you so much!

dk333333 · 2023-06-02T19:57:12Z

Hi @freeformstu apologies for the naive question, but how/where do I enable this feature? I had cherry picked an earlier commit but will need to enable it for real when upgrading. I checked the docs but could not find it there

…bazelbuild#1781) * Symlink in the exec-root so that relative paths will work, unchanged. * Address feedback * Address more feedback * buildifier

freeformstu force-pushed the dev/symlink-exec-root branch 5 times, most recently from 1909efb to 0f6c5b1 Compare January 17, 2023 22:10

illicitonion reviewed Jan 18, 2023

View reviewed changes

illicitonion approved these changes Jan 24, 2023

View reviewed changes

dk333333 mentioned this pull request Mar 2, 2023

[Question] Missing files from build scripts when running crates_vendor #1831

Closed

UebelAndre requested changes Apr 27, 2023

View reviewed changes

freeformstu force-pushed the dev/symlink-exec-root branch from 5bf0cb5 to cefce11 Compare May 2, 2023 21:12

UebelAndre requested changes May 3, 2023

View reviewed changes

freeformstu force-pushed the dev/symlink-exec-root branch from 1b6cc0a to 7fb87ce Compare May 16, 2023 22:16

UebelAndre requested changes May 19, 2023

View reviewed changes

freeformstu and others added 3 commits May 22, 2023 16:55

Symlink in the exec-root so that relative paths will work, unchanged.

c128408

Address feedback

17b9921

Address more feedback

903589e

freeformstu force-pushed the dev/symlink-exec-root branch from 3dedab2 to 903589e Compare May 22, 2023 23:55

buildifier

6c6be6e

freeformstu requested a review from UebelAndre May 23, 2023 00:07

UebelAndre approved these changes May 23, 2023

View reviewed changes

UebelAndre merged commit 94cbe4c into bazelbuild:main May 23, 2023

UebelAndre mentioned this pull request Jul 27, 2023

Cargo experimental_symlink_execroot #2084

Open

renovate bot mentioned this pull request Dec 27, 2023

chore(deps): update dependency rules_rust to v0.51.0 - autoclosed formatjs/formatjs#4211

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Symlink in the exec-root so that relative paths will work, unchanged. #1781

Symlink in the exec-root so that relative paths will work, unchanged. #1781

freeformstu commented Jan 17, 2023

illicitonion left a comment

illicitonion Jan 18, 2023

freeformstu Jan 18, 2023

illicitonion left a comment

illicitonion Jan 24, 2023

illicitonion Jan 24, 2023

illicitonion Jan 24, 2023

freeformstu commented Feb 1, 2023

illicitonion commented Feb 3, 2023

xortive commented Feb 23, 2023

UebelAndre left a comment

UebelAndre Apr 27, 2023

UebelAndre Apr 27, 2023

freeformstu May 2, 2023

freeformstu May 2, 2023

UebelAndre May 3, 2023

UebelAndre May 3, 2023

freeformstu commented May 17, 2023 •

edited

Loading

UebelAndre left a comment

UebelAndre May 19, 2023

freeformstu May 19, 2023

UebelAndre May 19, 2023

freeformstu May 19, 2023

UebelAndre May 19, 2023

freeformstu May 23, 2023

UebelAndre left a comment

dk333333 commented Jun 2, 2023

Symlink in the exec-root so that relative paths will work, unchanged. #1781

Symlink in the exec-root so that relative paths will work, unchanged. #1781

Conversation

freeformstu commented Jan 17, 2023

illicitonion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illicitonion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeformstu commented Feb 1, 2023

illicitonion commented Feb 3, 2023

xortive commented Feb 23, 2023

UebelAndre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeformstu commented May 17, 2023 • edited Loading

UebelAndre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UebelAndre left a comment

Choose a reason for hiding this comment

dk333333 commented Jun 2, 2023

freeformstu commented May 17, 2023 •

edited

Loading