Skip to content

Conversation

kormide
Copy link
Contributor

@kormide kormide commented Nov 30, 2021

Before re-writing the build.ts script to use bazel for building, I thought a good place to start would be to add some missing rules. Three of the packages built by build.ts didn't have corresponding pkg_npm targets.

@google-cla google-cla bot added cla: no and removed cla: no labels Nov 30, 2021
@josephperrott josephperrott added area: build & ci Related the build and CI infrastructure of the project target: patch This PR is targeted for the next patch release labels Nov 30, 2021
# @external_begin
load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar")
load("@build_bazel_rules_nodejs//:index.bzl", "pkg_npm")
# @external_end
Copy link
Contributor Author

@kormide kormide Nov 30, 2021

Choose a reason for hiding this comment

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

I added these external tags to follow the existing convention, but I'm not sure what they are used for.

Copy link
Member

Choose a reason for hiding this comment

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

I was going to ask. It doesn't seem like there is any value in having these as the CLI build files are not exposed publicly anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These comments were added by @kyliau and were needed for a G3 sync.

Copy link
Member

Choose a reason for hiding this comment

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

interesting. we should probably re-evaluate (or if @josephperrott already did), whether this is still something we want to maintain long-term. I suspect it will complicate the Bazel migration/maintaince.

Copy link
Member

Choose a reason for hiding this comment

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

This is something we do not need to include as we do not use our BUILD files internally in g3, we have google3 specific ones.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Drive by comment: I'm hoping to reevaluate how we integrate the CLI internally to improve the syncing story so we can focus just on the external side here.

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM, though we can likely remove the @external... blocks.

@kormide
Copy link
Contributor Author

kormide commented Nov 30, 2021

LGTM, though we can likely remove the @external... blocks.

I'll remove them.

@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Nov 30, 2021
@kormide
Copy link
Contributor Author

kormide commented Nov 30, 2021

I removed the @external tags. I can remove the ones for existing pkg_npm rules in futures PRs as I work through the bazel migration.

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM, but one comment.

@@ -6,6 +6,8 @@
load("@npm//@bazel/jasmine:index.bzl", "jasmine_node_test")
load("//tools:defaults.bzl", "ts_library")
load("//tools:ts_json_schema.bzl", "ts_json_schema")
load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar")
Copy link
Member

Choose a reason for hiding this comment

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

FYI: the pkg_tar rule from bazel_tools is deprecated and does not work on Windows. https://docs.bazel.build/versions/main/be/pkg.html

Not necessarily blocking for this PR as it probably is used elsewhere in the repo, but just a heads up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I'll look into changing those rules before the bazel built packages actually get used.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants