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

fix(ts_proto_library): require explicit srcs to copy #617

Merged
merged 5 commits into from
Jun 3, 2024
Merged

Conversation

alexeagle
Copy link
Member

The hack of using a glob over the srcs causes a collision when there is more than one ts_proto_library in a bazel package.
The resulting error doesn't give a clue what fix is required.


Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): yes (note, ts_proto_library isn't a stable API yet)
  • Suggested release notes appear below: yes

When ts_proto_library#copy_files is True, users must now repeat the .proto files whose outputs should be copied.

Test plan

  • New test cases added

egormodin and others added 4 commits May 31, 2024 15:13
…D file

error: `Error in directory_path: directory_path rule '_logger_pb.d.ts_dirpath' in package 'examples/proto_grpc' conflicts with existing directory_path rule`
The hack of using a glob over the srcs causes a collision when there is more than one ts_proto_library in a bazel package.
The resulting error doesn't give a clue what fix is required.
@alexeagle alexeagle requested a review from mrmeku June 2, 2024 17:41
@CLAassistant
Copy link

CLAassistant commented Jun 2, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

aspect-workflows bot commented Jun 2, 2024

Test

4 test targets passed

Targets
//docs:update_1_test [k8-fastbuild]                              35ms
//examples/proto_grpc:main [k8-fastbuild]                        202ms
//examples/proto_grpc:status_ts_proto.copy_0_test [k8-fastbuild] 49ms
//examples/proto_grpc:status_ts_proto.copy_1_test [k8-fastbuild] 43ms

Total test execution time was 329ms. 75 tests (94.9%) were fully cached saving 10s.


Buildifier      Format

Copy link
Contributor

@mrmeku mrmeku left a comment

Choose a reason for hiding this comment

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

It’s a shame we are in macro land and can’t reference the ProtoInfo provider. Maybe in the future we can convert the rule to use the implementation more directly

https://docs.bazel.build/versions/5.3.1/skylark/lib/ProtoInfo.html#direct_sources

@alexeagle alexeagle merged commit e120763 into main Jun 3, 2024
23 checks passed
@alexeagle alexeagle deleted the ts_multi_proto branch June 3, 2024 17:07
alexeagle added a commit that referenced this pull request Jun 3, 2024
* Example on bazel failing when 2 ts_proto_library are in a single BUILD file

error: `Error in directory_path: directory_path rule '_logger_pb.d.ts_dirpath' in package 'examples/proto_grpc' conflicts with existing directory_path rule`

* fix: explicit files_to_copy for multiple ts_proto_lib

* fix(ts_proto_library): require explicit srcs to copy

The hack of using a glob over the srcs causes a collision when there is more than one ts_proto_library in a bazel package.
The resulting error doesn't give a clue what fix is required.

* chore: docgen

* fix: docs

---------

Co-authored-by: egorm <[email protected]>
alexeagle added a commit that referenced this pull request Jun 3, 2024
* Example on bazel failing when 2 ts_proto_library are in a single BUILD file

error: `Error in directory_path: directory_path rule '_logger_pb.d.ts_dirpath' in package 'examples/proto_grpc' conflicts with existing directory_path rule`

* fix: explicit files_to_copy for multiple ts_proto_lib

* fix(ts_proto_library): require explicit srcs to copy

The hack of using a glob over the srcs causes a collision when there is more than one ts_proto_library in a bazel package.
The resulting error doesn't give a clue what fix is required.

* chore: docgen

* fix: docs

---------

Co-authored-by: egorm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants