Handle files from external repos (fix breaking change in Bazel 3.7.0) #2138

jgsogo · 2023-08-29T09:11:32Z

In Bazel 3.7.0, the path returned by build_file_path changed (bazelbuild/bazel#12344), it no longer contains the external/<repo> prefix for external files. This breaks the expectations of this rule in the paths.relativize function call.

In this PR I prepend the workspace_root (external/<repo>) to build_file_path (<path/in/the/repo>), and use path (external/<repo>/path/in/the/repo) instead of short_path (../<repo>/path/in/the/repo).

For files that are generated in different configurations I need to strip the root which contains the bazel-out/darwin-fastbuild-ST-a1a0f4088294/bin/ that will never appear in the package_root path.

~~Note.- I will probably need some help to add tests for this change~~ I've tried to add one test

UebelAndre

Thanks for working on this! Just one question 😄

UebelAndre · 2023-08-30T16:04:50Z

rust/private/rust.bzl


    if crate_root and (crate_root.is_source or crate_root.root.path != ctx.bin_dir.path):
-        generated_root = ctx.actions.declare_file(paths.relativize(crate_root.short_path, package_root))
+        crate_root_path = paths.relativize(crate_root.path, crate_root.root.path)


Would it be simpler to just check if the path starts with ../ instead?

Suggested change

crate_root_path = paths.relativize(crate_root.path, crate_root.root.path)

if crate_root.short_path.startswith("../"):

generated_root = ctx.actions.declare_file(paths.relativize("external/" + crate_root.short_path[len("../"):], package_root))

else:

generated_root = ctx.actions.declare_file(paths.relativize(crate_root.short_path, package_root))

Honestly, I find my approach easier to read without the if/else and no hardcoded strings (very unlikely, but they might change in the future). Maybe I can rename the variable crate_root_path to crate_root_rel_path to make it a bit more explicit?

In any case, no strong opinion here. If you feel like your approach is easier to read and maintain, I'm fine. You have more Bazel experience than me.

I think explicitly handling ../ vs external/ is easier for me to maintain but would not broadly say it's more maintainable. As it's written now I feels like there's a lot going on. With either change though I think there should be a comment explaining what's happening but I do kinda like the simplicity of targeting the specific failure. If you want to keep your approach could you break it out into another function with an explanation of what's going on?

UebelAndre · 2023-09-04T14:19:44Z

rust/private/rust.bzl


    if crate_root and (crate_root.is_source or crate_root.root.path != ctx.bin_dir.path):
-        generated_root = ctx.actions.declare_file(paths.relativize(crate_root.short_path, package_root))
+        crate_root_path = paths.relativize(crate_root.path, crate_root.root.path)


I think explicitly handling ../ vs external/ is easier for me to maintain but would not broadly say it's more maintainable. As it's written now I feels like there's a lot going on. With either change though I think there should be a comment explaining what's happening but I do kinda like the simplicity of targeting the specific failure. If you want to keep your approach could you break it out into another function with an explanation of what's going on?

jgsogo · 2023-09-04T21:19:02Z

I moved the logic to another function and added some description to it with a (hopefully) good enough comment. Thanks a lot for the reviews!

UebelAndre

I can take another look later today but I’m wondering why this regression wasn’t caught. Is this fixing behavior for Bazel 3? Is there some kind of regression test that can be added as part of this PR?

jgsogo · 2023-09-05T16:58:08Z

I can take another look later today but I’m wondering why this regression wasn’t caught. Is this fixing behavior for Bazel 3? Is there some kind of regression test that can be added as part of this PR?

I think this has never been tested. The breaking change was reported in 2020 (bazelbuild/bazel#12344), and it looks like there are not so many people using rules_rust to consume Rust targets from external repositories that involve generated files 🤷

In order to test this we would need to use an external repository. I don't have enough Bazel knowledge to know if this can be reproduced without actually using another repository (maybe using another WORKSPACE in this same repo?)

scentini · 2023-09-05T17:09:29Z

Yeah, I introduced the code initially, and coming from a monorepo, external dependencies didn't come to mind.

There is no way to test it without external dependencies, however we have examples of using external dependencies under //test/cc_common_link and test/cc_common_link/with_global_alloc

jgsogo · 2023-09-06T09:07:57Z

Thanks for the pointers, @scentini . I've added one basic test to validate the new implementation (I've checked it fails with previous logic).

rust/private/rust.bzl

UebelAndre · 2023-09-06T16:29:59Z

rust/private/rust.bzl

+    if src_file.is_source or src_file.root.path != ctx.bin_dir.path:
+        src_short_path = paths.relativize(src_file.path, src_file.root.path)
+        src_symlink = ctx.actions.declare_file(paths.relativize(src_short_path, package_root))
+        ctx.actions.symlink(


Small nit, can you put the action to symlink back in _transform_sources? The thing I wanted to make crystal clear is what is happening with the path manipulation here.

Honestly I'm not sure about it, there will be more duplicated logic and more lines. As a newcomer, I find it harder to read (I was not used to the previous implementation).

Maybe I'm missing something and you have a much better proposal. I will work on the comment about the tests and come back here later to think about something better/easier.

def _symlink_for_non_generated_source(ctx, src_file, package_root): if src_file.is_source or src_file.root.path != ctx.bin_dir.path: src_short_path = paths.relativize(src_file.path, src_file.root.path) src_symlink = ctx.actions.declare_file(paths.relativize(src_short_path, package_root)) return src_symlink else: return None def _transform_sources(ctx, srcs, crate_root): has_generated_sources = len([src for src in srcs if not src.is_source]) > 0 if not has_generated_sources: return srcs, crate_root package_root = paths.dirname(paths.join(ctx.label.workspace_root, ctx.build_file_path)) generated_sources = [] for src_file in srcs: if src_file == crate_root: continue src_symlink = _symlink_for_non_generated_source(ctx, src_file, package_root) if src_symlink: ctx.actions.symlink( output = src_symlink, target_file = src_file, progress_message = "Creating symlink to source file: {}".format(src_file.path), ) generated_sources.append(src_symlink) else: generated_sources.append(src_file) generated_root = crate_root if crate_root: src_symlink = _symlink_for_non_generated_source(ctx, crate_root, package_root) if src_symlink: ctx.actions.symlink( output = src_symlink, target_file = crate_root, progress_message = "Creating symlink to source file: {}".format(src_file.path), ) generated_sources.append(src_symlink) else: generated_sources.append(crate_root) return generated_sources, generated_root

UebelAndre · 2023-09-06T16:31:50Z

.bazelci/presubmit.yml

@@ -575,6 +575,14 @@ tasks:
    test_flags:
      - "--@rules_rust//rust/toolchain/channel=nightly"
      - "--@rules_rust//:no_std=alloc"
+  generated_inputs_external_repo_ubuntu2004:


Instead of adding a new job, could you not instead make a repository rule that writes a small target to an external repo and add it to https://github.com/bazelbuild/rules_rust/blob/0.27.0/test/deps.bzl ?

I think I did the right thing here 8d0b467.

FYI. If the rust library is created in the root of the repository, then current implementation doesn't fail.

.gitignore

UebelAndre

Awesome work, thank you so much!

test/generated_inputs/BUILD.bazel

jgsogo · 2023-09-11T06:39:48Z

It's been a pleasure. Looking forward to the next release. Thanks for the hard work on this project 🙌

…bazelbuild#2138) Fixes bazelbuild#1780 In Bazel 3.7.0, the path returned by `build_file_path` changed (bazelbuild/bazel#12344), it no longer contains the `external/<repo>` prefix for external files. This breaks the expectations of this rule in the [`paths.relativize` function](https://github.com/bazelbuild/bazel-skylib/blob/main/docs/paths_doc.md#pathsrelativize) call. In this PR I prepend the `workspace_root` (`external/<repo>`) to `build_file_path` (`<path/in/the/repo>`), and use `path` (`external/<repo>/path/in/the/repo`) instead of `short_path` (`../<repo>/path/in/the/repo`). For files that are generated in different configurations I need to strip the `root` which contains the `bazel-out/darwin-fastbuild-ST-a1a0f4088294/bin/` that will never appear in the `package_root` path. ~Note.- I will probably need some help to add tests for this change~ I've tried to add one test --------- Co-authored-by: UebelAndre <[email protected]>

jgsogo added 2 commits August 29, 2023 10:58

Consider workspace_root in package_root

2b529aa

relativize for different cfgs

c8bbfb8

jgsogo marked this pull request as ready for review August 29, 2023 09:42

This comment was marked as resolved.

Sign in to view

jgsogo changed the title ~~Consider workspace_root in package_root~~ Handle files from external repos (fix breaking change in Bazel 3.7.0) Aug 29, 2023

jgsogo added 2 commits August 30, 2023 11:36

trigger CI again

0a85c6b

trigger CI again

d046a6d

UebelAndre requested changes Aug 30, 2023

View reviewed changes

Merge branch 'main' into fix/1780-external-repo

b245e57

UebelAndre requested changes Sep 4, 2023

View reviewed changes

jgsogo added 3 commits September 4, 2023 17:41

Merge remote-tracking branch 'upstream/main' into fix/1780-external-repo

d216ef9

move logic to external function

17c9d01

Merge remote-tracking branch 'upstream/main' into fix/1780-external-repo

5b90787

UebelAndre requested changes Sep 5, 2023

View reviewed changes

jgsogo added 5 commits September 6, 2023 10:30

Add test

d30367f

Add test to CI

e3895d4

fix buildifier, fmt

4ff6df5

simplify source

ef26d2f

trigger CI

e9e0a31

UebelAndre requested changes Sep 6, 2023

View reviewed changes

jgsogo added 4 commits September 7, 2023 10:32

Merge remote-tracking branch 'upstream/main' into fix/1780-external-repo

bb88bb8

Review feedback - improve comment

a891183

Review feedback - different strategy for testing

8d0b467

run buildifier

923ac33

jgsogo force-pushed the fix/1780-external-repo branch from d0b6281 to 923ac33 Compare September 7, 2023 09:35

UebelAndre requested changes Sep 7, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

jgsogo commented Sep 7, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

jgsogo added 2 commits September 7, 2023 21:51

Update .gitignore

ae8c6ba

trigger CI

d24e2dc

UebelAndre approved these changes Sep 9, 2023

View reviewed changes

test/generated_inputs/BUILD.bazel Show resolved Hide resolved

Update test/generated_inputs/BUILD.bazel

0fa922e

UebelAndre merged commit 1b19a46 into bazelbuild:main Sep 9, 2023

jgsogo deleted the fix/1780-external-repo branch September 11, 2023 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle files from external repos (fix breaking change in Bazel 3.7.0) #2138

Handle files from external repos (fix breaking change in Bazel 3.7.0) #2138

jgsogo commented Aug 29, 2023 •

edited

Loading

This comment was marked as resolved.

UebelAndre left a comment

UebelAndre Aug 30, 2023

jgsogo Aug 31, 2023 •

edited

Loading

UebelAndre Sep 4, 2023

UebelAndre Sep 4, 2023

jgsogo commented Sep 4, 2023

UebelAndre left a comment

jgsogo commented Sep 5, 2023

scentini commented Sep 5, 2023

jgsogo commented Sep 6, 2023

UebelAndre Sep 6, 2023

jgsogo Sep 7, 2023

UebelAndre Sep 6, 2023

jgsogo Sep 7, 2023

UebelAndre left a comment

jgsogo commented Sep 11, 2023

Handle files from external repos (fix breaking change in Bazel 3.7.0) #2138

Handle files from external repos (fix breaking change in Bazel 3.7.0) #2138

Conversation

jgsogo commented Aug 29, 2023 • edited Loading

This comment was marked as resolved.

UebelAndre left a comment

Choose a reason for hiding this comment

UebelAndre Aug 30, 2023

Choose a reason for hiding this comment

jgsogo Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

UebelAndre Sep 4, 2023

Choose a reason for hiding this comment

UebelAndre Sep 4, 2023

Choose a reason for hiding this comment

jgsogo commented Sep 4, 2023

UebelAndre left a comment

Choose a reason for hiding this comment

jgsogo commented Sep 5, 2023

scentini commented Sep 5, 2023

jgsogo commented Sep 6, 2023

UebelAndre Sep 6, 2023

Choose a reason for hiding this comment

jgsogo Sep 7, 2023

Choose a reason for hiding this comment

UebelAndre Sep 6, 2023

Choose a reason for hiding this comment

jgsogo Sep 7, 2023

Choose a reason for hiding this comment

UebelAndre left a comment

Choose a reason for hiding this comment

jgsogo commented Sep 11, 2023

jgsogo commented Aug 29, 2023 •

edited

Loading

jgsogo Aug 31, 2023 •

edited

Loading