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

Improve Protobuf's handling of __init__.py files #10498

Closed
Eric-Arellano opened this issue Jul 28, 2020 · 11 comments
Closed

Improve Protobuf's handling of __init__.py files #10498

Eric-Arellano opened this issue Jul 28, 2020 · 11 comments
Assignees
Labels
Projects
Milestone

Comments

@Eric-Arellano
Copy link
Contributor

Now that we no longer automagically create files, it's confusing what users should do with Protobuf.

If they aren't using PEP 420, they will need to create an __init__.py in the protobuf_library() folder, along with a python_library target to own it. Then, the new __init__.py dependency inference rule will find this file and infer a dependency on it. This is not at all obvious, nor elegant to require putting a Python file in your generic .proto folder.

Instead, we should likely add an option generate_init_pys in a new [protoc-python] subsystem, along with a plugin field on protobuf_library called generate_init_pys. If true, this will generate the __init__.py file automatically.

@Eric-Arellano Eric-Arellano added this to the 2.0.0.alpha0 milestone Jul 28, 2020
@Eric-Arellano Eric-Arellano self-assigned this Jul 28, 2020
@Eric-Arellano Eric-Arellano added this to To do in Pants 2.0 via automation Jul 28, 2020
@benjyw
Copy link
Sponsor Contributor

benjyw commented Jul 28, 2020

Sounds like relying on PEP420 is what proto expects you to do?

@benjyw
Copy link
Sponsor Contributor

benjyw commented Jul 29, 2020

I'm not sure we should be in the business of generating __init__.py files. For example, how would we know what to put in it? Perhaps it needs to be a namespace package invocation? For example, if we generate _pb2.py into a parallel package to some existing python code, but in a separate source root?

The requirement to have an __init__.py file in the package to which you generate _pb2.py files is not really unique to Pants. You'd have to do this one way or another anyway.

We can encourage people to embed .proto files alongside their python code, instead of in separate packages or separate source roots. Then the existing __init__.py comes along for the ride.

@danieljanes
Copy link

Embedding .proto files alongside Python code is not really an option for us. One of the reasons for using ProtoBuf is to have shared definitions across multiple languages, so treating ProtoBuf as it's own "language" makes sense. Having the .proto files alongside our Python code makes it easier from a Python perspective, but harder from the perspective of every other language in the repo.

@benjyw
Copy link
Sponsor Contributor

benjyw commented Aug 4, 2020

That makes sense. We've come up with a different solution, hopefully more elegant: #10549

@benjyw
Copy link
Sponsor Contributor

benjyw commented Aug 4, 2020

Would this work for your case? Essentially your protobuf_library targets would specify python_source_root=src/python (or wherever your python sources live) and the generated files would be nested under that source root, so they would take advantage of whatever existing __init__.pys live there.

@danieljanes
Copy link

That sounds pretty good. Is there a way to try this once it's merged? Are there nightly builds?

@benjyw
Copy link
Sponsor Contributor

benjyw commented Aug 4, 2020

You can actually run Pants at any master SHA. I will write up how to do so (you may need to update your pants runner script, I'll follow up with how).

@benjyw
Copy link
Sponsor Contributor

benjyw commented Aug 5, 2020

Closed in #10549

@benjyw benjyw closed this as completed Aug 5, 2020
Pants 2.0 automation moved this from To do to Done Aug 5, 2020
@benjyw
Copy link
Sponsor Contributor

benjyw commented Aug 5, 2020

@danieljanes I've updated the runner script at https://pantsbuild.github.io/setup/pants to support running at any (recent) master SHA. See comments in the script for how to do that.

@danieljanes
Copy link

Thanks @benjyw. I've used this successfully to update the example repo https://github.com/danieljanes/pants-python-protobuf based on the PR from @Eric-Arellano - no __init__ files under src/proto necessary any more!

@benjyw
Copy link
Sponsor Contributor

benjyw commented Aug 6, 2020

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Pants 2.0
  
Done
Development

No branches or pull requests

3 participants