fix(builtin): linker incorrectly resolves workspace node_modules for windows
#2659
Conversation
This file contains 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
devversion
force-pushed
the
fix/windows-linker-node-modules-not-linked-properly
branch
from
ab1895a to
f7e7579
Compare
…r windows Depending on whether there are generated files being stored in `bazel-out/bin/external/npm` or not, the linker might resolve the workspace node modules incorrectly for Windows. e.g. consider the manifest being the followed: `` npm/karma/bin/_karma.module_mappings.json C:/users/paul/<..>/execroot/angular_material/bazel-out/<..>/bin/external/npm/<..>` npm/node_modules/@angular/core/index.js C:/users/paul/projects/material2/node_modules/@angular/core/index.js ``` The linker currently on Windows will resolve the NPM workspace to the `bazel-out` directory. This is incorrect as on Windows, the node modules are not symlinked as with linux/macOS. Hence the node modules are not symlinked to the execroot properly and tests/builds fail due to NodeJS imports being "faulty". The fix is to simply use the runfile helpers to resolve the workspace node modules. There is no need in resolving the workspace root if we can just directly narrow the lookup to the workspace node modules. This is needed as the runfile resolution with manifest naively looks for the first entry starting with `npm` in the manifest. This breaks the assumption that the resolved workspace path refers to a directory that also contains the `node_modules` folder (as seen in the example above).
devversion
force-pushed
the
fix/windows-linker-node-modules-not-linked-properly
branch
from
f7e7579 to
ab8cc76
Compare
devversion
requested review from
alexeagle,
gregmagolan,
mattem and
soldair
as code owners
gregmagolan
reviewed
May 7, 2021
| @@ -309,7 +309,7 @@ function main(args, runfiles) { | |||
| } | |||
| else { | |||
| log_verbose(`no npm workspace node_modules folder under ${packagePath} to link to; creating node_modules directories in ${process.cwd()} for ${packagePath} 1p deps`); | |||
| yield mkdirp('${packagePath}/node_modules'); | |||
| yield mkdirp(`${packagePath}/node_modules`); | |||
gregmagolan
approved these changes
May 7, 2021
|
Thanks Paul, awesome fix 😎 |
twheys
pushed a commit
to twheys/rules_nodejs
that referenced
this pull request
Jan 13, 2022
…r windows (bazel-contrib#2659) Depending on whether there are generated files being stored in `bazel-out/bin/external/npm` or not, the linker might resolve the workspace node modules incorrectly for Windows. e.g. consider the manifest being the followed: `` npm/karma/bin/_karma.module_mappings.json C:/users/paul/<..>/execroot/angular_material/bazel-out/<..>/bin/external/npm/<..>` npm/node_modules/@angular/core/index.js C:/users/paul/projects/material2/node_modules/@angular/core/index.js ``` The linker currently on Windows will resolve the NPM workspace to the `bazel-out` directory. This is incorrect as on Windows, the node modules are not symlinked as with linux/macOS. Hence the node modules are not symlinked to the execroot properly and tests/builds fail due to NodeJS imports being "faulty". The fix is to simply use the runfile helpers to resolve the workspace node modules. There is no need in resolving the workspace root if we can just directly narrow the lookup to the workspace node modules. This is needed as the runfile resolution with manifest naively looks for the first entry starting with `npm` in the manifest. This breaks the assumption that the resolved workspace path refers to a directory that also contains the `node_modules` folder (as seen in the example above).
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.
Depending on whether there are generated files being stored in
bazel-out/bin/external/npmor not, the linker might resolve theworkspace node modules incorrectly for Windows. e.g.
consider the manifest being the followed:
The linker currently on Windows will resolve the NPM workspace to the
bazel-outdirectory. This is incorrect as on Windows, the node modulesare not symlinked as with linux/macOS. Hence the node modules are not
symlinked to the execroot properly and tests/builds fail due to NodeJS
imports being "faulty".
The fix is to simply use the runfile helpers to resolve the workspace
node modules. There is no need in resolving the workspace root if we
can just directly narrow the lookup to the workspace node modules.
This is needed as the runfile resolution with manifest naively looks
for the first entry starting with
npmin the manifest. This breaksthe assumption that the resolved workspace path refers to a directory
that also contains the
node_modulesfolder (as seen in the exampleabove).