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-... [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.