From 51e33fc6f0aa8cf37be1174306bbd1de06302600 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 1 Mar 2022 22:13:37 +0100 Subject: [PATCH] fix(builtin): `yarn_install` with vendored yarn `.cjs` file breaks `yarn_install` allows for a vendored/custom Yarn JavaScript file. To ensure that Yarn runs with the proper Node version, `yarn_install` calls the JS file with the host NodeJS version also installed through Bazel. Currently the detection logic does not account for `.cjs` or `.mjs`. This is a little confusing as the install continues to work but breaks surprisingly in Windows then, or when a engines requirement is no longer satisifed (there are more reasons as well) We should support `cjs` and `mjs` to avoid confusion, and especially `cjs` since Yarn itself always vendors itself with that extension! --- docs/Built-ins.md | 4 +-- internal/common/is_js_file.bzl | 38 ++++++++++++++++++++++++++++ internal/js_library/js_library.bzl | 12 ++------- internal/npm_install/npm_install.bzl | 5 ++-- 4 files changed, 45 insertions(+), 14 deletions(-) create mode 100644 internal/common/is_js_file.bzl diff --git a/docs/Built-ins.md b/docs/Built-ins.md index 6b1be56eb4..9a49109929 100755 --- a/docs/Built-ins.md +++ b/docs/Built-ins.md @@ -830,7 +830,7 @@ will modify files in your workspace. NB: If `symlink_node_modules` is enabled, the node_modules folder is re-used between executions of the repository rule. Patches may be re-applied to files in this case and fail to apply. A marker file - `node_modules/.bazel-post-install-patches` is left in this mode when patches are applied. When the + `node_modules/.bazel-post-install-patches` is left in this mode when patches are applied. When the marker file is detected, patch file failures are treated as WARNINGS. For this reason, it is recommended to patch npm packages with an npm tool such as https://www.npmjs.com/package/patch-package when `symlink_node_modules` is enabled which handles re-apply patching logic more robustly. @@ -1496,7 +1496,7 @@ will modify files in your workspace. NB: If `symlink_node_modules` is enabled, the node_modules folder is re-used between executions of the repository rule. Patches may be re-applied to files in this case and fail to apply. A marker file - `node_modules/.bazel-post-install-patches` is left in this mode when patches are applied. When the + `node_modules/.bazel-post-install-patches` is left in this mode when patches are applied. When the marker file is detected, patch file failures are treated as WARNINGS. For this reason, it is recommended to patch npm packages with an npm tool such as https://www.npmjs.com/package/patch-package when `symlink_node_modules` is enabled which handles re-apply patching logic more robustly. diff --git a/internal/common/is_js_file.bzl b/internal/common/is_js_file.bzl new file mode 100644 index 0000000000..45b359be5a --- /dev/null +++ b/internal/common/is_js_file.bzl @@ -0,0 +1,38 @@ +# 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. + +"""Helper function and variable for determining JavaScript files.""" + +JS_FILE_EXTENSIONS = ["js", "mjs", "cjs"] + +def is_javascript_filename(filename, include_map_files = False): + """Gets whether the specified filename corresponds to a JavaScript file. + + Args: + filename: File name to check. + include_map_files: Whether corresponding `.map` files should also return `True`. + + Returns: + A boolean indicating whether the file corresponds to a JavaScript file. + """ + for extension in JS_FILE_EXTENSIONS: + if filename.endswith(".%s" % extension): + return True + if include_map_files and filename.endswith(".%s.map" % extension): + return True + return False + +def is_javascript_file(file, include_map_files = False): + """Gets whether the specified Bazel `File` corresponds to a JavaScript file.""" + return is_javascript_filename(file.basename, include_map_files) diff --git a/internal/js_library/js_library.bzl b/internal/js_library/js_library.bzl index 7e27505c56..d08fd49eeb 100644 --- a/internal/js_library/js_library.bzl +++ b/internal/js_library/js_library.bzl @@ -30,6 +30,7 @@ load( "js_ecma_script_module_info", "js_named_module_info", ) +load("//internal/common:is_js_file.bzl", "is_javascript_file") load( "//third_party/github.com/bazelbuild/bazel-skylib:rules/private/copy_file_private.bzl", "copy_bash", @@ -105,15 +106,6 @@ def write_amd_names_shim(actions, amd_names_shim, targets): amd_names_shim_content += "define(\"%s\", function() { return %s });\n" % n actions.write(amd_names_shim, amd_names_shim_content) -JS_EXTENSIONS = ["js", "mjs", "cjs"] - -def _is_javascript_file(file): - for extension in JS_EXTENSIONS: - if file.basename.endswith(".%s" % extension) or \ - file.basename.endswith(".%s.map" % extension): - return True - return False - def _to_manifest_path(ctx, file): if file.short_path.startswith("../"): return file.short_path[3:] @@ -169,7 +161,7 @@ def _impl(ctx): file = dst # register js files - if _is_javascript_file(file) or file.basename.endswith(".json"): + if is_javascript_file(file, include_map_files = True) or file.basename.endswith(".json"): js_files.append(file) # register typings diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index 6e14ba0e86..13f02b10a9 100644 --- a/internal/npm_install/npm_install.bzl +++ b/internal/npm_install/npm_install.bzl @@ -24,6 +24,7 @@ See discussion in the README. load("@rules_nodejs//nodejs/private:os_name.bzl", "is_windows_os", "os_name") load("@rules_nodejs//nodejs/private:node_labels.bzl", "get_node_label", "get_npm_label") load("//:version.bzl", "VERSION") +load("//internal/common:is_js_file.bzl", "is_javascript_filename") load("@bazel_skylib//lib:paths.bzl", "paths") COMMON_ATTRIBUTES = dict(dict(), **{ @@ -240,7 +241,7 @@ will modify files in your workspace. NB: If `symlink_node_modules` is enabled, the node_modules folder is re-used between executions of the repository rule. Patches may be re-applied to files in this case and fail to apply. A marker file - `node_modules/.bazel-post-install-patches` is left in this mode when patches are applied. When the + `node_modules/.bazel-post-install-patches` is left in this mode when patches are applied. When the marker file is detected, patch file failures are treated as WARNINGS. For this reason, it is recommended to patch npm packages with an npm tool such as https://www.npmjs.com/package/patch-package when `symlink_node_modules` is enabled which handles re-apply patching logic more robustly.""", @@ -829,7 +830,7 @@ def _yarn_install_impl(repository_ctx): if is_windows_host and _repository_contains_file(repository_ctx, yarn_label.workspace_name, "bin/yarn.cmd"): yarn_label = yarn_label.relative(":bin/yarn.cmd") - if yarn_label.name.endswith(".js"): + if is_javascript_filename(yarn_label.name): yarn_cmd = [node, yarn_label] else: # Our wrapper scripts include the "node" executable