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

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

Merged
merged 11 commits into from
Apr 18, 2024

Conversation

philsc
Copy link
Contributor

@philsc philsc commented Jun 5, 2023

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.

I added a new test in tests/bazel_repository_test to validate
that we can import the flatbuffers repository without having to
deal with rules_js and friends. It can be expanded to make
further assertions about the behaviour of C++ code pulling in
the flatbuffers repository.

Fixes #7988

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.
@philsc philsc force-pushed the unreviewed/phil/fix-7988-b branch from 4bc4a02 to f6c9671 Compare June 18, 2023 22:18
Copy link
Contributor

@jkuszmaul jkuszmaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me; any chance we could get it in @dbaileychess?

@dbaileychess dbaileychess enabled auto-merge (squash) April 18, 2024 05:02
@dbaileychess dbaileychess merged commit 7106d86 into google:master Apr 18, 2024
45 checks passed
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
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 <[email protected]>
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bazel build rules require npm
3 participants