Re: [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 2535 bytes --]

On Mon, Jun 28, 2021 at 01:35:51PM -0700, Cole Helbling wrote:
> On Mon Jun 28, 2021 at 10:31 AM PDT, Alyssa Ross wrote:
> > My previous attempt[1] at this was totally wrong, because I didn't
> > understand what change needed to be made, and didn't have any way of
> > testing it.
> >
> > To make sure this time that I got it right, I wrote a package for
> > tremplin[2], which was the only Chromium OS component I could find
> > that was written in Go and actually used vm_protos.  With these
> > changes, I was able to add "${vm_protos}/lib/gopath" to tremplin's
> > extraSrcPaths, and get a successful build.
> >
> > [1]: https://spectrum-os.org/lists/archives/spectrum-devel/20210627165035.899276-1-hi@alyssa.is/
> > [2]: https://chromium.googlesource.com/chromiumos/platform/tremplin
> >
> > Alyssa Ross (2):
> >   common-mk: add goproto_library source_relative opt
> >   vm_tools: proto: set go_package correctly
> >
> >  common-mk/proto_library.gni   | 7 +++++++
> >  vm_tools/proto/BUILD.gn       | 5 +++++
> >  vm_tools/proto/tremplin.proto | 2 +-
> >  vm_tools/proto/vm_crash.proto | 2 +-
> >  vm_tools/proto/vm_guest.proto | 1 +
> >  vm_tools/proto/vm_host.proto  | 1 +
> >  6 files changed, 16 insertions(+), 2 deletions(-)
> >
> > --
> > 2.31.1

Hey Cole, glad you're still around. :)

> Good thing I didn't (have enough time to) review that previous patch :P
>
> Is this something unique to this project, or should / will this be upstreamed
> (if it isn't already)?

It's probably not that useful to upstream, because they're still using the
old version of protoc-gen-go, and when they do get around to upgrading,
I imagine they'll want to do it across the board, so it'll end up being
different to the fix I've made here that only affects one component.
I expect they'd want to just set source_relative unconditionally, for
example, whereas I've made it an option that defaults to off.  I did it
that way because I don't have the resources to check all of Chromium OS,
when the only bits of it I know how to build are the bits I've packaged
for Spectrum.

Additionally, I'm a bit wary about upstreaming things to Google, because
they have a mandatory CLA.  If we had some big change where we'd really
benefit from them maintaining it upstream, I might be able to overcome
my reservations, but this isn't it.

> That aside, diff LGTM.
>
> Reviewed-by: Cole Helbling <cole.e.helbling@outlook.com>

Thanks!  I'll be sending some patches shortly that add these patches to
chromiumOSPackages.vm_protos in Spectrum's Nixpkgs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]