fix: support for extracting rules_webtesting archives in protractor_w…

…eb_test_suite (+3 squashed commits) Squashed commits: [d93fb4d] fix: cleanup in node_repositories.bzl [3efce87] test: fix toolchain test now that nodejs_binary chooses the correct runfiles for the host platform Runfiles are no longer tested as what should be tested is that ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo.tool_files are for the correct `--platform`. [7551535] fix: fix nodejs_binary cross-platform RBE issue bazel-contrib#1305 * node_launcher.sh now picks the node binary at runtime instead of it being a templated arg evaluated at build time * node_binary now adds the correct node tool_files to the nodejs_binary runfiles Fixes bazel-contrib#1305
gregmagolan · Nov 9, 2019 · 3a49f6e · 3a49f6e
1 parent 7653a22
commit 3a49f6e
Show file tree

Hide file tree

Showing 9 changed files with 312 additions and 168 deletions.
diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml
@@ -21,6 +21,9 @@ tasks:
     - "--test_tag_filters=-e2e,-examples,-no-bazelci,-no-bazelci-ubuntu,-manual"
     test_targets:
     - "//..."
+    # //internal/node/test:nodejs_toolchain_linux_test is a "manual" test that must be run
+    # explicitly; it should pass when running on Linux with no --platform set.
+    - "//internal/node/test:nodejs_toolchain_linux_test"
   ubuntu1604_e2e:
     name: ubuntu1604_e2e
     platform: ubuntu1604
@@ -96,6 +99,9 @@ tasks:
     - "--test_tag_filters=-e2e,-examples,-no-bazelci,-no-bazelci-ubuntu,-manual"
     test_targets:
     - "//..."
+    # //internal/node/test:nodejs_toolchain_linux_test is a "manual" test that must be run
+    # explicitly; it should pass when running on Linux with no --platform set.
+    - "//internal/node/test:nodejs_toolchain_linux_test"
   ubuntu1804_debug:
     name: ubuntu1804_debug
     platform: ubuntu1804
@@ -130,7 +136,7 @@ tasks:
     - "//..."
   ubuntu1804_examples:
     name: ubuntu1804_examples
-    platform: ubuntu1604
+    platform: ubuntu1804
     # We need to reduce the memory & CPU usage of the top-level
     # bazel process for bazel-in-bazel tests to not
     # deplete the system memory completely.
@@ -156,6 +162,34 @@ tasks:
     # TODO(gregmagolan): make node_repositories acccept different archives for different platforms
     - "//examples:examples_vendored_node"
     - "//examples:examples_vendored_node_and_yarn"
+  ubuntu1804_cross_compile_darwin:
+    name: ubuntu1804_cross_compile_darwin
+    platform: ubuntu1804
+    # Build on linux with the node --platform set to darwin
+    build_flags:
+    - "--platforms=@build_bazel_rules_nodejs//toolchains/node:darwin_amd64"
+    build_targets:
+    - "//..."
+    test_flags:
+    - "--platforms=@build_bazel_rules_nodejs//toolchains/node:darwin_amd64"
+    test_targets:
+    # //internal/node/test:nodejs_toolchain_darwin_test is a "manual" test that must be run
+    # explicitly with --platforms=@build_bazel_rules_nodejs//toolchains/node:darwin_amd64 set
+    - "//internal/node/test:nodejs_toolchain_darwin_test"
+  ubuntu1804_cross_compile_windows:
+    name: ubuntu1804_cross_compile_windows
+    platform: ubuntu1804
+    # Build on linux with the node --platform set to Windows
+    build_flags:
+    - "--platforms=@build_bazel_rules_nodejs//toolchains/node:windows_amd64"
+    build_targets:
+    - "//..."
+    test_flags:
+    - "--platforms=@build_bazel_rules_nodejs//toolchains/node:windows_amd64"
+    test_targets:
+    # //internal/node/test:nodejs_toolchain_windows_test is a "manual" test that must be run
+    # explicitly with --platforms=@build_bazel_rules_nodejs//toolchains/node:windows_amd64 set
+    - "//internal/node/test:nodejs_toolchain_windows_test"
   macos:
     name: macos
     platform: macos
@@ -177,6 +211,9 @@ tasks:
     - "--test_tag_filters=-e2e,-examples,-no-bazelci,-no-bazelci-mac,-manual"
     test_targets:
     - "//..."
+    # //internal/node/test:nodejs_toolchain_darwin_test is a "manual" test that must be run
+    # explicitly; it should pass when running on OSX with no --platform set.
+    - "//internal/node/test:nodejs_toolchain_darwin_test"
   macos_e2e:
     name: macos_e2e
     platform: macos
@@ -225,13 +262,20 @@ tasks:
     - "--test_arg=--test_tag_filters=-no-bazelci,-no-bazelci-mac,-manual"
     test_targets:
     - "//..."
-  macos_cross_compile:
-    name: macos_cross_compile
+  macos_cross_compile_linux:
+    name: macos_cross_compile_linux
     platform: macos
+    # Build on mac with the node --platform set to linux
     build_flags:
     - "--platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64"
     build_targets:
     - "//..."
+    test_flags:
+    - "--platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64"
+    test_targets:
+    # //internal/node/test:nodejs_toolchain_linux_test is a "manual" test that must be run
+    # explicitly with --platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64 set
+    - "//internal/node/test:nodejs_toolchain_linux_test"
   macos_fake_rbe:
     name: macos_fake_rbe
     platform: macos
@@ -264,6 +308,9 @@ tasks:
     - "--test_tag_filters=-e2e,-examples,-no-bazelci,-no-bazelci-windows,-fix-windows,-manual"
     test_targets:
     - "//..."
+    # //internal/node/test:nodejs_toolchain_windows_test is a "manual" test that must be run
+    # explicitly; it should pass when running on Windows with no --platform set.
+    - "//internal/node/test:nodejs_toolchain_windows_test"
   windows_e2e:
     name: windows_e2e
     platform: windows
@@ -294,13 +341,20 @@ tasks:
     - "--test_arg=--local_resources=13288,1.0,1.0"
     test_targets:
     - "//..."
-  windows_cross_compile:
-    name: windows_cross_compile
+  windows_cross_compile_linux:
+    name: windows_cross_compile_linux
     platform: windows
+    # Build on windows with the node --platform set to linux
     build_flags:
     - "--platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64"
     build_targets:
     - "//..."
+    test_flags:
+    - "--platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64"
+    test_targets:
+    # //internal/node/test:nodejs_toolchain_linux_test is a "manual" test that must be run
+    # explicitly with --platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64 set
+    - "//internal/node/test:nodejs_toolchain_linux_test"
   rbe_ubuntu1604:
     name: rbe_ubuntu1604
     platform: rbe_ubuntu1604

diff --git a/internal/common/path_utils.bzl b/internal/common/path_utils.bzl
@@ -0,0 +1,21 @@
+# Copyright 2017 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Path utils
+
+Helper functions for path manipulations.
+"""
+
+def strip_external(path):
+    return path[len("external/"):] if path.startswith("external/") else path
diff --git a/internal/node/node.bzl b/internal/node/node.bzl
@@ -23,6 +23,7 @@ a `module_name` attribute can be `require`d by that name.
 load("@build_bazel_rules_nodejs//:providers.bzl", "JSNamedModuleInfo", "NodeRuntimeDepsInfo", "NpmPackageInfo", "node_modules_aspect")
 load("//internal/common:expand_into_runfiles.bzl", "expand_location_into_runfiles")
 load("//internal/common:module_mappings.bzl", "module_mappings_runtime_aspect")
+load("//internal/common:path_utils.bzl", "strip_external")
 load("//internal/common:windows_utils.bzl", "create_windows_native_launcher_script", "is_windows")
 
 def _trim_package_node_modules(package_name):
@@ -173,15 +174,17 @@ def _nodejs_binary_impl(ctx):
     if hasattr(ctx.attr, "expected_exit_code"):
         expected_exit_code = ctx.attr.expected_exit_code
 
-    node_tool_info = ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo
-
-    # Make a copy so we don't try to mutate a frozen object
-    node_tool_files = node_tool_info.tool_files[:]
-    if node_tool_info.target_tool_path == "":
-        # If tool_path is empty and tool_target is None then there is no local
-        # node tool, we will just print a nice error message if the user
-        # attempts to do bazel run
-        fail("The node toolchain was not properly configured so %s cannot be executed. Make sure that target_tool_path or target_tool is set." % ctx.attr.name)
+    # Add both the node executable for the user's local machine which is in ctx.files._node and comes
+    # from @nodejs//:node_bin and the node executable from the selected node --platform which comes from
+    # ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo.
+    # In most cases these are the same files but for RBE and when explitely setting --platform for cross-compilation
+    # any given nodejs_binary should be able to run on both the user's local machine and on the RBE or selected
+    # platform.
+    #
+    # Rules such as nodejs_image should use only ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo
+    # when building the image as that will reflect the selected --platform.
+    node_tool_files = ctx.files._node[:]
+    node_tool_files.extend(ctx.toolchains["@build_bazel_rules_nodejs//toolchains/node:toolchain_type"].nodeinfo.tool_files)
 
     node_tool_files.append(ctx.file._link_modules_script)
     node_tool_files.append(ctx.file._bazel_require_script)
@@ -211,6 +214,8 @@ def _nodejs_binary_impl(ctx):
         # also be sure to include the params file in the program inputs
         node_tool_files.append(ctx.outputs.templated_args_file)
 
+    is_vendored_node = (ctx.attr._node.label.workspace_name != "nodejs_darwin_amd64" and ctx.attr._node.label.workspace_name != "nodenodejs_linux_amd64js_darwin_amd64" and ctx.attr._node.label.workspace_name != "nodejs_windows_amd64")
+
     substitutions = {
         "TEMPLATED_args": " ".join([
             expand_location_into_runfiles(ctx, a)
@@ -221,9 +226,9 @@ def _nodejs_binary_impl(ctx):
         "TEMPLATED_expected_exit_code": str(expected_exit_code),
         "TEMPLATED_link_modules_script": _to_manifest_path(ctx, ctx.file._link_modules_script),
         "TEMPLATED_loader_path": script_path,
-        "TEMPLATED_node": node_tool_info.target_tool_path,
         "TEMPLATED_repository_args": _to_manifest_path(ctx, ctx.file._repository_args),
         "TEMPLATED_script_path": _to_execroot_path(ctx, ctx.file.entry_point),
+        "TEMPLATED_vendored_node": strip_external(ctx.file._node.path) if is_vendored_node else "",
     }
     ctx.actions.expand_template(
         template = ctx.file._launcher_template,
@@ -466,6 +471,10 @@ jasmine_node_test(
         default = Label("//internal/node:node_loader.js"),
         allow_single_file = True,
     ),
+    "_node": attr.label(
+        default = Label("@nodejs//:node_bin"),
+        allow_single_file = True,
+    ),
     "_repository_args": attr.label(
         default = Label("@nodejs//:bin/node_repo_args.sh"),
         allow_single_file = True,

diff --git a/internal/node/node_launcher.sh b/internal/node/node_launcher.sh
@@ -114,7 +114,46 @@ TEMPLATED_env_vars
 # This redirects to stderr so it doesn't interfere with Bazel's worker protocol
 # find . -name thingImLookingFor 1>&2
 
-readonly node=$(rlocation "TEMPLATED_node")
+readonly vendored_node="TEMPLATED_vendored_node"
+
+if [ -n "${vendored_node}" ]; then
+  # Use the vendored node path
+  readonly node=$(rlocation "${vendored_node}")
+else
+  # Check environment for which node path to use
+  unameOut="$(uname -s)"
+  case "${unameOut}" in
+      Linux*)     machine=linux ;;
+      Darwin*)    machine=darwin ;;
+      CYGWIN*)    machine=windows ;;
+      MINGW*)     machine=windows ;;
+      MSYS_NT*)   machine=windows ;;
+      *)          machine=linux
+                  printf "\nUnrecongized uname '${unameOut}'; defaulting to use node for linux.\n" >&2
+                  printf "Please file an issue to https://github.com/bazelbuild/rules_nodejs/issues if \n" >&2
+                  printf "you would like to add your platform to the supported rules_nodejs node platforms.\n\n" >&2
+                  ;;
+  esac
+
+  case "${machine}" in
+    # The following paths must match up with _download_node in node_repositories
+    linux) readonly node_toolchain="nodejs_linux_amd64/bin/nodejs/bin/node" ;;
+    darwin) readonly node_toolchain="nodejs_darwin_amd64/bin/nodejs/bin/node" ;;
+    windows) readonly node_toolchain="nodejs_windows_amd64/bin/nodejs/node.exe" ;;
+    *) readonly node_toolchain="nodejs_linux_amd64/bin/nodejs/bin/node" ;;
+  esac
+
+  readonly node=$(rlocation "${node_toolchain}")
+fi
+
+if [ ! -f "${node}" ]; then
+    printf "\n>>>> FAIL: The node binary '${node_toolchain} not found in runfiles.\n" >&2
+    printf "This node toolchain was chosen based on your uname '${unameOut}'.\n" >&2
+    printf "Please file an issue to https://github.com/bazelbuild/rules_nodejs/issues if \n" >&2
+    printf "you would like to add your platform to the supported rules_nodejs node platforms. <<<<\n\n" >&2
+    exit 1
+fi
+
 readonly repository_args=$(rlocation "TEMPLATED_repository_args")
 MAIN=$(rlocation "TEMPLATED_loader_path")
 readonly link_modules_script=$(rlocation "TEMPLATED_link_modules_script")