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

Adds args string_list option to proto_compile #58

Merged
merged 2 commits into from
Jan 31, 2017
Merged

Adds args string_list option to proto_compile #58

merged 2 commits into from
Jan 31, 2017

Conversation

pcj
Copy link
Contributor

@pcj pcj commented Jan 21, 2017

  • Also makes the langs attribute non-mandatory.

Closes #57

* Also makes the `langs` attribute non-mandatory.

Closes #57
@pcj
Copy link
Contributor Author

pcj commented Jan 21, 2017

@perezd Does this LGTY?

Copy link
Contributor

@perezd perezd left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for turning this around so fast.

@perezd
Copy link
Contributor

perezd commented Jan 21, 2017

Interesting second observation, using this approach, I don't believe I can have proto A depend on proto B and have them both appear in a descriptor_set. Can I specify proto_deps in proto_compile in some way?

EDIT: Oh wait, spoke too soon, I think the deps attr works OK for this actually.

@perezd
Copy link
Contributor

perezd commented Jan 22, 2017

Another thing I'm noticing, there is no way to reference well known protos via only proto_compile, is there? That is a filegroup of protos listed at @com_github_google_protobuf//:well_known_protos...Is there a way to get these in the right place?

@pcj
Copy link
Contributor Author

pcj commented Jan 22, 2017

I updated the example for well_known_protos. WDYT?

@perezd
Copy link
Contributor

perezd commented Jan 23, 2017

So that does appear to work, but it feels awkward. I would press forward with this, but I will probably just wrap that in a macro def to reduce the noise at call sites.

Larger more strategic question: maybe we should consider making proto_descriptor_set_out a "proto language" rule? Its described as effectively an output target anyhow like java/cpp/etc.:
https://developers.google.com/protocol-buffers/docs/techniques#self-description

WDYT?

@pcj
Copy link
Contributor Author

pcj commented Jan 23, 2017

So what would that look like? Something like...?

proto_language(
    name = "descriptor",
    args = [
         "--include_imports",
         "--include_source_info",
     ],
     pb_imports = ["external/com_github_google_protobuf/src"],
     pb_inputs = ["@com_github_google_protobuf//:well_known_protos"],
)

descriptor_proto_library(
  name = "api",
  protos = [...],
)

@perezd
Copy link
Contributor

perezd commented Jan 24, 2017

Yeah I think that makes the most sense. The output of that would be api.pb.descriptor. Although, I would still prefer "srcs" over "protos" 😄

@pcj pcj merged commit af37086 into master Jan 31, 2017
@pcj
Copy link
Contributor Author

pcj commented Jan 31, 2017

@perezd Merged this and created separate issue for descriptor language issue #62.

@perezd
Copy link
Contributor

perezd commented Jan 31, 2017 via email

@pcj pcj deleted the proto_args branch October 27, 2017 13:01
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