twitchtv / twirp

A simple RPC framework with protobuf service definitions
https://twitchtv.github.io/twirp/docs/intro.html
Apache License 2.0
7.21k stars 326 forks source link

Makefile does not enforce a particular protoc version, so generated files churn #18

Closed jmank88 closed 6 years ago

jmank88 commented 6 years ago

Running make from master/v5.0.0 (db96cdf354e8dc053e5ee5fe890bb0a7f18123ab) without any changes modifies 9 python test files. I expected no changes to checked-in files.

Each includes additional code to register a service descriptor, similar to:

_SVC = _descriptor.ServiceDescriptor(
  name='Svc',
  full_name='twirp.internal.twirptest.proto.Svc',
  file=DESCRIPTOR,
  index=0,
  options=None,
  serialized_start=54,
  serialized_end=141,
  methods=[
  _descriptor.MethodDescriptor(
    name='Send',
    full_name='twirp.internal.twirptest.proto.Svc.Send',
    index=0,
    containing_service=None,
    input_type=_MSG,
    output_type=_MSG,
    options=None,
  ),
])
_sym_db.RegisterServiceDescriptor(_SVC)

DESCRIPTOR.services_by_name['Svc'] = _SVC

Should these files be updated with these changes?

spenczar commented 6 years ago

Hm, this probably indicates that you have a different version of protoc than I do - I don't see any changes when I run make.

This is unfortunate. It's a little hard to pin the version of protoc and keep it the same across invocations. We might need to revisit the install_proto.sh script that the Makefile invokes and lock things down tighter.

jmank88 commented 6 years ago

It looks like I'm running 3.4.0 but the script would otherwise install 3.1.0.

jmank88 commented 6 years ago

The generated Go files have begun to drift out of sync as well - #52 as a reference. It would be neat for CI to run make (or at least generation) and disallow modifications.

spenczar commented 6 years ago

I'm not sure I'd call this fixed yet; we're going to continue to drift in the future. I think it will be fixed when make enforces a particular protoc version.

I'm leaning towards saying make should just check protoc --version and yell at you if it doesn't exactly match a blessed version (3.5.1 seems good, I guess?).

spenczar commented 6 years ago

Fixed in #63.