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