[PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x
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
We need this for the go_package changes in protoc-gen-go 1.5.x. If we didn't use source-relative paths, the full module path would be repeated in the output location, so we'd get paths like src/chromiumos/vm_tools/vm_crash/chromiumos/vm_tools/vm_crash/vm_crash.pb.go. To avoid the duplication, we either need to set source_relative, or set proto_out_dir to just go/src. The latter isn't workable, because then everything two libraries that both use common.proto will both generate outputs called "go/src/common.pb.go", which will upset GN. --- common-mk/proto_library.gni | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/common-mk/proto_library.gni b/common-mk/proto_library.gni index fb9fb4231d..23645a134f 100644 --- a/common-mk/proto_library.gni +++ b/common-mk/proto_library.gni @@ -225,6 +225,9 @@ template("proto_library") { # proto_lib_dirs (optional) # Directories to search for protos a proto file depends on. # proto_in_dir and "${sysroot}/usr/share/proto" are added by default. +# source_relative (optional) +# If true, the output file is placed in the same relative directory as the +# input file (but under proto_out_dir). template("goproto_library") { action(target_name) { forward_variables_from(invoker, @@ -254,6 +257,10 @@ template("goproto_library") { go_plugin_parameters = [] + if (defined(invoker.source_relative) && invoker.source_relative) { + go_plugin_parameters += [ "paths=source_relative" ] + } + if (defined(invoker.gen_grpc) && invoker.gen_grpc) { go_plugin_parameters += [ "plugins=grpc" ] } -- 2.31.1
protoc-gen-go 1.5.x has become a lot stricter about this. We have to use import_mapping for common.proto because it ends up being included in multiple Go libraries. I'm not sure why it needs to be built once per library, but that's the way it works. --- 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 + 5 files changed, 9 insertions(+), 2 deletions(-) diff --git a/vm_tools/proto/BUILD.gn b/vm_tools/proto/BUILD.gn index 79c9b94c9f..aadc40165c 100644 --- a/vm_tools/proto/BUILD.gn +++ b/vm_tools/proto/BUILD.gn @@ -60,6 +60,8 @@ goproto_library("vm-crash-gorpcs") { proto_in_dir = "./" proto_out_dir = "go/src/chromiumos/vm_tools/vm_crash" gen_grpc = true + source_relative = true + import_mapping = [ "common.proto=chromiumos/vm_tools/vm_crash" ] sources = [ "${proto_in_dir}/common.proto", "${proto_in_dir}/vm_crash.proto", @@ -97,6 +99,7 @@ goproto_library("tremplin-gorpcs") { proto_in_dir = "./" proto_out_dir = "go/src/chromiumos/vm_tools/tremplin_proto" gen_grpc = true + source_relative = true sources = [ "${proto_in_dir}/tremplin.proto" ] } @@ -120,6 +123,8 @@ goproto_library("vm-gorpcs") { proto_in_dir = "./" proto_out_dir = "go/src/chromiumos/vm_tools/vm_rpc" gen_grpc = true + source_relative = true + import_mapping = [ "common.proto=chromiumos/vm_tools/vm_rpc" ] sources = [ "${proto_in_dir}/common.proto", "${proto_in_dir}/vm_guest.proto", diff --git a/vm_tools/proto/tremplin.proto b/vm_tools/proto/tremplin.proto index aac76f7a9e..e6a7bbed0e 100644 --- a/vm_tools/proto/tremplin.proto +++ b/vm_tools/proto/tremplin.proto @@ -8,7 +8,7 @@ option cc_enable_arenas = true; // This file defines services for tremplin, the container springboard service. package vm_tools.tremplin; -option go_package = "tremplin_proto"; +option go_package = "chromiumos/vm_tools/tremplin_proto"; // This needs to be duplicated because the gyp rule for building // go code makes it difficult to have imports. diff --git a/vm_tools/proto/vm_crash.proto b/vm_tools/proto/vm_crash.proto index 6e4f62fe13..3cd4279989 100644 --- a/vm_tools/proto/vm_crash.proto +++ b/vm_tools/proto/vm_crash.proto @@ -7,7 +7,7 @@ syntax = "proto3"; option cc_enable_arenas = true; package vm_tools.cicerone; -option go_package = "vm_crash"; +option go_package = "chromiumos/vm_tools/vm_crash"; import "common.proto"; diff --git a/vm_tools/proto/vm_guest.proto b/vm_tools/proto/vm_guest.proto index 86f11d0812..d0946078d5 100644 --- a/vm_tools/proto/vm_guest.proto +++ b/vm_tools/proto/vm_guest.proto @@ -8,6 +8,7 @@ option cc_enable_arenas = true; // This file defines services that will be running in the guest VM. package vm_tools; +option go_package = "chromiumos/vm_tools/vm_rpc"; import "common.proto"; import "google/protobuf/timestamp.proto"; diff --git a/vm_tools/proto/vm_host.proto b/vm_tools/proto/vm_host.proto index a8bd066f61..19759b0271 100644 --- a/vm_tools/proto/vm_host.proto +++ b/vm_tools/proto/vm_host.proto @@ -8,6 +8,7 @@ option cc_enable_arenas = true; // This file defines services that will be running on the host for the VM. package vm_tools; +option go_package = "chromiumos/vm_tools/vm_rpc"; import "common.proto"; -- 2.31.1
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
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)? That aside, diff LGTM. Reviewed-by: Cole Helbling <cole.e.helbling@outlook.com>
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.
participants (2)
-
Alyssa Ross -
Cole Helbling