fix: exports_directories_only causes node to resolve from runfiles/node_modules

[thesayyn]

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature (please, look at the "Scope of the project" section in the README.md file)
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:

What is the current behavior?

fixes #3264 #3273

What is the new behavior?

Does this PR introduce a breaking change?

• Yes
• No

Other information

[gregmagolan]

For reference, the relevant code in the launcher for this change are
https://github.com/bazelbuild/rules_nodejs/blob/0ab97ba874cf3c28a0e8187b6ae4d48e732cbfa4/internal/node/launcher.sh#L215
and
https://github.com/bazelbuild/rules_nodejs/blob/0ab97ba874cf3c28a0e8187b6ae4d48e732cbfa4/internal/node/launcher.sh#L341

Resolution of entry point follows this logic

  # Entry point is the user-supplied script
  MAIN="${PWD}/"TEMPLATED_entry_point_execroot_path
  # TODO: after we link-all-bins we should not need this extra lookup
  if [[ ! -f "$MAIN" ]]; then
    if [ "$FROM_EXECROOT" = true ]; then
      MAIN="$EXECROOT/"TEMPLATED_entry_point_execroot_path
    else
      MAIN=TEMPLATED_entry_point_manifest_path
    fi
  fi

and then for a TreeArtifact (which exports_directories produces for npm packges) the path within the tree artifact is appended

  if [[ -n "TEMPLATED_entry_point_main" ]]; then
    MAIN="${MAIN}/"TEMPLATED_entry_point_main
  fi

[gregmagolan]

@thesayyn , does changing the -f in the launcher to a -e fix this without having to add the --bazel_run_from_execroot flag?

  if [[ ! -e "$MAIN" ]]; then
    if [ "$FROM_EXECROOT" = true ]; then
      MAIN="$EXECROOT/"TEMPLATED_entry_point_execroot_path
    else
      MAIN=TEMPLATED_entry_point_manifest_path
    fi
  fi

[thesayyn]

that should work but I am not sure how'd behave when the linker is disabled. this might be the reason why we have --bazel_run_from_execroot in the first place but let me play with the entry point and try to eliminate using --bazel_run_from_execroot for this issue.

[thesayyn]

does changing the -f in the launcher to a -e fix this without having to add the --bazel_run_from_execroot flag?

Yeah. that works

[gregmagolan]

does changing the -f in the launcher to a -e fix this without having to add the --bazel_run_from_execroot flag?

Yeah. that works

Great. That makes perfect sense then on the regression with exports_directories_only. Lets land the -e fix and avoid adding the bazel_run_from_execroot flag

[thesayyn]

Seems like a random repository download failure. I don't have permission to rerun the tests. Ready for review.

[gregmagolan]

oh. you had to drop the bazel-out portion here? why was that?

[thesayyn]

when the file is a tree artifact, it is going to be created by an action thus bazel will put it into the bazel-out directory. something like bazel-out/darwin_arm64-fastbuild/bin/external/npm_directory_artifacts/node_modules/pkg_with_bin
then linker will pick up this package and put it into execroot/node_modules.
so in order to perform the mapping to the linked location, it has to get rid of the bin_dir portion.

bazel-out/darwin_arm64-fastbuild/bin/external/npm_directory_artifacts/node_modules/pkg_with_bin -> node_modules/pkg_with_bin
external/npm/node_modules/pkg_with_bin -> node_modules/pkg_with_bin

[gregmagolan]

Oh. I see. This is so we can match the special case of

    if parts[0] == "external":
        if parts[2] == "node_modules":
            # external/npm/node_modules -> node_modules/foo
            # the linker will make sure we can resolve node_modules from npm
            return "/".join(parts[2:])

added 2 years ago in #1287

That bit of logic seems wrong now that we can link into sub-directories... but also seems out of scope to fix that here

[gregmagolan]

Sorted it out. We don't want to strip the output path unless we're transforming from external/<npm>/node_modules/foo to node_modules/foo. With that in mind, the function becomes

def _to_execroot_path(ctx, file):
    # Check for special case of external/<npm>/node_modules or
    # bazel-out/<platform>/bin/external/<npm>/node_modules.
    # TODO: This assumes we are linking this to the root node_modules which is not always the case
    # since the linker was updated to support linking to sub-directories since this special case was
    # added
    parts = file.path.split("/")
    if file.is_directory and not file.is_source:
        # Strip the output root to handle the case of a TreeArtifact exported by js_library with
        # exports directories only. Since a TreeArtifact is generated by an action, it resides
        # inside bazel-out directory.
        parts = parts[3:]
    if len(parts) > 3 and parts[0] == "external" and parts[2] == "node_modules":
        # Transform external/npm/node_modules/foo to node_modules/foo.
        # The linker will make sure we can resolve node_modules from npm.
        return "/".join(parts[2:])

    return file.path

[gregmagolan]

Other than that, this looks great. Thanks for tracking down the underlying issue @thesayyn . I'll push this little fix and this is landable.

[thesayyn]

Ah great. It's more robust right now. Thanks!

[thesayyn]

Ready again. bizarre dl.google.com registry errors are gone.

[gregmagolan]

🌮

[m0ar]

Awesome @thesayyn ! This has been blocking us! 💞

I know it hasn't even been 15 minutes, but got any idea when this could make it into a release @gregmagolan ? 🙏

[gregmagolan]

We should be able to get a 5.4.0 release out tomorrow. Hasn't been that much landed since 5.3.1 2 days ago. 5.3.1...stable

cc @alexeagle