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

Add protobuf linting using Buf #14540

Merged
merged 8 commits into from
Mar 11, 2022
Merged

Conversation

jyggen
Copy link
Member

@jyggen jyggen commented Feb 19, 2022

This PR aims to add initial linting support for protobufs.

Some notes:

  • I put the code in backend/codegen/protobuf/lint/buf for now, though happy to move it to backend/protobuf/lint/buf instead (as suggested in Protobuf linting (and more!) using Buf CLI #13189).
  • I put the external tool for Buf in backend/codegen/protobuf instead of in backend/codegen/protobuf/lint/buf since I imagine it can and will be reused to future Buf functionality (like the BC breaking check).
  • I haven't gotten Protobuf dependencies working yet, so one of the tests fail. I believe the import in my test proto is correct from a protobuf perspective - so not sure if I have to add some manual dependency wiring in the test itself, change the rule to use TransitiveTargets instead somehow, or integrate directly with infer_protobuf_dependencies(). Leaving that to the experts :)
  • It's not visible in the tests, so I need to add an additional test for that, but Buf itself has some issues resolving imports when the paths it receieves in --path are prefixed with their source root(s). I got around it in my own PoC plugin by simply telling Buf about my source roots in its config file, but I believe using StrippedSourceFiles would be a more proper solution by the sound of it (not to mention that the config key I used in the PoC only works in their v1beta config format).

@Eric-Arellano
Copy link
Contributor

I put the code in backend/codegen/protobuf/lint/buf for now, though happy to move it to backend/protobuf/lint/buf instead

Ah, I wasn't careful enough in my comment. I agree with your decision to use backend/codegen/protobuf/lint/buf :) The point I care about is it being lint/buf.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Huzzah!

change the rule to use TransitiveTargets instead somehow

Idk how Buf works, but Protoc does need all transitive deps to be present:

# Protoc needs all transitive dependencies on `protobuf_libraries` to work properly. It won't
# actually generate those dependencies; it only needs to look at their .proto files to work
# with imports.
transitive_targets = await Get(
TransitiveTargets, TransitiveTargetsRequest([request.protocol_target.address])
)

Once direct deps are working, it'd be great to update the test to check transitive deps are handled, like:

def test_generates_python(rule_runner: RuleRunner) -> None:
# This tests a few things:
# * We generate the correct file names.
# * Protobuf files can import other protobuf files, and those can import others
# (transitive dependencies). We'll only generate the requested target, though.
# * We can handle multiple source roots, which need to be preserved in the final output.
rule_runner.write_files(
{
"src/protobuf/dir1/f.proto": dedent(
"""\
syntax = "proto3";
package dir1;
message Person {
string name = 1;
int32 id = 2;
string email = 3;
}
"""
),
"src/protobuf/dir1/f2.proto": dedent(
"""\
syntax = "proto3";
package dir1;
"""
),
"src/protobuf/dir1/BUILD": "protobuf_sources()",
"src/protobuf/dir2/f.proto": dedent(
"""\
syntax = "proto3";
package dir2;
import "dir1/f.proto";
"""
),
"src/protobuf/dir2/BUILD": (
"protobuf_sources(dependencies=['src/protobuf/dir1'], "
"python_source_root='src/python')"
),
# Test another source root.
"tests/protobuf/test_protos/f.proto": dedent(
"""\
syntax = "proto3";
package test_protos;
import "dir2/f.proto";
"""
),
"tests/protobuf/test_protos/BUILD": (
"protobuf_sources(dependencies=['src/protobuf/dir2'])"
),
}
)

but I believe using StrippedSourceFiles would be a more proper solution by the sound of it

That is what we do for Protoc

# NB: By stripping the source roots, we avoid having to set the value `--proto_path`
# for Protobuf imports to be discoverable.
all_stripped_sources_request = Get(
StrippedSourceFiles,
SourceFilesRequest(
tgt[ProtobufSourceField]
for tgt in transitive_targets.closure
if tgt.has_field(ProtobufSourceField)
),
)
target_stripped_sources_request = Get(
StrippedSourceFiles, SourceFilesRequest([request.protocol_target[ProtobufSourceField]])
)

It would indeed be great to update tests to use source roots, at least one test with no source root (i.e. /) and one with multiple. rules_integration_test.py might give some inspiration.

--

I'm not sure what is going wrong with dependencies..it looks right to me. Let me know if you feel stuck on it, I can try to play around with it or pair program :) I'm definitely not a Protobuf expert, but implemented the original Protoc support so could hopefully be helpful.

src/python/pants/backend/codegen/protobuf/buf.py Outdated Show resolved Hide resolved
src/python/pants/backend/codegen/protobuf/buf.py Outdated Show resolved Hide resolved
src/python/pants/backend/codegen/protobuf/buf.py Outdated Show resolved Hide resolved
src/python/pants/backend/codegen/protobuf/buf.py Outdated Show resolved Hide resolved
@jyggen jyggen changed the title [Draft] Add protobuf linting using Buf Add protobuf linting using Buf Mar 2, 2022
@jyggen jyggen marked this pull request as ready for review March 2, 2022 16:05
@jyggen
Copy link
Member Author

jyggen commented Mar 2, 2022

All comments have been addressed. Managed to get dependencies and source roots working after scratching my head for a bit. Added a test for it to make sure it really works :)

@jyggen jyggen requested a review from Eric-Arellano March 2, 2022 16:07
@stuhood stuhood requested a review from tdyas March 3, 2022 20:52
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

This is awesome. Thank you so much! This will be a main highlight for Pants 2.11 :)

@Eric-Arellano Eric-Arellano merged commit 732d944 into pantsbuild:main Mar 11, 2022
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