Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from rules_nodejs to rules_js/rules_ts (take 2) #7928

Merged

Conversation

philsc
Copy link
Contributor

@philsc philsc commented Apr 29, 2023

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.
  5. 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.
  6. 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

philsc added 2 commits April 28, 2023 21:27
This is the second version of patch google#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 google#7817
@github-actions github-actions bot added codegen Involving generating code from schema grpc javascript json labels Apr 29, 2023
@philsc
Copy link
Contributor Author

philsc commented Apr 29, 2023

The buildkite query for the tests needs to be adjusted to something like this:

$ bazel --nosystem_rc --nohome_rc cquery --output=starlark --starlark:expr='target.label if "IncompatiblePlatformProvider" not in providers(target) else ""' "tests(set('//tests/...')) except tests(attr("tags", "manual", set('//tests/...')))"
@//tests/ts:JavaScriptUnionVectorTest_test
@//tests/ts/test_dir:import_test
@//tests:flatbuffers_test
@//tests/ts:bazel_repository_test
@//tests/ts:JavaScriptTest_test
@//tests/ts:JavaScriptComplexArraysTest_test

That way, the @//tests/ts:bazel_repository_test gets filtered out on MacOs.

@dbaileychess
Copy link
Collaborator

The buildkite query for the tests needs to be adjusted to something like this:

$ bazel --nosystem_rc --nohome_rc cquery --output=starlark --starlark:expr='target.label if "IncompatiblePlatformProvider" not in providers(target) else ""' "tests(set('//tests/...')) except tests(attr("tags", "manual", set('//tests/...')))"
@//tests/ts:JavaScriptUnionVectorTest_test
@//tests/ts/test_dir:import_test
@//tests:flatbuffers_test
@//tests/ts:bazel_repository_test
@//tests/ts:JavaScriptTest_test
@//tests/ts:JavaScriptComplexArraysTest_test

That way, the @//tests/ts:bazel_repository_test gets filtered out on MacOs.

Not immediately sure where to add this.

The buildkite uses this python file: https://raw.githubusercontent.com/bazelbuild/continuous-integration/master/buildkite/bazelci.py

which references our presubmit file: https://raw.githubusercontent.com/google/flatbuffers/master/.bazelci/presubmit.yml

So perhaps you can just add the filters there as part of this PR?

@philsc
Copy link
Contributor Author

philsc commented May 1, 2023

Oh! I think that's it, thank you!
I'd grepped around for bazel query, came up empty, and assumed that it was part of the buildkite configuration instead.

I will fix this. Sorry for the noise.

@philsc
Copy link
Contributor Author

philsc commented May 3, 2023

Created bazelbuild/continuous-integration#1618 to hopefully address the CI issue.

@meteorcloudy
Copy link
Contributor

Can you rerun the presubmit again, we recently had some infrastructure change that broke the test filtering, should be fixed now.

@meteorcloudy
Copy link
Contributor

Context is bazelbuild/continuous-integration#1614

@philsc
Copy link
Contributor Author

philsc commented May 3, 2023

I merged with master which should trigger the presubmit again. Thanks for the heads up, @meteorcloudy !

@philsc
Copy link
Contributor Author

philsc commented May 3, 2023

Looks like that worked, thanks!

@dbaileychess dbaileychess merged commit c1e7aee into google:master May 3, 2023
@dbaileychess
Copy link
Collaborator

Thanks!

jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
* Migrate from rules_nodejs to rules_js/rules_ts (take 2)

This is the second version of patch google#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 google#7817

* Fix the query

---------

Co-authored-by: Derek Bailey <[email protected]>
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
* Migrate from rules_nodejs to rules_js/rules_ts (take 2)

This is the second version of patch google#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 google#7817

* Fix the query

---------

Co-authored-by: Derek Bailey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen Involving generating code from schema grpc javascript json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TS] Upgrade/replace @bazel/typescript
3 participants