Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable linker with nodejs_binary #1382

Closed
Toxicable opened this issue Nov 22, 2019 · 2 comments
Closed

enable linker with nodejs_binary #1382

Toxicable opened this issue Nov 22, 2019 · 2 comments

Comments

@Toxicable
Copy link

🚀 feature request

Tracking enabling the linker with nodejs_binary

@mrmeku
Copy link
Collaborator

mrmeku commented Dec 4, 2019

Making nodejs_binary use the linker

Problem

Currently nodejs_binary makes use of a node_modules within external/npm. This has caused rules_nodejs to have to maintain a custom require script such that source files in a user's workspace can require files as if they existed within a node_modules directory in the root of their repo.

Unfortunately this strategy does not always work as expected in practice. Many libraries are written with the expectation of a directory layout that mirrors the node module resolution algorithm. Even Angular's own karma testing library has that expectation written in multiple places.

Solution

Rather than making custom patches or PRs for every library which has such an expectation, we instead would rather just provide the directory structure that programs expect to ensure maximum compatibility with execution under bazel

Hooking up the linker

Taking run_node as inspiration (since it already hooked up to the linker), we need to do a couple of things to successfully hook up nodejs_binary.

First and foremost we need to do something similar to what register_node_modules_linker does under the hood.

  1. Write a module manifest to a known location (we can use the existing implementation if we extract it to a helper function)
  2. Inform our binary about the module manifest using the --bazel_node_modules_manifest flag. (NOTE: register_node_modules_linker can add this argument directly but we cannot since the build step of nodejs_binary does not invoke an action, but rather makes an .sh script which can be run using bazel run)

So assuming we extract the module manifest creation logic to a helper function and call it directly, we then need to add its location to our generated .sh script.

The current strategy for adding file locations to our runner script is via the use of TEMPLATED variables that are expanded using ctx.actions.expand_tempalate here. We will need to add one more templated variable for this purpose.

If the templated variable is populated, we need only to set $MODULES_MANIFEST within node_launcher.sh to its value to have successfully hooked up the linker.

Setting the right executable

Hooking up the linker is not sufficient however. We need to ensure that the binary we are executing is the one that is linked to node_modules rather than the unlinked version which lives within external/npm. This will require changes either to node_loader.js itself or how it is called.

node_loader.js is called assuming the second argument is the binary / entry point to launch. Here, the loader could be changed to map the entry point specified from external/npm to the linked version. Otherwise the caller (aka node_launcher.sh) will have chanbge how it calls node_loader.js to accomplish that goal.

Hopefully I'm not missing something, but I think that should be all the steps!

@alexeagle
Copy link
Collaborator

Maybe we can change the behavior with a flag, and set that flag from all the generated binaries and tests that generate_build_files produces for npm bin - that could still make 1.0 in theory

alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Dec 6, 2019
- nodejs_binary and nodejs_test will always produce a modules manifest needed by the linker
- if the program isn't run with a user-specific linker manifest, we use the static one as a default
- always pass the flag to opt-out of custom module resolver for generated rules

Fixes bazel-contrib#1382
alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Dec 7, 2019
- nodejs_binary and nodejs_test will always produce a modules manifest needed by the linker
- if the program isn't run with a user-specific linker manifest, we use the static one as a default
- always pass the flag to opt-out of custom module resolver for generated rules

Fixes bazel-contrib#1382
alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Dec 7, 2019
- nodejs_binary and nodejs_test will always produce a modules manifest needed by the linker
- if the program isn't run with a user-specific linker manifest, we use the static one as a default
- always pass the flag to opt-out of custom module resolver for generated rules

Fixes bazel-contrib#1382
mrmeku pushed a commit to mrmeku/rules_nodejs that referenced this issue Dec 9, 2019
- nodejs_binary and nodejs_test will always produce a modules manifest needed by the linker
- if the program isn't run with a user-specific linker manifest, we use the static one as a default
- always pass the flag to opt-out of custom module resolver for generated rules

Fixes bazel-contrib#1382
alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Dec 10, 2019
- nodejs_binary and nodejs_test will always produce a modules manifest needed by the linker
- if the program isn't run with a user-specific linker manifest, we use the static one as a default
- always pass the flag to opt-out of custom module resolver for generated rules

Fixes bazel-contrib#1382
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants