From 552178e720acd175787f7685dbdd0755bf0b25a3 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 22 Dec 2021 10:32:00 -0800 Subject: [PATCH] fix(builtin): pkg_npm shouldn't assume the name of the nodejs toolchain (#3129) Instead it should just ask the resolved toolchain for the info it needs --- examples/BUILD.bazel | 8 +++++++ examples/angular_bazel_architect/WORKSPACE | 15 +++++++++++++ examples/vue/WORKSPACE | 15 +++++++++++++ internal/pkg_npm/npm_script_generator.js | 9 +++----- internal/pkg_npm/pkg_npm.bzl | 19 ++++++---------- nodejs/private/toolchains_repo.bzl | 1 - nodejs/repositories.bzl | 25 ++++++++++++---------- nodejs/toolchain.bzl | 7 ++++++ 8 files changed, 68 insertions(+), 31 deletions(-) diff --git a/examples/BUILD.bazel b/examples/BUILD.bazel index 1a2e89f80b..cb605aed04 100644 --- a/examples/BUILD.bazel +++ b/examples/BUILD.bazel @@ -225,6 +225,10 @@ example_integration_test( example_integration_test( name = "examples_vue", npm_packages = {}, + repositories = { + "//:release": "build_bazel_rules_nodejs", + "//:release-core": "rules_nodejs", + }, ) example_integration_test( @@ -249,6 +253,10 @@ example_integration_test( "@alan-agius4", "@jbedard", ], + repositories = { + "//:release": "build_bazel_rules_nodejs", + "//:release-core": "rules_nodejs", + }, tags = [ # TODO(alexeagle): re-enable when it stops timing out on 4.x branch "manual", diff --git a/examples/angular_bazel_architect/WORKSPACE b/examples/angular_bazel_architect/WORKSPACE index feb2814930..800f5ea9fc 100644 --- a/examples/angular_bazel_architect/WORKSPACE +++ b/examples/angular_bazel_architect/WORKSPACE @@ -16,6 +16,14 @@ http_archive( urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/4.4.0/rules_nodejs-4.4.0.tar.gz"], ) +# Temporary state: this example needs to have both build_bazel_rules_nodejs and rules_nodejs. +# Once we have cut over the toolchains to rules_nodejs only, we shouldn't need this. +http_archive( + name = "rules_nodejs", + sha256 = "a2b1b60c51b0193ed1646accf77a28cfd4f4ce1f6c86f32ce11455101be3a9c4", + urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/4.4.3/rules_nodejs-core-4.4.3.tar.gz"], +) + load("@build_bazel_rules_nodejs//nodejs:repositories.bzl", "rules_nodejs_dependencies") rules_nodejs_dependencies() @@ -36,3 +44,10 @@ yarn_install( symlink_node_modules = False, yarn_lock = "//:yarn.lock", ) + +load("@rules_nodejs//nodejs:repositories.bzl", "nodejs_register_toolchains") + +nodejs_register_toolchains( + name = "node16", + node_version = "16.9.0", +) diff --git a/examples/vue/WORKSPACE b/examples/vue/WORKSPACE index 9da0e94897..08a8c98d6e 100644 --- a/examples/vue/WORKSPACE +++ b/examples/vue/WORKSPACE @@ -11,10 +11,25 @@ http_archive( urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/4.4.0/rules_nodejs-4.4.0.tar.gz"], ) +# Temporary state: this example needs to have both build_bazel_rules_nodejs and rules_nodejs. +# Once we have cut over the toolchains to rules_nodejs only, we shouldn't need this. +http_archive( + name = "rules_nodejs", + sha256 = "a2b1b60c51b0193ed1646accf77a28cfd4f4ce1f6c86f32ce11455101be3a9c4", + urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/4.4.3/rules_nodejs-core-4.4.3.tar.gz"], +) + load("@build_bazel_rules_nodejs//nodejs:repositories.bzl", "rules_nodejs_dependencies") rules_nodejs_dependencies() +load("@rules_nodejs//nodejs:repositories.bzl", "nodejs_register_toolchains") + +nodejs_register_toolchains( + name = "node16", + node_version = "16.9.0", +) + load("@build_bazel_rules_nodejs//:index.bzl", "node_repositories", "npm_install") node_repositories( diff --git a/internal/pkg_npm/npm_script_generator.js b/internal/pkg_npm/npm_script_generator.js index 606510d189..d5f6ab6243 100644 --- a/internal/pkg_npm/npm_script_generator.js +++ b/internal/pkg_npm/npm_script_generator.js @@ -24,8 +24,7 @@ const fs = require('fs'); function main(args) { const - [outDir, packPath, publishPath, runNpmTemplatePath, packBatPath, publishBatPath, - runNpmBatTemplatePath] = args; + [outDir, packPath, publishPath, runNpmTemplatePath, packBatPath, publishBatPath] = args; const cwd = process.cwd(); if (/[\//]sandbox[\//]/.test(cwd)) { console.error('Error: npm_script_generator must be run with no sandbox'); @@ -36,10 +35,8 @@ function main(args) { fs.writeFileSync(packPath, npmTemplate.replace('TMPL_args', `pack "${cwd}/${outDir}"`)); fs.writeFileSync(publishPath, npmTemplate.replace('TMPL_args', `publish "${cwd}/${outDir}"`)); - const npmBatTemplate = fs.readFileSync(runNpmBatTemplatePath, {encoding: 'utf-8'}); - fs.writeFileSync(packBatPath, npmBatTemplate.replace('TMPL_args', `pack "${cwd}/${outDir}"`)); - fs.writeFileSync( - publishBatPath, npmBatTemplate.replace('TMPL_args', `publish "${cwd}/${outDir}"`)); + fs.writeFileSync(packBatPath, npmTemplate.replace('TMPL_args', `pack "${cwd}/${outDir}"`)); + fs.writeFileSync(publishBatPath, npmTemplate.replace('TMPL_args', `publish "${cwd}/${outDir}"`)); } if (require.main === module) { diff --git a/internal/pkg_npm/pkg_npm.bzl b/internal/pkg_npm/pkg_npm.bzl index ac2d9057ab..59894b7474 100644 --- a/internal/pkg_npm/pkg_npm.bzl +++ b/internal/pkg_npm/pkg_npm.bzl @@ -143,22 +143,14 @@ See the section on stamping in the [README](stamping) ), "_npm_script_generator": attr.label( default = Label("//internal/pkg_npm:npm_script_generator"), - cfg = "host", + cfg = "exec", executable = True, ), "_packager": attr.label( default = Label("//internal/pkg_npm:packager"), - cfg = "host", + cfg = "exec", executable = True, ), - "_run_npm_bat_template": attr.label( - default = Label("@nodejs//:run_npm.bat.template"), - allow_single_file = True, - ), - "_run_npm_template": attr.label( - default = Label("@nodejs//:run_npm.sh.template"), - allow_single_file = True, - ), }) # Used in angular/angular /packages/bazel/src/ng_package/ng_package.bzl @@ -283,22 +275,22 @@ def create_package(ctx, deps_files, nested_packages): def _create_npm_scripts(ctx, package_dir): args = ctx.actions.args() + toolchain = ctx.toolchains["@rules_nodejs//nodejs:toolchain_type"].nodeinfo args.add_all([ package_dir.path, ctx.outputs.pack_sh.path, ctx.outputs.publish_sh.path, - ctx.file._run_npm_template.path, + toolchain.run_npm.path, ctx.outputs.pack_bat.path, ctx.outputs.publish_bat.path, - ctx.file._run_npm_bat_template.path, ]) ctx.actions.run( progress_message = "Generating npm pack & publish scripts", mnemonic = "GenerateNpmScripts", executable = ctx.executable._npm_script_generator, - inputs = [ctx.file._run_npm_template, ctx.file._run_npm_bat_template, package_dir], + inputs = [toolchain.run_npm, package_dir], outputs = [ctx.outputs.pack_sh, ctx.outputs.publish_sh, ctx.outputs.pack_bat, ctx.outputs.publish_bat], arguments = [args], # Must be run local (no sandbox) so that the pwd is the actual execroot @@ -362,6 +354,7 @@ pkg_npm = rule( attrs = PKG_NPM_ATTRS, doc = _DOC, outputs = PKG_NPM_OUTPUTS, + toolchains = ["@rules_nodejs//nodejs:toolchain_type"], ) def pkg_npm_macro(name, tgz = None, **kwargs): diff --git a/nodejs/private/toolchains_repo.bzl b/nodejs/private/toolchains_repo.bzl index 9dcdc60268..0df444b30d 100644 --- a/nodejs/private/toolchains_repo.bzl +++ b/nodejs/private/toolchains_repo.bzl @@ -105,7 +105,6 @@ resolved_toolchain(name = "resolved_toolchain", visibility = ["//visibility:publ toolchain( name = "{platform}_toolchain", exec_compatible_with = {compatible_with}, - target_compatible_with = {compatible_with}, toolchain = "@{user_node_repository_name}_{platform}//:node_toolchain", toolchain_type = "@rules_nodejs//nodejs:toolchain_type", ) diff --git a/nodejs/repositories.bzl b/nodejs/repositories.bzl index 8c9e3993cb..f39cb985a4 100644 --- a/nodejs/repositories.bzl +++ b/nodejs/repositories.bzl @@ -521,16 +521,16 @@ if %errorlevel% neq 0 exit /b %errorlevel% # This template file is used by the packager tool and the pkg_npm rule. # `yarn publish` is not ready for use under Bazel, see https://github.com/yarnpkg/yarn/issues/610 - repository_ctx.file("run_npm.sh.template", content = """ + if repository_ctx.attr.platform.startswith("windows"): + run_npm = """ +"{node}" "{script}" TMPL_args %* +""" + else: + run_npm = """ "{node}" "{script}" TMPL_args "$@" -""".format( - node = repository_ctx.path(node_entry), - script = repository_ctx.path(npm_script), - )) +""" - repository_ctx.file("run_npm.bat.template", content = """ -"{node}" "{script}" TMPL_args %* -""".format( + repository_ctx.file("run_npm.template", content = run_npm.format( node = repository_ctx.path(node_entry), script = repository_ctx.path(npm_script), )) @@ -616,8 +616,7 @@ if %errorlevel% neq 0 exit /b %errorlevel% build_content = """# Generated by node_repositories.bzl package(default_visibility = ["//visibility:public"]) exports_files([ - "run_npm.sh.template", - "run_npm.bat.template", + "run_npm.template", "{node_entry}", "{npm_entry}", "{yarn_entry}", @@ -667,7 +666,11 @@ filegroup( if repository_ctx.attr.platform: build_content += """ load("@rules_nodejs//nodejs:toolchain.bzl", "node_toolchain") -node_toolchain(name = "node_toolchain", target_tool = ":node_bin") +node_toolchain( + name = "node_toolchain", + target_tool = ":node_bin", + run_npm = ":run_npm.template", +) """ repository_ctx.file("BUILD.bazel", content = build_content) diff --git a/nodejs/toolchain.bzl b/nodejs/toolchain.bzl index f8beb6d6f5..0651907718 100644 --- a/nodejs/toolchain.bzl +++ b/nodejs/toolchain.bzl @@ -22,6 +22,8 @@ NodeInfo = provider( "tool_files": """Files required in runfiles to make the nodejs executable available. May be empty if the target_tool_path points to a locally installed node binary.""", + "run_npm": """A template for a script that wraps npm. + On Windows, this is a Batch script, otherwise it uses Bash.""", }, ) @@ -57,6 +59,7 @@ def _node_toolchain_impl(ctx): nodeinfo = NodeInfo( target_tool_path = target_tool_path, tool_files = tool_files, + run_npm = ctx.file.run_npm, ) # Export all the providers inside our ToolchainInfo @@ -84,6 +87,10 @@ node_toolchain = rule( doc = "Path to an existing nodejs executable for the target platform.", mandatory = False, ), + "run_npm": attr.label( + doc = "A template file that allows us to execute npm", + allow_single_file = True, + ), }, doc = """Defines a node toolchain.