fix(builtin): remove unnecessary loader script - #3493
Closed
kormide wants to merge 1 commit into
Closed
Conversation
kormide
force-pushed
the
remove-loader-script
branch
3 times, most recently
from
June 23, 2022 07:17
f09854b to
979223f
Compare
devversion
reviewed
Jun 23, 2022
devversion
left a comment
devversion
left a comment
Contributor
There was a problem hiding this comment.
Not an approver, but nice!!
kormide
force-pushed
the
remove-loader-script
branch
3 times, most recently
from
June 23, 2022 20:38
1b3bd9f to
4633d84
Compare
kormide
commented
Jun 23, 2022
Comment on lines
-23
to
-24
| process.env.PATH = [require('path').dirname(process.execPath), process.env.PATH].join( | ||
| /^win/i.test(process.platform) ? ';' : ':'); |
Collaborator
Author
There was a problem hiding this comment.
@alexeagle Looks like this is already done in the require patch: https://github.com/bazelbuild/rules_nodejs/blob/stable/internal/node/require_patch.cjs#L30
kormide
force-pushed
the
remove-loader-script
branch
from
June 23, 2022 22:33
4633d84 to
e7c9b3d
Compare
Collaborator
Author
|
I thought this was working but it was a false positive. The problem is that our require patch only works for node's cjs loader, not their esm loader. If we want to support esm scripts with the linker disabled, we'll need a similar monkey patch for esm. |
12 tasks
devversion
added a commit
to devversion/angular
that referenced
this pull request
Jul 11, 2022
The integration test rule now always executes tests in a directory outside of the execroot. This ensures a more hermetic test environment on platforms without a sandbox, or when tests are run with `tags: local`. As part of this change, an issue with Yarn 1.x. and `nodejs_binary` unveiled. Yarn 1.x. is resolved properly using the tool mapping but when Yarn inside Yarn is invoked (e.g. using `package.json` scripts), then the second nested Yarn invocation fails due to an unknown path. This happens because Yarn in the original invocation creates another directory in the `/tmp` directory and modifies the `PATH` so that the same `yarn` can be invoked again. This makes sense conceptually but breaks in our case because the Yarn Bash script put into the `/tmp` directory tries to resolve our vendored Yarn script relatively (which is the wrong spot here). It worked previously in the execroot because `nodejs_binary` invoked Yarn through `<external>/../node.sh <absolute-path-to-yarn-cjs-script>`, but outside the execroot it invoked Yarn using a relative path (which then ended up being preserved in the Yarn temporary directory). We can fix this by updating the Bazel NodeJS repository which seems to have coincidentally have a fix for the path resolution. See: bazel-contrib/rules_nodejs#3493 (comment).
jessicajaniuk
pushed a commit
to angular/angular
that referenced
this pull request
Jul 11, 2022
The integration test rule now always executes tests in a directory outside of the execroot. This ensures a more hermetic test environment on platforms without a sandbox, or when tests are run with `tags: local`. As part of this change, an issue with Yarn 1.x. and `nodejs_binary` unveiled. Yarn 1.x. is resolved properly using the tool mapping but when Yarn inside Yarn is invoked (e.g. using `package.json` scripts), then the second nested Yarn invocation fails due to an unknown path. This happens because Yarn in the original invocation creates another directory in the `/tmp` directory and modifies the `PATH` so that the same `yarn` can be invoked again. This makes sense conceptually but breaks in our case because the Yarn Bash script put into the `/tmp` directory tries to resolve our vendored Yarn script relatively (which is the wrong spot here). It worked previously in the execroot because `nodejs_binary` invoked Yarn through `<external>/../node.sh <absolute-path-to-yarn-cjs-script>`, but outside the execroot it invoked Yarn using a relative path (which then ended up being preserved in the Yarn temporary directory). We can fix this by updating the Bazel NodeJS repository which seems to have coincidentally have a fix for the path resolution. See: bazel-contrib/rules_nodejs#3493 (comment). PR Close #46778
jessicajaniuk
pushed a commit
to angular/angular
that referenced
this pull request
Jul 11, 2022
The integration test rule now always executes tests in a directory outside of the execroot. This ensures a more hermetic test environment on platforms without a sandbox, or when tests are run with `tags: local`. As part of this change, an issue with Yarn 1.x. and `nodejs_binary` unveiled. Yarn 1.x. is resolved properly using the tool mapping but when Yarn inside Yarn is invoked (e.g. using `package.json` scripts), then the second nested Yarn invocation fails due to an unknown path. This happens because Yarn in the original invocation creates another directory in the `/tmp` directory and modifies the `PATH` so that the same `yarn` can be invoked again. This makes sense conceptually but breaks in our case because the Yarn Bash script put into the `/tmp` directory tries to resolve our vendored Yarn script relatively (which is the wrong spot here). It worked previously in the execroot because `nodejs_binary` invoked Yarn through `<external>/../node.sh <absolute-path-to-yarn-cjs-script>`, but outside the execroot it invoked Yarn using a relative path (which then ended up being preserved in the Yarn temporary directory). We can fix this by updating the Bazel NodeJS repository which seems to have coincidentally have a fix for the path resolution. See: bazel-contrib/rules_nodejs#3493 (comment). PR Close #46778
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
FYI: @devversion
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: Related to #3277
ESM scripts couldn't be used with
nodejs_binarywhen--nobazel_run_linkerwas set. This is because the loader script is always a.cjsfile.What is the new behavior?
Remove the loader script as it no longer seems to be necessary. It would be helpful if someone with more historical context could verify this.
Does this PR introduce a breaking change?