feat: add flag controlling the default for use_execroot_entry_point#2837
feat: add flag controlling the default for use_execroot_entry_point#2837acozzette wants to merge 8 commits into
Conversation
|
acozzette
force-pushed
the
use-execroot-entry-point
branch
2 times, most recently
from
62b523a to
810e38d
Compare
acozzette
force-pushed
the
use-execroot-entry-point
branch
3 times, most recently
from
b0bcf10 to
50ba91b
Compare
![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: e6ed12806e
ℹ️ 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".
|
I think this needs more docs in the Can we explain how this helps with cross-platform builds and avoids duplicating runfiles, avoids the optionDependencies bug etc, how we may switch the default in rules_js v4 etc...? |
acozzette
force-pushed
the
use-execroot-entry-point
branch
from
2b6f2cc to
7873349
Compare
|
I updated the docstring and also added those details to docs/use_execroot_entry_point.md. |
acozzette
force-pushed
the
use-execroot-entry-point
branch
from
7873349 to
76c8f7d
Compare
| entry point used is the one in that output tree (the "execroot entry point"), | ||
| rather than the copy inside the runfiles symlink tree. With everything | ||
| consolidated in `bazel-out/<target-cfg>/bin/`, Node.js sees a single | ||
| `node_modules` tree. This is the right choice for Next.js for two reasons: |
There was a problem hiding this comment.
Why bring up Next.js at this point? We were talking about generic bazel/node/node_modules and suddenly Next.js is mentioned.
Can we remain generic and just use Nextjs as an example in one sentence at the end of the paragraph?
There was a problem hiding this comment.
Good point, the info about Next.js seems very abrupt. I tweaked things to cut down on the detail a little bit and make it clear that Next.js is just one example.
This change introduces the flag `--@aspect_rules_js//js:use_execroot_entry_point`, which controls the default behavior of the `use_execroot_entry_point` option on `js_run_binary` and `js_run_devserver`. We could like to recommend moving away from enabling `use_execroot_entry_point`, and this flag provides an easy way to do that at a global level, while still making it possible to override that on specific targets if necessary. I also added CI test runs with `--@aspect_rules_js//js:use_execroot_entry_point=False` so that we have good test coverage of both modes. This required explicitly setting `use_execroot_entry_point` on some targets that only work with one more or the other. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
acozzette
force-pushed
the
use-execroot-entry-point
branch
from
76c8f7d to
79c5671
Compare
|
I realized while working on #2853 that there's really no problem with using |
|
Can you outline why it's not a problem there? That means |
|
So for |
|
Would that conflict with our goal of making |
|
I don't really think so. Since |
| @@ -0,0 +1,71 @@ | |||
| # use_execroot_entry_point | |||
There was a problem hiding this comment.
Maybe in a followup PR, but currently nothing actually links to this doc? Maybe #2852 needs to solve that as well?
| expect inputs and outputs in the same directory tree or that execute | ||
| target-platform code during the build, in which case `True` is required. | ||
|
|
||
| To disable `use_execroot_entry_point` globally, pass the build flag: |
There was a problem hiding this comment.
It doesn't disable it globally though, it just changes the default?
2 participants
This change introduces the flag
--@aspect_rules_js//js:use_execroot_entry_point, which controls the default behavior of theuse_execroot_entry_pointoption onjs_run_binary.We could like to recommend moving away from enabling
use_execroot_entry_point, and this flag provides an easy way to do that at a global level, while still making it possible to override that on specific targets if necessary.I also added CI test runs with
--@aspect_rules_js//js:use_execroot_entry_point=Falseso that we have good test coverage of both modes. This required explicitly settinguse_execroot_entry_pointon some targets that only work with one more or the other.Changes are visible to end-users: yes
There is now a
--@aspect_rules_js//js:use_execroot_entry_pointflag which can be set toTrueorFalseto determine the defaultuse_execroot_entry_pointbehavior forjs_run_binary.Test plan