rules_js icon indicating copy to clipboard operation
rules_js copied to clipboard
verified publisher icon aspect-build

js_binary and js_run_binary to support invocation across bazel workspaces

Open f-luo opened this issue 3 months ago • 13 comments

Addresses issue: https://github.com/aspect-build/rules_js/issues/2343

Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: no

Test plan

  • Covered by existing test cases

f-luo avatar Sep 06 '25 03:09 f-luo

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 06 '25 03:09 CLAassistant

Bazel 7 (Test)

All tests were cache hits

294 tests (100.0%) were fully cached saving 47s.

Bazel 8 (Test)

All tests were cache hits

257 tests (100.0%) were fully cached saving 40s.

Bazel 7 (Test)

e2e/bzlmod

All tests were cache hits

5 tests (100.0%) were fully cached saving 676ms.

Bazel 7 (Test)

e2e/git_dep_metadata

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Bazel 7 (Test)

e2e/gyp_no_install_script

All tests were cache hits

2 tests (100.0%) were fully cached saving 189ms.

Bazel 7 (Test)

e2e/js_binary_workspace

All tests were cache hits

1 test (100.0%) was fully cached saving 44ms.

Bazel 7 (Test)

e2e/js_image_oci

All tests were cache hits

1 test (100.0%) was fully cached saving 5s.

Bazel 7 (Test)

e2e/npm_link_package

All tests were cache hits

2 tests (100.0%) were fully cached saving 195ms.

Bazel 7 (Test)

e2e/npm_link_package-esm

All tests were cache hits

2 tests (100.0%) were fully cached saving 228ms.

Bazel 7 (Test)

e2e/npm_link_package-rerooted

All tests were cache hits

2 tests (100.0%) were fully cached saving 195ms.

Bazel 7 (Test)

e2e/npm_translate_lock

All tests were cache hits

3 tests (100.0%) were fully cached saving 471ms.

Bazel 7 (Test)

e2e/npm_translate_lock_disable_hooks

All tests were cache hits

3 tests (100.0%) were fully cached saving 257ms.

Bazel 7 (Test)

e2e/npm_translate_lock_empty

All tests were cache hits

2 tests (100.0%) were fully cached saving 180ms.

Bazel 7 (Test)

e2e/npm_translate_lock_exclude_package_contents

All tests were cache hits

1 test (100.0%) was fully cached saving 34ms.

Bazel 7 (Test)

e2e/npm_translate_lock_link_workspace

All tests were cache hits

2 tests (100.0%) were fully cached saving 231ms.

Bazel 7 (Test)

e2e/npm_translate_lock_multi

All tests were cache hits

2 tests (100.0%) were fully cached saving 164ms.

Bazel 7 (Test)

e2e/npm_translate_lock_partial_clone

All tests were cache hits

1 test (100.0%) was fully cached saving 61ms.

Bazel 7 (Test)

e2e/npm_translate_lock_replace_packages

All tests were cache hits

4 tests (100.0%) were fully cached saving 640ms.

Bazel 7 (Test)

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

1 test (100.0%) was fully cached saving 97ms.

Bazel 7 (Test)

e2e/npm_translate_package_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Bazel 7 (Test)

e2e/npm_translate_yarn_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Bazel 7 (Test)

e2e/package_json_module

All tests were cache hits

1 test (100.0%) was fully cached saving 282ms.

Bazel 7 (Test)

e2e/patch_from_repo

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Bazel 7 (Test)

e2e/pnpm_lockfiles

All tests were cache hits

77 tests (100.0%) were fully cached saving 9s.

Bazel 7 (Test)

e2e/pnpm_repo_install

All tests were cache hits

1 test (100.0%) was fully cached saving 989ms.

Bazel 7 (Test)

e2e/pnpm_version

All tests were cache hits

1 test (100.0%) was fully cached saving 63ms.

Bazel 7 (Test)

e2e/pnpm_workspace

All tests were cache hits

15 tests (100.0%) were fully cached saving 3s.

Bazel 7 (Test)

e2e/pnpm_workspace_deps

All tests were cache hits

3 tests (100.0%) were fully cached saving 350ms.

Bazel 7 (Test)

e2e/pnpm_workspace_rerooted

All tests were cache hits

15 tests (100.0%) were fully cached saving 3s.

Bazel 7 (Test)

e2e/repo_mapping

All tests were cache hits

3 tests (100.0%) were fully cached saving 459ms.

Bazel 7 (Test)

e2e/runfiles

All tests were cache hits

1 test (100.0%) was fully cached saving 122ms.

Bazel 7 (Test)

e2e/stamped_package_json

All tests were cache hits

1 test (100.0%) was fully cached saving 44ms.

Bazel 7 (Test)

e2e/vendored_node

All tests were cache hits

1 test (100.0%) was fully cached saving 97ms.

Bazel 7 (Test)

e2e/vendored_tarfile

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Bazel 7 (Test)

e2e/verify_patches

All tests were cache hits

2 tests (100.0%) were fully cached saving 109ms.

Bazel 7 (Test)

e2e/worker

All tests were cache hits

1 test (100.0%) was fully cached saving 35ms.

Bazel 7 (Test)

e2e/workspace

All tests were cache hits

1 test (100.0%) was fully cached saving 35ms.

Buildifier      Format

aspect-workflows[bot] avatar Sep 06 '25 03:09 aspect-workflows[bot]

@f-luo could you try prefixing the chdir value with this in js_binary itself? https://github.com/aspect-build/rules_js/blob/main/js/private/js_binary.bzl

That would then close https://github.com/aspect-build/rules_js/issues/2275.

matthewjh avatar Sep 10 '25 21:09 matthewjh

@f-luo could you try prefixing the chdir value with this in js_binary itself? https://github.com/aspect-build/rules_js/blob/main/js/private/js_binary.bzl

That would then close #2275.

I do think that would address it more broadly. My only concern is that doing it at the js_binary level would risk breaking existing usages, more specifically: if someone is already passing in external/workspace+/pkg.

Unless we also detect the presence of external/...+/ to deduplicate.

Perhaps someone from rules_js (@alexeagle) and the original creator of nextjs_* rulesets (@jbedard) can chime in here?

f-luo avatar Sep 10 '25 21:09 f-luo

I agree this should be changed in js_binary. I don't think there are any existing functional use cases that would be broken by stripping external/ from paths.

alexeagle avatar Sep 15 '25 22:09 alexeagle

Okay, that sounds good. I can take a stab at it. But as an FYI I will be traveling for the next 2 weeks, so will come back to this afterwards... unless anyone else would like to drive it in the meantime :)

f-luo avatar Sep 18 '25 18:09 f-luo

@f-luo since you're updating this... can you try reproducing the issue to demonstrate a) what's broken and b) that your fix works?

Most likely we'd need to add an e2e test? Or maybe move examples/nextjs into e2e/ so it is its own MODULE?

jbedard avatar Nov 16 '25 04:11 jbedard

@jbedard , thanks for checking in. Just repro'ed it on the latest 2.8.1. The repro steps and outcome is consistent with the report at https://github.com/aspect-build/rules_js/issues/2343.

The fix in this PR does fix it. More specifically I tried:

From same workspace as //ui:

  • Ran bazel build <nextjs_standalone_server> and bazel run <nextjs_standalone_server>

From another bazel workspace:

  • Ran bazel build @workspace//ui:<nextjs_standalone_server> and bazel run @workspace//ui:<nextjs_standalone_server>

The behavior is now consistent.

I like the idea of have a test here. Let me move some stuff around and add coverage here, thank you.

P.S. seems like one of the CI workflows failed due to disk issue:

For example:

Error in download_and_extract: java.io.IOException: Error extracting /home/runner/.cache/bazel/_bazel_runner/8d53428c79e05e77d1e55522fa21a0fd/external/rules_java~~toolchains~remotejdk11_linux/temp695009450509042695/zulu11.72.19-ca-jdk11.0.23-linux_x64.tar.gz to /home/runner/.cache/bazel/_bazel_runner/8d53428c79e05e77d1e55522fa21a0fd/external/rules_java~~toolchains~remotejdk11_linux/temp695009450509042695: write (No space left on device)

f-luo avatar Nov 16 '25 05:11 f-luo

It would be great if we could get a super simple test demonstrating it. If not simple then maybe just converting the nextjs example to an e2e test?

P.S. seems like one of the CI workflows failed due to disk issue:

Yeah, sorry the GH runners seem overloaded atm and people have been busy. I've been spamming the re-run button but you might not have permission to do that. I hope just testing locally is enough for you while working on this PR?

jbedard avatar Nov 16 '25 05:11 jbedard

Yup, np! Just added a simple e2e test. I wasn't sure if there's a hook somewhere to automate running e2e. I've testing via:

bash e2e/js_binary_workspace/test.sh

Let me know if there's anything I need to do in addition, happy to iterate on this.

Thanks again!

f-luo avatar Nov 16 '25 06:11 f-luo

Great, that is nice and simple (a lot simpler then involving nextjs in a test) 👍

We might be able to avoid the test.sh script, but otherwise this LGTM, thanks!

jbedard avatar Nov 16 '25 06:11 jbedard

I think we just need to add the e2e test to .github/workflows/ci.yaml and .aspect/workflows/config.yaml

jbedard avatar Nov 16 '25 10:11 jbedard

@alexeagle @dzbarsky can you take a look at the js_binary changes being done here? WDYT?

jbedard avatar Nov 17 '25 01:11 jbedard