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-robotbot added the area: build & ci 
devversionforce-pushed the fix-runfile-execution branch from 0361838 to d98ee70Compare
alan-agius4 approved these changes Apr 16, 2025
alan-agius4 added the target: patch devversionforce-pushed the fix-runfile-execution branch from d98ee70 to 3d90a65Compare
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).
devversionforce-pushed the fix-runfile-execution branch from 3d90a65 to 7bb17d1Compare
@josephperrott FYI: We should try to do the same in the components repo and revert the |
devversion added action: cleanup Sign up for freeto 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).