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

Error on build ts_proto_library from external proto #622

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

egormodin
Copy link
Contributor

Error:
Error in directory_path: illegal rule name: _@com_google_protobuf//:timestamp_pb.d.ts_dirpath: invalid target name '_@com_google_protobuf//:timestamp_pb.d.ts_dirpath': target names may not contain '//' path separators

Second issue: proto file is not usually accessible, but the d.ts files is needed to be copied, so there is no valid way to build it into source tree.

I suppose the d.ts file should be created from external proto and copied into source tree. Since there is no access to the proto file then the rule should be able to use only proto_library. By adding dependency into ts_proto_library I am trying to ask compiler to use same ts file as I am able to use in the code.

egormodin added 6 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`
# Conflicts:
#	examples/proto_grpc/BUILD.bazel
#	ts/proto.bzl
Error:
`Error in directory_path: illegal rule name: _@com_google_protobuf//:timestamp_pb.d.ts_dirpath: invalid target name '_@com_google_protobuf//:timestamp_pb.d.ts_dirpath': target names may not contain '//' path separators`

Second issue: `proto` file is not usually accessible, but the `d.ts` files is needed to be copied, so there is no valid way to build it into source tree.

I suppose the d.ts file should be created from external proto and copied into source tree. Since there is no access to the `proto` file then the rule should be able to use only `proto_library`. By adding dependency into `ts_proto_library` I am trying to ask compiler to use same ts file as I am able to use in the code.
@egormodin
Copy link
Contributor Author

@thesayyn

Copy link

aspect-workflows bot commented Jun 5, 2024

Test

⚠️ GitHub Actions build #560 failed.


Buildifier

Buildifier managed files require formatting

--- ./examples/proto_grpc/BUILD.bazel	2024-06-05 21:15:58.316102467 +0000
+++ /tmp/buildifier-tmp-1368899131	2024-06-05 21:16:38.956390386 +0000
@@ -33,9 +33,9 @@
 ts_proto_library(
     name = "com_google_protobuf_timestamp",
     node_modules = ":node_modules",
+    proto = "@com_google_protobuf//:timestamp_proto",
     # Usually not accessible.
     proto_srcs = ["@com_google_protobuf//:timestamp.proto"],
-    proto = "@com_google_protobuf//:timestamp_proto",
 )
 
 ts_proto_library(
@@ -45,8 +45,8 @@
     proto_srcs = logger_srcs,
     visibility = ["//visibility:public"],
     deps = [
-        "//examples/connect_node/proto",
         ":com_google_protobuf_timestamp",
+        "//examples/connect_node/proto",
     ],
 )
 

💡 Run the following to apply the suggested formatting fixes

bazel run //:buildifier

Format

Formatting check has failed

💡 Some formatting failures can be fixed automatically by running the command below, while others may require manual fixes

bazel run //:format -- examples/proto_grpc/BUILD.bazel

ℹ️ A patch file containing the changes has been archived as an artifact of this build

@egormodin
Copy link
Contributor Author

In the current tapestry project we are using go_googleapis protos from https://github.com/googleapis/googleapis/tree/master/google, but I was not able to modify WORKSPACE file to bring it in to the example project

@thesayyn
Copy link
Member

thesayyn commented Jun 5, 2024

No worries, i believe timestamp proto is different than googleapis as for WKT types no generation needed. I'll fix it.

BTW you can do in MODULE.bazel for rules_ts.

http_archive = use_repo_rule("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
    name = "googleapis", 
    urls = ["https://github.com/googleapis/googleapis/archive/ef8d4740570bd099dd1a484e200a3d9433f793e2.tar.gz"]
)

@egormodin
Copy link
Contributor Author

right, no generation needed, but the current example results with the same error that I have by using go_googleapis.

@egormodin
Copy link
Contributor Author

btw to add @go_googleapis space we have a rule in the project WORKSPACE file

load("@com_google_api//:repository_rules.bzl", "switched_rules_by_language")

switched_rules_by_language(
    name = "com_google_googleapis_imports",
    go = True,
    grpc = True,
    java = True,
)

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.

2 participants