fix: fix js_run_binary so tools executed with use_execroot_entry_point use exec-configuration runfiles#2823
Conversation
|
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e13346f88
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| assert_contains( | ||
| name = "run_tool_uses_exec_config_runfiles", | ||
| actual = ":run_tool_linux_arm64", | ||
| expected = "exec-config-data", |
There was a problem hiding this comment.
This assertion assumes the execution platform is never linux_arm64, but on ARM64 Linux runners the tool’s exec configuration will also match :linux_arm64_target, so target_config_marker.txt is selected and the expected exec-config-data string is wrong. That makes the new test fail on valid environments even when js_run_binary is behaving correctly; the check needs to force a distinct exec platform (or make the marker selection architecture-agnostic) to avoid host-dependent failures.
Useful? React with 👍 / 👎.
longlho
force-pushed
the
fix-js-run-binary-exec-tool-runfiles
branch
3 times, most recently
from
d1ab1c2 to
5b03c08
Compare
longlho
force-pushed
the
fix-js-run-binary-exec-tool-runfiles
branch
from
5b03c08 to
132d373
Compare
|
I have been working on this same issue and came up with a fix similar to yours recently: #2809 However, I came to the realization that we probably should not try to "fix" |
3 participants
js_run_binaryruns itstoolin exec config throughbazel-librun_binary, but previously hoisted the tool runfiles through target-config helper targets. When target and exec platforms differ, this can omit exec-platform files, including native optional npm packages such as@rolldown/binding-linux-x64-gnu.This changes the runfiles hoist to use a helper rule with
cfg = "exec"on the tool attribute. It also makes the launcher resolve execroot entry points withJS_BINARY__BINDIRwhenJS_BINARY__USE_EXECROOT_ENTRY_POINTis set, while preservingBAZEL_BINDIRfor action context.Repro repo: https://github.com/longlho/rules-js-exec-platform-repro
Changes are visible to end-users: yes
Fix
js_run_binarywithuse_execroot_entry_pointunder differing target and execution platforms, including native optional npm dependency resolution for JS tools.Test plan
npm exec --yes @bazel/bazelisk -- test //js/private/test/js_run_binary_exec_config:run_tool_uses_exec_config_runfiles --test_output=allnpm exec --yes @bazel/bazelisk -- run //:buildifier.checknpm exec --yes @bazel/bazelisk -- test //js/private/test/js_binary_sh/... //js/private/test/js_run_binary_exec_config:run_tool_uses_exec_config_runfiles --test_output=errors