fix: allow js_run_binary to hoist runfiles in the exec configuration#2809
fix: allow js_run_binary to hoist runfiles in the exec configuration#2809acozzette wants to merge 7 commits intoaspect-build:mainfrom
Conversation
|
acozzette
force-pushed
the
js-run-binary
branch
10 times, most recently
from
a69a993 to
82a7e1d
Compare
acozzette
force-pushed
the
js-run-binary
branch
from
006bf1b to
2703b36
Compare
The `js_run_binary` macro has a `use_execroot_entry_point` option which is enabled by default and which hoists out the runfiles into the execroot. Currently this hoisting effectively rebuilds the runfiles in the target platform config, but they should really be in the exec config since they consist of code that is going to run during the build action. This change fixes that problem by making sure we hoist the runfiles in the exec config. I also updated the launcher script, since it needs to be prepared to look for the entry point in a different directory. This is all guarded by the `hoist_runfiles_to_exec_cfg` option on `js_run_binary`, which effectively defaults to False for now. The default is actually `None` but only for technical reasons. The default is controlled by a new flag `//js:_hoist_runfiles_to_exec_cfg`, but this is intended only for our own CI purposes, allowing us to run most tests against both behaviors. There are a handful of `js_run_binary` targets that require one behavior or the other, so I updated those ones to explicitly specify the flag they need. This addresses aspect-build#2754, aspect-build#2121, and aspect-build#2073, but does not automatically fix them yet since the fix is opt-in for now. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
acozzette
force-pushed
the
js-run-binary
branch
from
442874b to
bb975a4
Compare
acozzette
changed the title
![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: bb975a4474
ℹ️ 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".
| @@ -19,7 +19,7 @@ bazel_dep(name = "rules_nodejs", version = "6.7.3") | |||
|
|
|||
| # Changes ensured by rules_js: | |||
| # 3.2.2: https://github.com/bazel-contrib/bazel-lib/commit/cac2d7855949d1b222fa26888892fbbe1d31015d | |||
There was a problem hiding this comment.
Add a comment just like this for the reason we need 3.3.0?
| # TEST: js_test(data = js_run_binary(srcs)) --------------- | ||
| js_run_binary( | ||
| name = "run-write-srcs", | ||
| srcs = ["data.json"], |
There was a problem hiding this comment.
This was essentially an undeclared input before, but magically worked because the tool also declared the input? Now it correctly fails with the new option enabled?
There was a problem hiding this comment.
Yes, that's right. With the new option enabled and without this line adding the dep via srcs, this would fail since data.json would no longer be in the same place.
| local suffix | ||
| if [[ "$short_path" == ../* ]]; then | ||
| echo "$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/external/${short_path:3}" | ||
| suffix="external/${short_path:3}" |
There was a problem hiding this comment.
Can we just modify short_parth instead of introducing the extra suffix var? Any disadvantage to that?
| # This flag controls the default value of the use_execroot_exec_cfg flag on | ||
| # js_run_binary. | ||
| bool_flag( | ||
| name = "use_execroot_exec_cfg", |
There was a problem hiding this comment.
WDYT about prefixing this with js_run_binary_ since that is the one and only usage?
2 participants
The
js_run_binarymacro has ause_execroot_entry_pointoption which is enabled by default and which hoists out the runfiles into the execroot. Currently this hoisting effectively rebuilds the runfiles in the target platform config, but they should really be in the exec config since they consist of code that is going to run during the build action.This change fixes that problem by making sure we hoist the runfiles in the exec config. I also updated the launcher script, since it needs to be prepared to look for the entry point in a different directory. This is all guarded by the
hoist_runfiles_to_exec_cfgoption onjs_run_binary, which effectively defaults to False for now. The default is actuallyNonebut only for technical reasons.The default is controlled by a new flag
//js:_hoist_runfiles_to_exec_cfg, but this is intended only for our own CI purposes, allowing us to run most tests against both behaviors. There are a handful ofjs_run_binarytargets that require one behavior or the other, so I updated those ones to explicitly specify the flag they need.This addresses #2754, #2121, and #2073, but does not automatically fix them yet since the fix is opt-in for now.
Co-Authored-By: Claude Sonnet 4.6 [email protected]
Changes are visible to end-users: yes
The
js_run_binarymacro now supports ahoist_runfiles_to_exec_cfgoption, which will cause runfiles to be hoisted in the exec configuration instead of the target configuration. This is currently disabled by default, but you can opt in to achieve a more correct build setup.Test plan