* [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x
@ 2021-06-28 17:31 Alyssa Ross
2021-06-28 17:31 ` [PATCH platform2 1/2] common-mk: add goproto_library source_relative opt Alyssa Ross
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Alyssa Ross @ 2021-06-28 17:31 UTC (permalink / raw)
To: devel; +Cc: Puck Meerburg
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH platform2 1/2] common-mk: add goproto_library source_relative opt 2021-06-28 17:31 [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x Alyssa Ross @ 2021-06-28 17:31 ` Alyssa Ross 2021-06-28 17:31 ` [PATCH platform2 2/2] vm_tools: proto: set go_package correctly Alyssa Ross 2021-06-28 20:35 ` [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x Cole Helbling 2 siblings, 0 replies; 5+ messages in thread From: Alyssa Ross @ 2021-06-28 17:31 UTC (permalink / raw) To: devel; +Cc: Puck Meerburg 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH platform2 2/2] vm_tools: proto: set go_package correctly 2021-06-28 17:31 [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x Alyssa Ross 2021-06-28 17:31 ` [PATCH platform2 1/2] common-mk: add goproto_library source_relative opt Alyssa Ross @ 2021-06-28 17:31 ` Alyssa Ross 2021-06-28 20:35 ` [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x Cole Helbling 2 siblings, 0 replies; 5+ messages in thread From: Alyssa Ross @ 2021-06-28 17:31 UTC (permalink / raw) To: devel; +Cc: Puck Meerburg 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x 2021-06-28 17:31 [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x Alyssa Ross 2021-06-28 17:31 ` [PATCH platform2 1/2] common-mk: add goproto_library source_relative opt Alyssa Ross 2021-06-28 17:31 ` [PATCH platform2 2/2] vm_tools: proto: set go_package correctly Alyssa Ross @ 2021-06-28 20:35 ` Cole Helbling 2021-06-30 9:12 ` Alyssa Ross 2 siblings, 1 reply; 5+ messages in thread From: Cole Helbling @ 2021-06-28 20:35 UTC (permalink / raw) To: Alyssa Ross, devel; +Cc: Puck Meerburg 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 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x 2021-06-28 20:35 ` [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x Cole Helbling @ 2021-06-30 9:12 ` Alyssa Ross 0 siblings, 0 replies; 5+ messages in thread From: Alyssa Ross @ 2021-06-30 9:12 UTC (permalink / raw) To: Cole Helbling; +Cc: devel, Puck Meerburg [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-30 9:12 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-28 17:31 [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x Alyssa Ross 2021-06-28 17:31 ` [PATCH platform2 1/2] common-mk: add goproto_library source_relative opt Alyssa Ross 2021-06-28 17:31 ` [PATCH platform2 2/2] vm_tools: proto: set go_package correctly Alyssa Ross 2021-06-28 20:35 ` [PATCH platform2 0/2] Make vm_protos compatible with protoc-gen-go 1.5.x Cole Helbling 2021-06-30 9:12 ` Alyssa Ross
Code repositories for project(s) associated with this public inbox https://spectrum-os.org/git/crosvm https://spectrum-os.org/git/doc https://spectrum-os.org/git/mktuntap https://spectrum-os.org/git/nixpkgs https://spectrum-os.org/git/spectrum https://spectrum-os.org/git/ucspi-vsock https://spectrum-os.org/git/www This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).