From 8ff9f66d39c3a9455640a31d189acdacf48e209d Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Thu, 10 Dec 2020 22:24:25 -0800 Subject: [PATCH] refactor: make dependencies on --bazel_patch_module_resolver explicit We are about to flip the default for this flag in #2125 so we need to explicitly opt-in in places where we need the behavior. Ideally most of these should be fixed, this would be necessary if we ever want to deprecate the module resolver patches --- e2e/node_loader_preserve_symlinks/BUILD.bazel | 2 ++ e2e/packages/BUILD.bazel | 4 ++++ e2e/typescript/BUILD.bazel | 4 ++++ examples/angular/tools/BUILD.bazel | 2 ++ examples/kotlin/BUILD.bazel | 6 +++++- examples/user_managed_deps/BUILD.bazel | 2 ++ internal/node/test/BUILD.bazel | 9 ++++++++- internal/node/test/binary_as_data/BUILD.bazel | 2 ++ internal/npm_install/BUILD.bazel | 2 ++ internal/npm_install/test/BUILD.bazel | 7 +++++++ internal/pkg_npm/BUILD.bazel | 2 ++ internal/pkg_web/BUILD.bazel | 4 ++++ packages/jasmine/test/BUILD.bazel | 8 ++++++++ packages/labs/grpc_web/BUILD.bazel | 2 ++ packages/labs/test/grpc_web/BUILD.bazel | 2 ++ packages/node-patches/BUILD.bazel | 6 +++++- packages/protractor/protractor_web_test.bzl | 2 ++ packages/protractor/test/protractor-utils/BUILD.bazel | 3 +++ .../bazelbuild/rules_typescript/internal/BUILD.bazel | 3 +++ 19 files changed, 69 insertions(+), 3 deletions(-) diff --git a/e2e/node_loader_preserve_symlinks/BUILD.bazel b/e2e/node_loader_preserve_symlinks/BUILD.bazel index ff7bc2228f..edf8636a04 100644 --- a/e2e/node_loader_preserve_symlinks/BUILD.bazel +++ b/e2e/node_loader_preserve_symlinks/BUILD.bazel @@ -9,4 +9,6 @@ nodejs_test( ], entry_point = ":node_loader_test.spec.js", node_modules = "@npm//:node_modules", + # legacy "node_modules" attribute means linker didn't see deps + templated_args = ["--bazel_patch_module_resolver"], ) diff --git a/e2e/packages/BUILD.bazel b/e2e/packages/BUILD.bazel index 8af7845a75..d523fd3f82 100644 --- a/e2e/packages/BUILD.bazel +++ b/e2e/packages/BUILD.bazel @@ -40,6 +40,8 @@ nodejs_test( node_modules = "@e2e_packages_npm_install_duplicate_for_determinism_testing//:node_modules", # TODO(gregmagolan): fix this test on windows tags = ["fix-windows"], + # TODO: use runfiles + templated_args = ["--bazel_patch_module_resolver"], ) nodejs_test( @@ -52,4 +54,6 @@ nodejs_test( node_modules = "@e2e_packages_yarn_install_duplicate_for_determinism_testing//:node_modules", # TODO(gregmagolan): fix this test on windows tags = ["fix-windows"], + # TODO: use runfiles + templated_args = ["--bazel_patch_module_resolver"], ) diff --git a/e2e/typescript/BUILD.bazel b/e2e/typescript/BUILD.bazel index 27c9d8faae..9f225bd1ca 100644 --- a/e2e/typescript/BUILD.bazel +++ b/e2e/typescript/BUILD.bazel @@ -51,6 +51,10 @@ jasmine_node_test( # Verify that worker_protocol.proto can be referenced as a target in the generated npm bazel workspace "@npm//@bazel/typescript/third_party/github.com/bazelbuild/bazel/src/main/protobuf:worker_protocol.proto", ], + templated_args = [ + # ts_library produces named AMD output with repo name in the require statement + "--bazel_patch_module_resolver", + ], deps = [ ":test_lib", ], diff --git a/examples/angular/tools/BUILD.bazel b/examples/angular/tools/BUILD.bazel index d2f335e89b..43fc2cdc90 100644 --- a/examples/angular/tools/BUILD.bazel +++ b/examples/angular/tools/BUILD.bazel @@ -11,5 +11,7 @@ nodejs_binary( "@npm//@bazel/typescript", ], entry_point = "@npm//:node_modules/@bazel/typescript/internal/tsc_wrapped/tsc_wrapped.js", + # TODO: turn on --worker_sandboxing and remove this flag to see failure to load the plugin + templated_args = ["--bazel_patch_module_resolver"], visibility = ["//:__subpackages__"], ) diff --git a/examples/kotlin/BUILD.bazel b/examples/kotlin/BUILD.bazel index db45b11943..9d211cd257 100644 --- a/examples/kotlin/BUILD.bazel +++ b/examples/kotlin/BUILD.bazel @@ -74,5 +74,9 @@ jasmine_node_test( ":bundle", "@npm//domino", ], - templated_args = ["--node_options=--experimental-modules"], + templated_args = [ + # TODO: don't rely on patching require() + "--bazel_patch_module_resolver", + "--node_options=--experimental-modules", + ], ) diff --git a/examples/user_managed_deps/BUILD.bazel b/examples/user_managed_deps/BUILD.bazel index 808b886076..19806397fa 100644 --- a/examples/user_managed_deps/BUILD.bazel +++ b/examples/user_managed_deps/BUILD.bazel @@ -43,6 +43,8 @@ jasmine_node_test( srcs = glob(["*.spec.js"]), node_modules = "//:node_modules", tags = ["no-local-jasmine-deps"], + # user-managed deps don't expose the provider the linker needs to make require work + templated_args = ["--bazel_patch_module_resolver"], deps = [ ":decrement", ":program", diff --git a/internal/node/test/BUILD.bazel b/internal/node/test/BUILD.bazel index 6c1d0398a3..ea1c999313 100644 --- a/internal/node/test/BUILD.bazel +++ b/internal/node/test/BUILD.bazel @@ -16,12 +16,13 @@ nodejs_binary( ) # You can have a nodejs_binary with a node_modules attribute -# and no fine grained deps +# and no fine grained deps, but it requires patching the module resolver nodejs_binary( name = "has_deps_legacy", data = ["has-deps.js"], entry_point = ":has-deps.js", node_modules = "@fine_grained_deps_yarn//:node_modules", + templated_args = ["--bazel_patch_module_resolver"], ) # You can have a nodejs_binary with no node_modules attribute @@ -101,6 +102,8 @@ nodejs_test( "@npm//node_resolve_nested_main", ], entry_point = ":module_resolution.spec.js", + # this is a test for the patched module resolver + templated_args = ["--bazel_patch_module_resolver"], ) nodejs_test( @@ -218,6 +221,8 @@ nodejs_test( "@npm//node_resolve_nested_main", ], entry_point = ":module_resolution_built.spec.js", + # TODO: passes locally without this flag but fails on CircleCI + templated_args = ["--bazel_patch_module_resolver"], ) nodejs_test( @@ -227,6 +232,8 @@ nodejs_test( ":genfile-runfile", ], entry_point = ":data_resolution_built.spec.js", + # TODO: fails on Windows without this flag + templated_args = ["--bazel_patch_module_resolver"], ) npm_package_bin( diff --git a/internal/node/test/binary_as_data/BUILD.bazel b/internal/node/test/binary_as_data/BUILD.bazel index 2220436925..f695d4dce5 100644 --- a/internal/node/test/binary_as_data/BUILD.bazel +++ b/internal/node/test/binary_as_data/BUILD.bazel @@ -9,4 +9,6 @@ nodejs_test( name = "main_bin_data_test", data = [":main_bin"], entry_point = "test.js", + # TODO: fails on Windows without this flag + templated_args = ["--bazel_patch_module_resolver"], ) diff --git a/internal/npm_install/BUILD.bazel b/internal/npm_install/BUILD.bazel index f138dc7c74..350ad6af64 100644 --- a/internal/npm_install/BUILD.bazel +++ b/internal/npm_install/BUILD.bazel @@ -56,5 +56,7 @@ nodejs_binary( "//third_party/npm/node_modules/named-amd", ], entry_point = ":browserify-wrapped.js", + # TODO: figure out why browserify isn't resolved properly + templated_args = ["--bazel_patch_module_resolver"], visibility = ["//visibility:public"], ) diff --git a/internal/npm_install/test/BUILD.bazel b/internal/npm_install/test/BUILD.bazel index 286295f9f9..b6f257cdd4 100644 --- a/internal/npm_install/test/BUILD.bazel +++ b/internal/npm_install/test/BUILD.bazel @@ -18,6 +18,9 @@ jasmine_node_test( "@fine_grained_goldens//:golden_files", "@npm//unidiff", ], + # Depends on having the .js file in source tree but resolve relative paths + # to .js files in the output tree + templated_args = ["--bazel_patch_module_resolver"], ) nodejs_binary( @@ -103,6 +106,8 @@ sh_test( ], node_modules = "@fine_grained_deps_%s//:node_modules" % pkgmgr, tags = ["no-local-jasmine-deps"], + # TODO: get this test running with just linker: failing under --config=no-runfiles + templated_args = ["--bazel_patch_module_resolver"], deps = [ "@fine_grained_deps_%s//jasmine" % pkgmgr, "@fine_grained_deps_%s//jasmine-core" % pkgmgr, @@ -122,6 +127,8 @@ sh_test( "fine.spec.js", ], tags = ["no-local-jasmine-deps"], + # TODO: get this test running with just linker: failing under --config=no-runfiles + templated_args = ["--bazel_patch_module_resolver"], deps = [ "@fine_grained_deps_%s//jasmine" % pkgmgr, "@fine_grained_deps_%s//jasmine-core" % pkgmgr, diff --git a/internal/pkg_npm/BUILD.bazel b/internal/pkg_npm/BUILD.bazel index ea0addbeb3..0c89f44efa 100644 --- a/internal/pkg_npm/BUILD.bazel +++ b/internal/pkg_npm/BUILD.bazel @@ -16,6 +16,8 @@ nodejs_binary( name = "packager", data = ["//third_party/github.com/gjtorikian/isBinaryFile"], entry_point = ":packager.js", + # TODO: figure out why isbinaryfile is not linked in a way this can resolve + templated_args = ["--bazel_patch_module_resolver"], ) nodejs_binary( diff --git a/internal/pkg_web/BUILD.bazel b/internal/pkg_web/BUILD.bazel index b0cfabca5e..96ae3e746c 100644 --- a/internal/pkg_web/BUILD.bazel +++ b/internal/pkg_web/BUILD.bazel @@ -27,12 +27,16 @@ nodejs_binary( ], entry_point = ":assembler.js", node_modules = ":node_modules_none", + # TODO: figure out why isbinaryfile isn't resolved properly + templated_args = ["--bazel_patch_module_resolver"], ) # BEGIN-INTERNAL jasmine_node_test( name = "assembler_test", srcs = ["assembler_spec.js"], + # TODO: figure out why isbinaryfile isn't resolved properly + templated_args = ["--bazel_patch_module_resolver"], deps = [ "assembler.js", "//third_party/github.com/gjtorikian/isBinaryFile", diff --git a/packages/jasmine/test/BUILD.bazel b/packages/jasmine/test/BUILD.bazel index 3799b50059..32651ba43b 100644 --- a/packages/jasmine/test/BUILD.bazel +++ b/packages/jasmine/test/BUILD.bazel @@ -46,6 +46,8 @@ jasmine_node_test( # Use the generated_require.spec.js from the output tree srcs = [":generated_require_spec"], data = ["test.json"], + # TODO: fails under --config=no-runfiles without this flag + templated_args = ["--bazel_patch_module_resolver"], ) copy_to_bin( @@ -118,6 +120,8 @@ jasmine_node_test( "coverage.spec.js", ":coverage_test_srcs", ], + # TODO: fails under --config=no-runfiles without this flag + templated_args = ["--bazel_patch_module_resolver"], ) jasmine_node_test( @@ -127,6 +131,8 @@ jasmine_node_test( "dynamic_import.js", ], args = [ + # TODO: investigate why this fails without the patched require() function + "--bazel_patch_module_resolver", # the --node_options arg will be consumed by the node launcher "--node_options=--experimental-modules", # the remaining args should be passed to the spec @@ -147,6 +153,8 @@ jasmine_node_test( "arg3", ], templated_args = [ + # TODO: investigate why this fails without the patched require() function + "--bazel_patch_module_resolver", # the --node_options templated arg will be consumed by the node launcher "--node_options=--experimental-modules", # the remaining args should be passed to the spec diff --git a/packages/labs/grpc_web/BUILD.bazel b/packages/labs/grpc_web/BUILD.bazel index a182bd1407..5539f11aec 100644 --- a/packages/labs/grpc_web/BUILD.bazel +++ b/packages/labs/grpc_web/BUILD.bazel @@ -15,6 +15,8 @@ nodejs_binary( name = "change_import_style", entry_point = ":change_import_style.js", node_modules = "@build_bazel_rules_typescript_grpc_web_compiletime_deps//:node_modules", + # TODO: figure out why this doesn't resolve minimist + templated_args = ["--bazel_patch_module_resolver"], visibility = ["//visibility:public"], ) diff --git a/packages/labs/test/grpc_web/BUILD.bazel b/packages/labs/test/grpc_web/BUILD.bazel index d69bde0f8d..e683142e3d 100644 --- a/packages/labs/test/grpc_web/BUILD.bazel +++ b/packages/labs/test/grpc_web/BUILD.bazel @@ -10,6 +10,8 @@ jasmine_node_test( "@npm//google-protobuf", "@npm//grpc-web", ], + # TODO: make the test work with generated protobuf code that isn't runfiles-aware + templated_args = ["--bazel_patch_module_resolver"], deps = [ ":commonjs_test_lib", ], diff --git a/packages/node-patches/BUILD.bazel b/packages/node-patches/BUILD.bazel index cb392adbdf..69d188f615 100644 --- a/packages/node-patches/BUILD.bazel +++ b/packages/node-patches/BUILD.bazel @@ -60,7 +60,11 @@ nodejs_test( ], entry_point = "@npm_node_patches//:node_modules/mocha/bin/mocha", tags = ["fix-windows"], - templated_args = ["$$(rlocation $(rootpath %s))" % s for s in test_js] + ["--nobazel_node_patches"], + templated_args = ["$$(rlocation $(rootpath %s))" % s for s in test_js] + [ + # TODO: passes locally on mac without this flag but fails on CircleCI + "--bazel_patch_module_resolver", + "--nobazel_node_patches", + ], ) rollup_bundle( diff --git a/packages/protractor/protractor_web_test.bzl b/packages/protractor/protractor_web_test.bzl index 7639822a1f..5d150fae26 100644 --- a/packages/protractor/protractor_web_test.bzl +++ b/packages/protractor/protractor_web_test.bzl @@ -261,6 +261,8 @@ def protractor_web_test( entry_point = Label(protractor_entry_point), data = srcs + deps + data + [Label(d) for d in peer_deps], testonly = 1, + # TODO: make protractor binary not depend on monkey-patched require() + templated_args = ["--bazel_patch_module_resolver"], visibility = ["//visibility:private"], ) diff --git a/packages/protractor/test/protractor-utils/BUILD.bazel b/packages/protractor/test/protractor-utils/BUILD.bazel index 0677e3e08a..e1a1b04ebc 100644 --- a/packages/protractor/test/protractor-utils/BUILD.bazel +++ b/packages/protractor/test/protractor-utils/BUILD.bazel @@ -28,5 +28,8 @@ jasmine_node_test( srcs = [":protractor_utils_tests_lib"], data = [ ":fake-devserver", + "//packages/protractor", ], + # TODO: fails under --config=no-runfiles without this flag + templated_args = ["--bazel_patch_module_resolver"], ) diff --git a/third_party/github.com/bazelbuild/rules_typescript/internal/BUILD.bazel b/third_party/github.com/bazelbuild/rules_typescript/internal/BUILD.bazel index d98375e42d..fa4b0d1391 100644 --- a/third_party/github.com/bazelbuild/rules_typescript/internal/BUILD.bazel +++ b/third_party/github.com/bazelbuild/rules_typescript/internal/BUILD.bazel @@ -83,6 +83,9 @@ nodejs_binary( ], entry_point = ":tsc_wrapped/tsc_wrapped.js", visibility = ["//visibility:public"], + # With RBE or --worker_sandboxing you'll see that when supports_workers=True it doesn't run_node + # so it doesn't have the linker. Still needs our custom patches on require() + templated_args = ["--bazel_patch_module_resolver"], ) ts_library(