Skip to content

Commit

Permalink
Migrate from rules_nodejs to rules_js/rules_ts (take 2) (#7928)
Browse files Browse the repository at this point in the history
* Migrate from rules_nodejs to rules_js/rules_ts (take 2)

This is the second version of patch #7923. The first version got
reverted because bazel query was failing:

    $ bazel --nosystem_rc --nohome_rc query tests(set('//...')) except tests(attr("tags", "manual", set('//...')))
    ERROR: Traceback (most recent call last):
    	File "/workdir/tests/ts/bazel_repository_test_dir/BUILD", line 6, column 22, in <toplevel>
    		npm_link_all_packages(name = "node_modules")
    	File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/external/npm/defs.bzl", line 188, column 13, in npm_link_all_packages
    		fail(msg)
    Error in fail: The npm_link_all_packages() macro loaded from @npm//:defs.bzl and called in bazel package 'tests/ts/bazel_repository_test_dir' may only be called in bazel packages that correspond to the pnpm root package '' and pnpm workspace projects ''

This was happening because the `.bazelrc` file only added
`--deleted_packages` to the `build` command. We also need it for the
`query` command. This second version of the patch fixes that.

Original commit message:

This patch migrates the current use of rules_nodejs to the new rules_js.
rules_js is the intended replacement of rules_nodejs as per this note:
https://github.com/aspect-build/rules_js#relationship-to-rules_nodejs

> rules_js is an alternative to the build_bazel_rules_nodejs Bazel module
> and accompanying npm packages hosted in
> https://github.com/bazelbuild/rules_nodejs, which is now
> unmaintained. All users are recommended to use rules_js instead.

There are a few notable changes in this patch:
1. The `flatbuffer_ts_library` macro no longer accepts a `package_name`
   attribute. This is because rules_js appears to manage the import
   naming of dependencies via top-level `npm_link_package` targets.
   Users will have to migrate.
2. I added a few more arguments to `flatbuffer_library_public()`. These
   helped with exposing esbuild to `ts/compile_flat_file.sh`.
3. I pinned the version of `typescript` in `package.json` so that
   rules_ts can download the exact same version. rules_ts doesn't know
   what to do if the version isn't exact.
4. Since rules_js uses the pnpm locking mechanism, we now have a
   `pnpm-lock.yaml` file instead of a yarn lock file.
4. I added bazel targets for a few of the existing tests in `tests/ts`.
   They can be run with `bazel test //test/ts:all`. Since there is no
   flexbuffers bazel target, I did not add a bazel target for the
   corresponding test.
5. I added a separate workspace in `tests/ts/bazel_repository_test_dir/`
   to validate that the flatbuffers code can be imported as an external
   repository. You can run the test with
   `bazel test //test/ts:bazel_repository_test`. For this to work, I
   needed to expose a non-trivial chunk of the flatbuffers code to the
   test. I achieved this through some recursive `distribution`
   filegroups. This is inspired by rules_python's workspace tests.

I did not do anything special to validate that the `gen_reflections`
parameter works the same. This patch doesn't change anything about
the TypeScript generation.

As a side note: I am not an expert with rules_js. This patch is my
attempt based on my limited understanding of the rule set.

Fixes #7817

* Fix the query

---------

Co-authored-by: Derek Bailey <[email protected]>
  • Loading branch information
philsc and dbaileychess authored May 3, 2023
1 parent 75143f8 commit c1e7aee
Show file tree
Hide file tree
Showing 35 changed files with 1,700 additions and 1,214 deletions.
1 change: 1 addition & 0 deletions .bazelignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
node_modules
4 changes: 4 additions & 0 deletions .bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +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
1 change: 1 addition & 0 deletions .npmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hoist=false
26 changes: 26 additions & 0 deletions BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
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"])
Expand All @@ -6,6 +8,13 @@ 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",
Expand All @@ -25,6 +34,23 @@ config_setting(
],
)

filegroup(
name = "distribution",
srcs = [
"BUILD.bazel",
"WORKSPACE",
"build_defs.bzl",
"typescript.bzl",
"//grpc/src/compiler:distribution",
"//reflection:distribution",
"//src:distribution",
"//ts:distribution",
] + glob([
"include/flatbuffers/*.h",
]),
visibility = ["//visibility:public"],
)

# Public flatc library to compile flatbuffer files at runtime.
cc_library(
name = "flatbuffers",
Expand Down
84 changes: 67 additions & 17 deletions WORKSPACE
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
workspace(name = "com_github_google_flatbuffers")

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive", "http_file")

http_archive(
name = "platforms",
Expand Down Expand Up @@ -76,30 +76,80 @@ load("@com_github_grpc_grpc//bazel:grpc_extra_deps.bzl", "grpc_extra_deps")
grpc_extra_deps()

# rules_go from https://github.com/bazelbuild/rules_go/releases/tag/v0.34.0

http_archive(
name = "aspect_rules_js",
sha256 = "124ed29fb0b3d0cba5b44f8f8e07897cf61b34e35e33b1f83d1a943dfd91b193",
strip_prefix = "rules_js-1.24.0",
url = "https://github.com/aspect-build/rules_js/releases/download/v1.24.0/rules_js-v1.24.0.tar.gz",
)

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")

pnpm_repository(name = "pnpm")

http_archive(
name = "build_bazel_rules_nodejs",
sha256 = "965ee2492a2b087cf9e0f2ca472aeaf1be2eb650e0cfbddf514b9a7d3ea4b02a",
urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/5.2.0/rules_nodejs-5.2.0.tar.gz"],
name = "aspect_rules_ts",
sha256 = "8eb25d1fdafc0836f5778d33fb8eaac37c64176481d67872b54b0a05de5be5c0",
strip_prefix = "rules_ts-1.3.3",
url = "https://github.com/aspect-build/rules_ts/releases/download/v1.3.3/rules_ts-v1.3.3.tar.gz",
)

load("@build_bazel_rules_nodejs//:repositories.bzl", "build_bazel_rules_nodejs_dependencies")
load("@aspect_rules_ts//ts:repositories.bzl", "rules_ts_dependencies")

build_bazel_rules_nodejs_dependencies()
rules_ts_dependencies(
# Since rules_ts doesn't always have the newest integrity hashes, we
# compute it manually here.
# $ curl --silent https://registry.npmjs.org/typescript/5.0.4 | jq ._integrity
ts_integrity = "sha512-cW9T5W9xY37cc+jfEnaUvX91foxtHkza3Nw3wkoF4sSlKn0MONdkdEndig/qPBWXNkmplh3NzayQzCiHM4/hqw==",
ts_version_from = "//:package.json",
)

load("@build_bazel_rules_nodejs//:index.bzl", "node_repositories", "yarn_install")
load("@rules_nodejs//nodejs:repositories.bzl", "DEFAULT_NODE_VERSION", "nodejs_register_toolchains")

node_repositories()
nodejs_register_toolchains(
name = "nodejs",
node_version = DEFAULT_NODE_VERSION,
)

yarn_install(
npm_translate_lock(
name = "npm",
exports_directories_only = False,
# Unfreeze to add/remove packages.
frozen_lockfile = False,
package_json = "//:package.json",
symlink_node_modules = False,
yarn_lock = "//:yarn.lock",
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("@build_bazel_rules_nodejs//toolchains/esbuild:esbuild_repositories.bzl", "esbuild_repositories")
load("@npm//:repositories.bzl", "npm_repositories")

esbuild_repositories(npm_repository = "npm")
npm_repositories()

http_archive(
name = "aspect_rules_esbuild",
sha256 = "2ea31bd97181a315e048be693ddc2815fddda0f3a12ca7b7cc6e91e80f31bac7",
strip_prefix = "rules_esbuild-0.14.4",
url = "https://github.com/aspect-build/rules_esbuild/releases/download/v0.14.4/rules_esbuild-v0.14.4.tar.gz",
)

# Register a toolchain containing esbuild npm package and native bindings
load("@aspect_rules_esbuild//esbuild:repositories.bzl", "LATEST_VERSION", "esbuild_register_toolchains")

esbuild_register_toolchains(
name = "esbuild",
esbuild_version = LATEST_VERSION,
)

http_file(
name = "bazel_linux_x86_64",
downloaded_file_path = "bazel",
sha256 = "e89747d63443e225b140d7d37ded952dacea73aaed896bca01ccd745827c6289",
urls = [
"https://github.com/bazelbuild/bazel/releases/download/6.1.2/bazel-6.1.2-linux-x86_64",
],
)
17 changes: 14 additions & 3 deletions build_defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ def flatbuffer_library_public(
restricted_to = None,
target_compatible_with = None,
flatc_path = "@com_github_google_flatbuffers//:flatc",
output_to_bindir = False):
output_to_bindir = False,
tools = None,
extra_env = None,
**kwargs):
"""Generates code files for reading/writing the given flatbuffers in the requested language using the public compiler.
Args:
Expand All @@ -73,6 +76,11 @@ def flatbuffer_library_public(
to use.
flatc_path: Bazel target corresponding to the flatc compiler to use.
output_to_bindir: Passed to genrule for output to bin directory.
tools: Optional, passed to genrule for list of tools to make available
during the action.
extra_env: Optional, must be a string of "VAR1=VAL1 VAR2=VAL2". These get
set as environment variables that "flatc_path" sees.
**kwargs: Passed to the underlying genrule.
This rule creates a filegroup(name) with all generated source files, and
Expand All @@ -83,6 +91,8 @@ def flatbuffer_library_public(
include_paths = default_include_paths(flatc_path)
include_paths_cmd = ["-I %s" % (s) for s in include_paths]

extra_env = extra_env or ""

# '$(@D)' when given a single source target will give the appropriate
# directory. Appending 'out_prefix' is only necessary when given a build
# target with multiple sources.
Expand All @@ -92,7 +102,7 @@ def flatbuffer_library_public(
genrule_cmd = " ".join([
"SRCS=($(SRCS));",
"for f in $${SRCS[@]:0:%s}; do" % len(srcs),
"OUTPUT_FILE=\"$(OUTS)\" $(location %s)" % (flatc_path),
"OUTPUT_FILE=\"$(OUTS)\" %s $(location %s)" % (extra_env, flatc_path),
" ".join(include_paths_cmd),
" ".join(flatc_args),
language_flag,
Expand All @@ -105,12 +115,13 @@ def flatbuffer_library_public(
srcs = srcs + includes,
outs = outs,
output_to_bindir = output_to_bindir,
tools = [flatc_path],
tools = (tools or []) + [flatc_path],
cmd = genrule_cmd,
compatible_with = compatible_with,
target_compatible_with = target_compatible_with,
restricted_to = restricted_to,
message = "Generating flatbuffer files for %s:" % (name),
**kwargs
)
if reflection_name:
reflection_genrule_cmd = " ".join([
Expand Down
10 changes: 10 additions & 0 deletions grpc/src/compiler/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,16 @@ package(
default_visibility = ["//visibility:public"],
)

filegroup(
name = "distribution",
srcs = [
"BUILD.bazel",
] + glob([
"*.cc",
"*.h",
]),
)

filegroup(
name = "common_headers",
srcs = [
Expand Down
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,11 @@
"homepage": "https://google.github.io/flatbuffers/",
"dependencies": {},
"devDependencies": {
"@bazel/typescript": "5.2.0",
"@types/node": "18.15.11",
"@typescript-eslint/eslint-plugin": "^5.57.0",
"@typescript-eslint/parser": "^5.57.0",
"esbuild": "^0.17.14",
"eslint": "^8.37.0",
"typescript": "^5.0.3"
"typescript": "5.0.4"
}
}
Loading

0 comments on commit c1e7aee

Please sign in to comment.