aspect-build / aspect-build/rules_js

[Bug]: `js_run_devserver` does not sync `external/` directory

Open
#879 1 comment 0 reactions 0 assignees View on GitHub
bug
Dominant language
Starlark
Stars
378
Forks
183
Avg merge
1d 9h
Merged PRs (30d)
32

Description

### What happened?

When updating a binary to use `js_run_devserver`, we have some code that expects files to be located at `our_repo/external/some_external_repo/foo`. `js_run_devserver` does not create the files there. Instead, it lays out the temp sandbox as so:

```
tmp_dir/
our_repo/
our_repo_files
...
some_external_repo/
foo
```

This produces a layout that looks different from the default bazel runfiles directory where the files would exist like so:
```
.runfiles/
our_repo/
external/
some_external_repo/
foo
our_repo_files
...
some_external_repo/
foo
```

### Version

Development (host) and target OS/architectures: linux x86

Output of `bazel --version`: `5.3.1`

Version of the Aspect rules, or other relevant rules from your
`WORKSPACE` or `MODULE.bazel` file: `rules_js@v1.8.1` -- I haven't tested this on `rules_js@main`, but don't see any changes between 1.8.1 and now that would lead to a fix for this issue.

### How to reproduce

```shell
I don't have a reproduction available in the rules_js repo, but:
* Add a js_run_devserver target that takes a dependency on a file from an external repo
* Try to read that file from `tmp_dir/repo_name/external/external_repo/foo`
```

### Any other information?

I've traced this down to the fact that `file.short_path` inside of a rule implementation returns `../repo_name/foo` for external repo files. I've searched to see why this behavior exists and am guessing it is some legacy holdover, but haven't gone digging into the bazel source code to figure it out.

I was able to fix this by updating `js_run_devserver.bzl` with the following patch to have it lay out the `repo/external/*` directory:

```diff
diff --git a/js/private/js_run_devserver.bzl b/js/private/js_run_devserver.bzl
index d935905..4ad2cec 100644
--- a/js/private/js_run_devserver.bzl
+++ b/js/private/js_run_devserver.bzl
@@ -134,8 +134,17 @@ def _impl(ctx):
if not "/.aspect_rules_js/" in f.path:
data_files.append(f)

+ data_files_fixed = []
+ for f in data_files:
+ short_path = f.short_path
+ # Create both the external and ../ version of external repo files.
+ if short_path.startswith("../"):
+ data_files_fixed.append("external" + short_path[2:])
+
+ data_files_fixed.append(short_path)
+
config = {
- "data_files": [f.short_path for f in data_files],
+ "data_files": data_files_fixed
}
if ctx.attr.tool:
config["tool"] = ctx.executable.tool.short_path
```

My questions:
* Does it make sense to upstream a patch like this to ensure that the js_run_devserver tmp/ sandbox matches the bazel sandbox
* ~Does anyone know why this behavior exists with external repo files being placed in two places and `file.short_path` being the `../repo_name/*` version?~
* edit: I just discovered [Alex Eagle's blog post](https://blog.aspect.dev/bazelrc-flags) that mentions `--nolegacy_external_runfiles` which explains that the `my_repo/external/other_repo` symlinks are a legacy behavior.

### Fund our work

- [ ] Sponsor our open source work by donating a [bug bounty](https://opencollective.com/aspect-build/)

Contributor guide

Open the contributing guide

Research direction

Start with `js/private/js_run_devserver.bzl` and inspect how `_impl` collects `data_files` and builds the `data_files` config. Compare the sandbox layout described in the issue with the default Bazel runfiles layout, keeping the `file.short_path` behavior in mind. Done means external-repository files are accessible under the expected `repo/external/` path; the issue provides no reproduction or named test.

Written by the indexing model from the issue text.

Assessment

Domain
build-system, tooling
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.