Remove npm/rules_js dependency for C++ only use cases (#7990)

philsc · dbaileychess · web-flow · commit 7106d86685e3 · 2024-04-18T05:06:06.000Z
When flatbuffers is being used from a project that has no use for JavaScript, users encounter an error similar to the following: ERROR: Skipping '@com_github_google_flatbuffers//:flatbuffers': error loading package '@com_github_google_flatbuffers//': Unable to find package for @npm//:defs.bzl: The repository '@npm' could not be resolved: Repository '@npm' is not defined. WARNING: Target pattern parsing failed. ERROR: error loading package '@com_github_google_flatbuffers//': Unable to find package for @npm//:defs.bzl: The repository '@npm' could not be resolved: Repository '@npm' is not defined. INFO: Elapsed time: 0.023s INFO: 0 processes. FAILED: Build did NOT complete successfully (0 packages loaded) currently loading: @com_github_google_flatbuffers// That's not ideal. Users that only care about C++ for example shouldn't be forced to deal with rules_js and friends. This patch attempts to fix that by moving the rules_js-specific things into the `ts` and `tests/ts` directories. This should allow non-JavaScript projects to ignore rules_js and friends completely. Here I basically followed the `rules_foo` example from rules_js: https://github.com/aspect-build/rules_js/tree/main/e2e/rules_foo The idea is that flatbuffers has its own npm dependencies regardless of what other projects may have. This means we should not force the user to import flatbuffers's npm dependencies. The new `ts/repositories.bzl` file is used by dependents to import flatbuffers's dependencies. They can still import their own dependencies. This cleanup allowed me to move all JavaScript-specific stuff from the top-level directory into subdirectories. There should be no changes in this patch in terms of functionality. It's just a refactor of the rules_js call sites. Users will have to add a call to the function in `ts/repositories.bzl` in their own `WORKSPACE` file. They can use `tests/ts/bazel_repository_test/WORKSPACE` as an example. Co-authored-by: Derek Bailey <derekbailey@google.com>
diff --git a/.bazelignore b/.bazelignore
@@ -1 +1 @@
-node_modules
+ts/node_modules
diff --git a/.bazelrc b/.bazelrc
@@ -1,4 +1,4 @@
 # We cannot use "common" here because the "version" command doesn't support
 # --deleted_packages. We need to specify it for both build and query instead.
-build --deleted_packages=tests/ts/bazel_repository_test_dir
-query --deleted_packages=tests/ts/bazel_repository_test_dir
+build --deleted_packages=tests/bazel_repository_test_dir,tests/ts/bazel_repository_test_dir
+query --deleted_packages=tests/bazel_repository_test_dir,tests/ts/bazel_repository_test_dir
diff --git a/BUILD.bazel b/BUILD.bazel
@@ -1,5 +1,3 @@
-load("@aspect_rules_js//npm:defs.bzl", "npm_link_package")
-load("@npm//:defs.bzl", "npm_link_all_packages")
 load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_library")
 
 licenses(["notice"])
@@ -8,13 +6,6 @@ package(
     default_visibility = ["//visibility:public"],
 )
 
-npm_link_all_packages(name = "node_modules")
-
-npm_link_package(
-    name = "node_modules/flatbuffers",
-    src = "//ts:flatbuffers",
-)
-
 exports_files([
     "LICENSE",
     "tsconfig.json",
@@ -37,9 +28,13 @@ config_setting(
 filegroup(
     name = "distribution",
     srcs = [
+        ".bazelignore",
+        ".npmrc",
         "BUILD.bazel",
         "WORKSPACE",
         "build_defs.bzl",
+        "package.json",
+        "pnpm-lock.yaml",
         "typescript.bzl",
         "//grpc/src/compiler:distribution",
         "//reflection:distribution",
diff --git a/WORKSPACE b/WORKSPACE
@@ -11,6 +11,21 @@ http_archive(
     ],
 )
 
+# Import our own version of skylib before other rule sets (e.g. rules_swift)
+# has a chance to import an old version.
+http_archive(
+    name = "bazel_skylib",
+    sha256 = "66ffd9315665bfaafc96b52278f57c7e2dd09f5ede279ea6d39b2be471e7e3aa",
+    urls = [
+        "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/1.4.2/bazel-skylib-1.4.2.tar.gz",
+        "https://github.com/bazelbuild/bazel-skylib/releases/download/1.4.2/bazel-skylib-1.4.2.tar.gz",
+    ],
+)
+
+load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace")
+
+bazel_skylib_workspace()
+
 http_archive(
     name = "build_bazel_rules_apple",
     sha256 = "34c41bfb59cdaea29ac2df5a2fa79e5add609c71bb303b2ebb10985f93fa20e7",
@@ -101,7 +116,7 @@ load("@aspect_rules_js//js:repositories.bzl", "rules_js_dependencies")
 
 rules_js_dependencies()
 
-load("@aspect_rules_js//npm:npm_import.bzl", "npm_translate_lock", "pnpm_repository")
+load("@aspect_rules_js//npm:npm_import.bzl", "pnpm_repository")
 
 pnpm_repository(name = "pnpm")
 
@@ -129,17 +144,13 @@ nodejs_register_toolchains(
     node_version = DEFAULT_NODE_VERSION,
 )
 
-npm_translate_lock(
-    name = "npm",
-    npmrc = "//:.npmrc",
-    pnpm_lock = "//:pnpm-lock.yaml",
-    # Set this to True when the lock file needs to be updated, commit the
-    # changes, then set to False again.
-    update_pnpm_lock = False,
-    verify_node_modules_ignored = "//:.bazelignore",
+load("@com_github_google_flatbuffers//ts:repositories.bzl", "flatbuffers_npm")
+
+flatbuffers_npm(
+    name = "flatbuffers_npm",
 )
 
-load("@npm//:repositories.bzl", "npm_repositories")
+load("@flatbuffers_npm//:repositories.bzl", "npm_repositories")
 
 npm_repositories()
 
diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel
@@ -1,9 +1,14 @@
 load("@aspect_bazel_lib//lib:copy_to_bin.bzl", "copy_to_bin")
 load("@rules_cc//cc:defs.bzl", "cc_test")
 load("//:build_defs.bzl", "DEFAULT_FLATC_ARGS", "flatbuffer_cc_library")
+load(":defs.bzl", "flatbuffers_as_external_repo_test")
 
 package(default_visibility = ["//visibility:private"])
 
+exports_files([
+    "bazel_repository_test_template.sh",
+])
+
 # rules_js works around various JS tooling limitations by copying everything
 # into the output directory. Make the test data available to the tests this way.
 copy_to_bin(
@@ -264,3 +269,8 @@ flatbuffer_cc_library(
     name = "alignment_test_cc_fbs",
     srcs = ["alignment_test.fbs"],
 )
+
+flatbuffers_as_external_repo_test(
+    name = "bazel_repository_test",
+    directory = "bazel_repository_test_dir",
+)
diff --git a/tests/bazel_repository_test_dir/.bazelrc b/tests/bazel_repository_test_dir/.bazelrc
@@ -0,0 +1 @@
+build --symlink_prefix=/
diff --git a/tests/bazel_repository_test_dir/.gitignore b/tests/bazel_repository_test_dir/.gitignore
@@ -0,0 +1 @@
+bazel-*
diff --git a/tests/bazel_repository_test_dir/BUILD b/tests/bazel_repository_test_dir/BUILD
@@ -0,0 +1,10 @@
+# This test doesn't actually make use of the flatbuffers library. It's just
+# here to make sure we can link the library properly when it comes from an
+# external repository.  You're welcome to expand this test to do more.
+cc_test(
+    name = "pulls_in_flatbuffers_test",
+    srcs = ["pulls_in_flatbuffers_test.cpp"],
+    deps = [
+        "@com_github_google_flatbuffers//:flatbuffers",
+    ],
+)
diff --git a/tests/bazel_repository_test_dir/README.md b/tests/bazel_repository_test_dir/README.md
@@ -0,0 +1,8 @@
+This directory is not intended to be used independently of the flatbuffers
+repository. Instead, this whole directory serves as a unit test for the
+C++ integration in the flatbuffers repo.
+
+Run this test from the top-level of the flatbuffers repo.
+```console
+$ bazel test //tests:bazel_repository_test
+```
diff --git a/tests/bazel_repository_test_dir/WORKSPACE b/tests/bazel_repository_test_dir/WORKSPACE
@@ -0,0 +1,6 @@
+workspace(name = "bazel_repository_test")
+
+local_repository(
+    name = "com_github_google_flatbuffers",
+    path = "../../",
+)
diff --git a/tests/bazel_repository_test_dir/pulls_in_flatbuffers_test.cpp b/tests/bazel_repository_test_dir/pulls_in_flatbuffers_test.cpp
@@ -0,0 +1 @@
+int main() { return 0; }
diff --git a/tests/bazel_repository_test_template.sh b/tests/bazel_repository_test_template.sh
@@ -1,7 +1,7 @@
 #!/bin/bash
 
 # This test makes sure that a separate repository can import the flatbuffers
-# repository and use it in their JavaScript code.
+# repository and use it in their code.
 
 # --- begin runfiles.bash initialization v3 ---
 # Copy-pasted from the Bazel Bash runfiles library v3.
@@ -24,6 +24,6 @@ fi
 
 export PATH="$(dirname "${BAZEL_BIN}"):${PATH}"
 
-cd tests/ts/bazel_repository_test_dir/
+cd {{REPOSITORY_DIR}}
 
 bazel test //...
diff --git a/tests/defs.bzl b/tests/defs.bzl
@@ -0,0 +1,48 @@
+"""Helper macros and rules for tests."""
+
+load("@bazel_skylib//lib:paths.bzl", "paths")
+load("@bazel_skylib//rules:expand_template.bzl", "expand_template")
+
+def flatbuffers_as_external_repo_test(name, directory):
+    """Run all tests in a bazel workspace that imports flatbuffers as an external repository.
+
+    Args:
+        name: The name of the test target.
+        directory: The directory in which the bazel workspace is located. This is the directory
+            that imports flatbuffers as an external repository.
+    """
+    expand_template(
+        name = name + "__template_expansion",
+        out = name + ".sh",
+        substitutions = {
+            "{{REPOSITORY_DIR}}": paths.join(native.package_name(), directory),
+        },
+        template = "//tests:bazel_repository_test_template.sh",
+    )
+
+    native.sh_test(
+        name = name,
+        srcs = [":%s.sh" % name],
+        data = [
+            "//:distribution",
+            "@bazel_linux_x86_64//file",
+        ] + native.glob(
+            [
+                directory + "/**/*",
+            ],
+            exclude = [
+                directory + "/bazel-*/**",
+            ],
+        ),
+        tags = [
+            # Since we have bazel downloading external repositories inside this
+            # test, we need to give it access to the internet.
+            "requires-network",
+        ],
+        # We only have x86_64 Linux bazel exposed so restrict the test to that.
+        target_compatible_with = [
+            "@platforms//cpu:x86_64",
+            "@platforms//os:linux",
+        ],
+        deps = ["@bazel_tools//tools/bash/runfiles"],
+    )
diff --git a/tests/ts/BUILD.bazel b/tests/ts/BUILD.bazel
@@ -1,8 +1,19 @@
 load("@aspect_rules_js//js:defs.bzl", "js_test")
+load("@aspect_rules_js//npm:defs.bzl", "npm_link_package")
 load("//:typescript.bzl", "flatbuffer_ts_library")
+load("//tests:defs.bzl", "flatbuffers_as_external_repo_test")
 
 package(default_visibility = ["//visibility:private"])
 
+# This is a copy of //ts:node_modules/flatbuffers. The rules_js-based tests
+# require this target to live in the same or a parent package. Since we don't
+# want to put rules_js targets in the root package, we create a copy here.
+npm_link_package(
+    name = "node_modules/flatbuffers",
+    src = "//ts:flatbuffers",
+    root_package = "tests/ts",
+)
+
 flatbuffer_ts_library(
     name = "typescript_ts_fbs",
     srcs = ["typescript_keywords.fbs"],
@@ -37,8 +48,8 @@ TEST_COMPLEX_ARRAYS_DATA = glob([
     chdir = package_name(),
     data = data + [
         "package.json",
-        "//:node_modules/flatbuffers",
         "//tests:test_data_copied_to_bin",
+        "//tests/ts:node_modules/flatbuffers",
     ],
     entry_point = "%s.js" % test,
 ) for test, data in (
@@ -50,29 +61,7 @@ TEST_COMPLEX_ARRAYS_DATA = glob([
     ("JavaScriptComplexArraysTest", TEST_COMPLEX_ARRAYS_DATA),
 )]
 
-sh_test(
+flatbuffers_as_external_repo_test(
     name = "bazel_repository_test",
-    srcs = ["bazel_repository_test.sh"],
-    data = [
-        "//:distribution",
-        "@bazel_linux_x86_64//file",
-    ] + glob(
-        [
-            "bazel_repository_test_dir/**/*",
-        ],
-        exclude = [
-            "bazel_repository_test_dir/bazel-*/**",
-        ],
-    ),
-    tags = [
-        # Since we have bazel downloading external repositories inside this
-        # test, we need to give it access to the internet.
-        "requires-network",
-    ],
-    # We only have x86_64 Linux bazel exposed so restrict the test to that.
-    target_compatible_with = [
-        "@platforms//cpu:x86_64",
-        "@platforms//os:linux",
-    ],
-    deps = ["@bazel_tools//tools/bash/runfiles"],
+    directory = "bazel_repository_test_dir",
 )
diff --git a/tests/ts/bazel_repository_test_dir/BUILD.bazel b/tests/ts/bazel_repository_test_dir/BUILD.bazel
@@ -30,3 +30,12 @@ js_test(
     ],
     entry_point = "import_test.js",
 )
+
+js_test(
+    name = "independent_deps_test",
+    data = [
+        "package.json",
+        ":node_modules/lodash",
+    ],
+    entry_point = "independent_deps_test.js",
+)
diff --git a/tests/ts/bazel_repository_test_dir/README.md b/tests/ts/bazel_repository_test_dir/README.md
@@ -0,0 +1,8 @@
+This directory is not intended to be used independently of the flatbuffers
+repository. Instead, this whole directory serves as a unit test for the
+`rules_js` integration in the flatbuffers repo.
+
+Run this test from the top-level of the flatbuffers repo.
+```console
+$ bazel test //tests/ts:bazel_repository_test
+```
diff --git a/tests/ts/bazel_repository_test_dir/WORKSPACE b/tests/ts/bazel_repository_test_dir/WORKSPACE
@@ -46,8 +46,12 @@ nodejs_register_toolchains(
 
 npm_translate_lock(
     name = "npm",
+    data = [
+        "//:package.json",
+    ],
     npmrc = "//:.npmrc",
     pnpm_lock = "//:pnpm-lock.yaml",
+    update_pnpm_lock = False,
     verify_node_modules_ignored = "//:.bazelignore",
 )
 
@@ -69,3 +73,13 @@ esbuild_register_toolchains(
     name = "esbuild",
     esbuild_version = LATEST_VERSION,
 )
+
+load("@com_github_google_flatbuffers//ts:repositories.bzl", "flatbuffers_npm")
+
+flatbuffers_npm(
+    name = "flatbuffers_npm",
+)
+
+load("@flatbuffers_npm//:repositories.bzl", flatbuffers_npm_repositories = "npm_repositories")
+
+flatbuffers_npm_repositories()
diff --git a/tests/ts/bazel_repository_test_dir/independent_deps_test.js b/tests/ts/bazel_repository_test_dir/independent_deps_test.js
@@ -0,0 +1,18 @@
+// This test has nothing to do with flatbuffers. It only exists to validate
+// that other projects can use their own set of dependencies without having to
+// explicitly pull in flatbuffers's dependencies.
+//
+// We pick lodash here not for any particular reason. It could be any package,
+// really. I chose it because it's a relatively simple package.
+
+import assert from 'node:assert/strict'
+
+import _ from 'lodash'
+
+function main() {
+  console.log(_);
+  assert.deepStrictEqual(_.defaults({ 'a': 1 }, { 'a': 3, 'b': 2 }), { 'a': 1, 'b': 2 });
+  assert.deepStrictEqual(_.partition([1, 2, 3, 4], n => n % 2), [[1, 3], [2, 4]]);
+}
+
+main();
diff --git a/tests/ts/bazel_repository_test_dir/package.json b/tests/ts/bazel_repository_test_dir/package.json
@@ -3,6 +3,6 @@
   "type": "module",
   "private": true,
   "devDependencies": {
-    "@types/node": "18.15.11"
+    "lodash": "4.17.21"
   }
 }
diff --git a/tests/ts/bazel_repository_test_dir/pnpm-lock.yaml b/tests/ts/bazel_repository_test_dir/pnpm-lock.yaml
diff --git a/tests/ts/test_dir/BUILD.bazel b/tests/ts/test_dir/BUILD.bazel
@@ -20,7 +20,7 @@ js_test(
     data = [
         "package.json",
         ":include_ts_fbs",
-        "//:node_modules/flatbuffers",
+        "//tests/ts:node_modules/flatbuffers",
     ],
     entry_point = "import_test.js",
 )
diff --git a/ts/BUILD.bazel b/ts/BUILD.bazel
diff --git a/ts/repositories.bzl b/ts/repositories.bzl
