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

Add yarn_run/npm_run rules / Add node_modules to execroot #2507

Closed
elado opened this issue Mar 5, 2021 · 12 comments
Closed

Add yarn_run/npm_run rules / Add node_modules to execroot #2507

elado opened this issue Mar 5, 2021 · 12 comments
Labels
Can Close? We will close this in 30 days if there is no further activity enhancement

Comments

@elado
Copy link

elado commented Mar 5, 2021

🚀 feature request

Relevant Rules

Add new rules - yarn_run and npm_run

Description

I would like to revive #362 and suggest adding yarn_run / npm_run rules, and/or symlink node_modules in execroot sandbox.

While this may not be "the bazel way", I believe it'll be tremendously helpful with migration of existing projects that rely heavily on package scripts.

The major gap I see currently is the lack of presence of node_modules in the current execution directory.

Describe the solution you'd like

In a genrule, I can access yarn binary using $(execpath @nodejs//:yarn), and execute package scripts like

genrule(
    name = "foo",
    outs = [...],
    cmd = """
        $(execpath @nodejs//:yarn) script-from-package-json
    """,
    tools = [
        "@nodejs//:yarn",
        "@npm//:node_modules",
        "package.json",
        ...
    ],
)

However, any binary access that my package.json scripts rely on cannot be found because the CWD where yarn is running is in a sandbox execroot which doesn't contain node_modules:

[tmpdir]/sandbox/[os]-sandbox/[id]/execroot/[project]

So scripts like "build": "tsc index.ts --outDir .." can't find tsc.

The node_modules are symlinked in [tmpdir]/external/npm/node_modules.

I can think of two possible solutions for yarn_run/npm_run rules:

  1. Add external/npm/node_modules/.bin to the PATH before executing the script.
  2. Symlink node_modules in the sandbox. I believe this is more of a bulletproof solution, because I've seen some node modules that rely on file system and try to access the closes node_modules in the tree. Also, with workspaces, there could be multiple nested node_modules, so the .bin depends on where a script is executed from (chdir usage), so if this is the approach the tree need to be rebuilt accordingly.

As a POC, manual symlink of node_modules into the sandbox execroot works (is there a more resilient way to get to external/npm/node_modules?)

genrule(
    name = "build",
    outs = [..],
    cmd = """
        ln -s ../../../../../external/npm/node_modules .

        $(execpath @nodejs//:yarn) build
    """,
    tools = [
        "@nodejs//:yarn",
        "@npm//:node_modules",
        "package.json",
        "index.ts",
    ],
)

Missing node_modules/.bin in sh_test sandbox

Additionally, when running a test (sh_test), it's sandboxed in a different folder. When specifying packages (rather than @npm//:node_modules) they are symlinked individually, but the .bin folder is missing.

sandbox/darwin-sandbox/[id]/execroot/[project]/bazel-out/darwin-fastbuild/bin/[test_name].runfiles/npm/node_modules

Ideally .bin is also available for yarn (maybe @npm//.bin/tsc) so it can execute scripts by name.

Passing locations to scripts

With all that, another thing that *_run requires is to pass the location of the expected outs.
Env vars could help, but it's still not ideal:

// package.json
"build": "tsc index.ts --outDir ${TS_OUT_DIR:-lib}"
yarn_run(
    name = "build",
    outs = ["lib/index.js"],
    script_name = "build",
    env = [{ "TS_OUT_DIR": "$(location lib/index.js)" }],
    tools = [
        "@nodejs//:yarn",
        "@npm//:node_modules",
        "package.json",
        "index.ts",
    ],
)

Describe alternatives you've considered

  • Create a genrule and symlink node_modules into sandbox execroot
  • Add node_modules/.bin to PATH with something like export PATH=path/to/node_modules/.bin:$$PATH
@alexeagle
Copy link
Collaborator

I totally agree we should have this - we aren't opinionated about "the bazel way" in rules_nodejs.

@github-actions
Copy link

github-actions bot commented Jun 8, 2021

This issue has been automatically marked as stale because it has not had any activity for 90 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Jun 8, 2021
@ljharb
Copy link

ljharb commented Jun 8, 2021

bump

@github-actions github-actions bot removed the Can Close? We will close this in 30 days if there is no further activity label Jun 9, 2021
@github-actions
Copy link

github-actions bot commented Sep 8, 2021

This issue has been automatically marked as stale because it has not had any activity for 90 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Sep 8, 2021
@ljharb
Copy link

ljharb commented Sep 8, 2021

bump

@github-actions github-actions bot removed the Can Close? We will close this in 30 days if there is no further activity label Sep 9, 2021
@github-actions
Copy link

github-actions bot commented Dec 8, 2021

This issue has been automatically marked as stale because it has not had any activity for 90 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Dec 8, 2021
@ljharb
Copy link

ljharb commented Dec 8, 2021

bump

@github-actions github-actions bot removed the Can Close? We will close this in 30 days if there is no further activity label Dec 9, 2021
@softshipper
Copy link

can we use this rule?

@alexeagle
Copy link
Collaborator

@softshipper this is a feature request, no such rule exists. Help wanted :)

@shadogar
Copy link

shadogar commented Apr 5, 2022

+1 to this issue I think it would be massive to be able to do this

@github-actions
Copy link

github-actions bot commented Oct 6, 2022

This issue has been automatically marked as stale because it has not had any activity for 6 months. It will be closed if no further activity occurs in 30 days. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Oct 6, 2022
@github-actions
Copy link

github-actions bot commented Nov 6, 2022

This issue was automatically closed because it went 30 days without any activity since it was labeled "Can Close?"

@github-actions github-actions bot closed this as completed Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can Close? We will close this in 30 days if there is no further activity enhancement
Projects
None yet
Development

No branches or pull requests

5 participants