build: fix bazel run invocations of js_binary/js_test
#30119
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
angular-robot
bot
added
the
area: build & ci
devversion
force-pushed
the
fix-runfile-execution
branch
from
0361838 to
d98ee70
Compare
alan-agius4
approved these changes
Apr 16, 2025
alan-agius4
added
the
target: patch
devversion
force-pushed
the
fix-runfile-execution
branch
from
d98ee70 to
3d90a65
Compare
After throrough investigation and lots of time spent, I figured out why the `fs` patch didn't resolve paths within the `.runfiles` directory. The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink. This is weird because in other projects there are relative symlinks to the store (this is important for the sandbox escaping FS patches)), as otherwise the dereferencing is blocked/prevented and we'd never resolve into a store directory— preventing transitive deps from being properly resolved. After figuring this out, I've tracked down the Bazel issue and found a fix: bazelbuild/bazel@51d7a3c In addition, there seems a related `rules_js` issue that has the same investigation results: aspect-build/rules_js#1877 We should try this out, but keep an eye on Firefox and Chromium. Those browser artifacts might contain files with spaces. This is incompatible with the Bazel version we are using, so the inprocess runfile creation was a workaround for this— If we remove that flag, we might cause issues for Mac users. We have a few options I think: 1. drop the flag, and hope it doesn't break any runfile generation for browsers. I suspect it won't work...— alternatively try renaming browser files. 2. keep the flag for now, and temporarily keep the disabled FS patches (needs to be manually specified by users) 3. do solution (2) but once migration allows it, update to Bazel 7— so that the bug with inprocess resolution is nicely fixed. I think (3) is a good plan moving forward and we don't need any runfile tricks. In Bazel 8, the runfiles with space issue is also resolved— allowing us to drop the inprocess runfile creation as well (if we want).
devversion
force-pushed
the
fix-runfile-execution
branch
from
3d90a65 to
7bb17d1
Compare
|
@josephperrott FYI: We should try to do the same in the components repo and revert the |
devversion
added
action: cleanup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After throrough investigation and lots of time spent, I figured out why the
fspatch didn't resolve paths within the.runfilesdirectory.The .runfiles directory of e.g. a js_binary (with bazel run) does have symlinks for e.g. node_modules/verdaccio point to the execroot directly via an absolute symlink.
This is weird because in other projects there are relative symlinks to the store (this is important for the sandbox escaping FS patches)), as otherwise the dereferencing is blocked/prevented and we'd never resolve into a store directory— preventing transitive deps from being properly resolved.
After figuring this out, I've tracked down the Bazel issue and found a fix: bazelbuild/bazel@51d7a3c
In addition, there seems a related
rules_jsissue that has the same investigation results:aspect-build/rules_js#1877
We should try this out, but keep an eye on Firefox and Chromium. Those browser artifacts might contain files with spaces. This is incompatible with the Bazel version we are using, so the inprocess runfile creation was a workaround for this— If we remove that flag, we might cause issues for Mac users.
We have a few options I think:
drop the flag, and hope it doesn't break any runfile generation for browsers. I suspect it won't work...— alternatively try renaming browser files.
keep the flag for now, and temporarily keep the disabled FS patches (needs to be manually specified by users)
do solution (2) but once migration allows it, update to Bazel 7— so that the bug with inprocess resolution is nicely fixed.
I think (3) is a good plan moving forward and we don't need any runfile tricks. In Bazel 8, the runfiles with space issue is also resolved— allowing us to drop the inprocess runfile creation as well (if we want).