#2261 Fix npm_deps tests on Windows, add a fix for Bazel 9.0.0 #2700
Conversation
|
|
|
| // When running under test, files should be in runfiles. | ||
| // This package may also be used as a run_binary(tool) and not in a test. | ||
| if (process.env.TEST_WORKSPACE) { | ||
| // On Windows, Node.js resolves junctions to their real path so __filename |
There was a problem hiding this comment.
Is that a fs-patch bug? Why does that only happen on windows? 🤔
There was a problem hiding this comment.
Not an fs-patch bug. Node.js on Windows resolves junctions (directory symlinks) to their real path when returning __filename. This is expected Windows junction behavior.
There was a problem hiding this comment.
I mean... that is a bug. I'm just not sure if we can ever fix it?
|
@Mivr can we fix the failing tests as well? |
…files - Rename _make_symlink to _make_directory_symlink since it's only used for directory symlinks (npm package store) - Update callers in npm_package_store.bzl and npm_link_package_store.bzl - Regenerate image test golden files (file sizes changed due to Windows sandbox compatibility changes in index.js) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| name, | ||
| ) | ||
| symlink = ctx.actions.declare_symlink(store_symlink_path) | ||
| ctx.actions.symlink( |
There was a problem hiding this comment.
I wonder if there's a reason this doesn't use the util method? Should we change this to make_directory_symlink?
There was a problem hiding this comment.
If we use the util method then utils won't have to expose supports_symlink_target_type?
| def _make_symlink(ctx, symlink_path, target_path): | ||
| def _make_directory_symlink(ctx, symlink_path, target_path): | ||
| symlink = ctx.actions.declare_symlink(symlink_path) | ||
| ctx.actions.symlink( |
There was a problem hiding this comment.
Can we keep the 2 params like before and only add **_symlinksTypeArgs to the end? Also make _symlinksTypeArgs a global (replace _SUPPORTS_SYMLINK_TARGET_TYPE) so we aren't constructing it 100% of the time?
There was a problem hiding this comment.
Or just if/else and each can hardcode it's args without any **
|
Replaces #2325? |
5 participants
Bazel 8.x is not compatible with Windows at the moment due to: bazelbuild/bazel#26701.
This branch fixes some common Windows issues so that the bug formentioned can be observed at all.
Then it introduces a fix for Bazel 9.0.0+ where the needed attribute is present.
Bazel 8.x will not work with rules_js until the fix is backported.
For now this seems unlikely: bazelbuild/bazel#27607
Fixes: #2261
Changes are visible to end-users: no
Test plan