From 68a6d5b054fa6626f99bb9e8075d5b0bb6fda20a Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Tue, 5 Dec 2023 15:24:45 +1100 Subject: [PATCH 1/3] Remove `pipefail` bashism from POSIX shell script --- internal/npm_install/bulk_copy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/npm_install/bulk_copy.sh b/internal/npm_install/bulk_copy.sh index 43ffd3a9..85e6bc38 100755 --- a/internal/npm_install/bulk_copy.sh +++ b/internal/npm_install/bulk_copy.sh @@ -1,6 +1,6 @@ #!/bin/sh -set -eu -o pipefail +set -eu # The list of files to copy, relative to base so directories are preserved input_files_list="$1" From 5cbc3f14a52a3f42548c84acf0a68471b7c4c389 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Wed, 6 Dec 2023 13:13:31 +1100 Subject: [PATCH 2/3] Optimise dependency specification in `npm_install` and `yarn_install` --- internal/npm_install/npm_install.bzl | 45 ++++++++++++++-------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index 64a498ae..c12c6248 100755 --- a/internal/npm_install/npm_install.bzl +++ b/internal/npm_install/npm_install.bzl @@ -308,6 +308,18 @@ data attribute. default = 3600, doc = """Maximum duration of the package manager execution in seconds.""", ), + "_inputs": attr.label_list( + default = [ + "//internal/npm_install:pre_process_package_json.js", + "//internal/npm_install:index.js", + "//internal/npm_install:bulk_copy.sh", + ], + doc = """ + Internal files used by npm_install and yarn_install. + + Listing them here allows Bazel to mark dependencies earlier, permitting fewer and faster repository fetch restarts. + """, + ) }) PROXY_ENVVARS = [ @@ -600,21 +612,14 @@ def _propagate_http_proxy_env(repository_ctx, env): def _npm_install_impl(repository_ctx): """Core implementation of npm_install.""" - _check_min_bazel_version("npm_install", repository_ctx) - - is_windows_host = is_windows_os(repository_ctx) - # Mark inputs as dependencies with repository_ctx.path to reduce repo fetch restart costs - repository_ctx.path(repository_ctx.attr.package_json) - repository_ctx.path(repository_ctx.attr.yarn_lock) - for f in repository_ctx.attr.data: - repository_ctx.path(f) + # Mark generated inputs as dependencies for cheaper restarts + # Explicit inputs are already covered: https://github.com/bazelbuild/bazel/blob/e12c44da8efb77a5d87c00e64499ac3c504ae943/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java#L519 node = repository_ctx.path(get_node_label(repository_ctx)) - npm = get_npm_label(repository_ctx) - repository_ctx.path(Label("//internal/npm_install:pre_process_package_json.js")) - repository_ctx.path(Label("//internal/npm_install:index.js")) repository_ctx.path(Label("@nodejs_%s//:node_info" % os_name(repository_ctx))) repository_ctx.path(Label("@nodejs_%s//:yarn_info" % os_name(repository_ctx))) - repository_ctx.path(Label("//internal/npm_install:bulk_copy.sh")) + npm = get_npm_label(repository_ctx) + + _check_min_bazel_version("npm_install", repository_ctx) is_windows_host = is_windows_os(repository_ctx) @@ -752,20 +757,14 @@ check if yarn is being run by the `npm_install` repository rule.""", def _yarn_install_impl(repository_ctx): """Core implementation of yarn_install.""" - _check_min_bazel_version("yarn_install", repository_ctx) - - # Mark inputs as dependencies with repository_ctx.path to reduce repo fetch restart costs - repository_ctx.path(repository_ctx.attr.package_json) - repository_ctx.path(repository_ctx.attr.yarn_lock) - for f in repository_ctx.attr.data: - repository_ctx.path(f) + # Mark generated inputs as dependencies for cheaper restarts + # Explicit inputs are already covered: https://github.com/bazelbuild/bazel/blob/e12c44da8efb77a5d87c00e64499ac3c504ae943/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java#L519 node = repository_ctx.path(get_node_label(repository_ctx)) - yarn = get_yarn_label(repository_ctx) - repository_ctx.path(Label("//internal/npm_install:pre_process_package_json.js")) - repository_ctx.path(Label("//internal/npm_install:index.js")) repository_ctx.path(Label("@nodejs_%s//:node_info" % os_name(repository_ctx))) repository_ctx.path(Label("@nodejs_%s//:yarn_info" % os_name(repository_ctx))) - repository_ctx.path(Label("//internal/npm_install:bulk_copy.sh")) + yarn = get_yarn_label(repository_ctx) + + _check_min_bazel_version("yarn_install", repository_ctx) is_windows_host = is_windows_os(repository_ctx) From da364211f27e9488003583ebb3146f0142ae9822 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Wed, 6 Dec 2023 13:14:23 +1100 Subject: [PATCH 3/3] Intentionally use bash --- internal/npm_install/bulk_copy.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/npm_install/bulk_copy.sh b/internal/npm_install/bulk_copy.sh index 85e6bc38..6543702d 100755 --- a/internal/npm_install/bulk_copy.sh +++ b/internal/npm_install/bulk_copy.sh @@ -1,6 +1,6 @@ -#!/bin/sh +#!/usr/bin/env bash -set -eu +set -eu -o pipefail # The list of files to copy, relative to base so directories are preserved input_files_list="$1"