Rework proto generation (#1371)

* Rework proto generation The changes here are: - Fix the default goal (using "default" target is not doing it). - Bail out with a useful message if proto submodule is not checked out. - Replace the use of docker image with downloading the protoc binary and building the gogofast plugin ourselves. This gives us a greater control over the invocation of protoc. - Use rsync to copy the generated code, instead of pax. Pax did not work for me (it was complaining about the unknown -0 flag). The control over the protoc invocation will be useful later, when we will want to generate a code with data structures in one place and the collector code elsewhere. The collector code may or may not depend on gRPC, but data structures have no need for it. This split will happen when we move the gRPC code out of the OTLP exporter module into a submodule. Getting rid of docker has the upside that the generated files do not belong to root, so there is no hassle of changing the ownership of the files, and it is not requires to use sudo for the `clean` target. And not using docker is faster. The downside of this work is that it depends on more tools: rsync, unzip and wget. I can only hope that macOS users have those tools too, and that those tools are invoked the same. * Update protogen workflow
open-telemetry · Dec 7, 2020 · 0021ab0 · 0021ab0
1 parent 787e3f4
commit 0021ab0
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 41 deletions.
diff --git a/.github/workflows/protogen.yml b/.github/workflows/protogen.yml
@@ -14,8 +14,8 @@ jobs:
     - uses: actions/setup-go@v2
       with:
         go-version: '^1.14.0'
-    - run: sudo apt-get -y install pax
-    - run: make -f Makefile.proto protobuf
+    - run: sudo apt-get -y install rsync wget unzip
+    - run: make -f Makefile.proto protobuf clean
     - uses: stefanzweifel/git-auto-commit-action@v4
       id: commit-changes
       with:

diff --git a/Makefile.proto b/Makefile.proto
@@ -13,60 +13,117 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-# This Makefile.proto has rules to generate *.pb.go files in
-# `exporters/otlp/internal/opentelemetry-proto-gen` from the .proto files in
-# `exporters/otlp/internal/opentelemetry-proto` using protoc with a go plugin.
+# This Makefile.proto has rules to generate go code for otlp
+# exporter. It does it by copying the proto files from
+# `exporters/otlp/internal/opentelemetry-proto` (which is a
+# submodule that needs to be checked out) into `gen/proto`, changing
+# the go_package option to a valid string, generating the go files and
+# finally copying the files into the module. The files are not
+# generated in place, because protoc generates a too-deep directory
+# structure.
 #
-# The protoc binary and other tools are sourced from a docker image
-# `PROTOC_IMAGE`.
+# Currently, all the generated code is in
+# `exporters/otlp/internal/opentelemetry-proto-gen`.
 #
-# Prereqs: The archiving utility `pax` is installed.
+# Prereqs: wget (for downloading the zip file with protoc binary),
+# unzip (for unpacking the archive), rsync (for copying back the
+# generated files).
 
-PROTOC_IMAGE          := namely/protoc-all:1.29_2
-PROTOBUF_VERSION      := v1
-OTEL_PROTO_SUBMODULE  := exporters/otlp/internal/opentelemetry-proto
-PROTOBUF_GEN_DIR      := exporters/otlp/internal/opentelemetry-proto-gen
-PROTOBUF_TEMP_DIR     := gen/pb-go
-PROTO_SOURCE_DIR      := gen/proto
-SUBMODULE_PROTO_FILES := $(wildcard $(OTEL_PROTO_SUBMODULE)/opentelemetry/proto/*/$(PROTOBUF_VERSION)/*.proto \
-                           $(OTEL_PROTO_SUBMODULE)/opentelemetry/proto/collector/*/$(PROTOBUF_VERSION)/*.proto)
-SOURCE_PROTO_FILES    := $(subst $(OTEL_PROTO_SUBMODULE),$(PROTO_SOURCE_DIR),$(SUBMODULE_PROTO_FILES))
+PROTOC_VERSION := 3.14.0
 
-default: protobuf
+TOOLS_DIR                       := $(abspath ./.tools)
+TOOLS_MOD_DIR                   := ./internal/tools
+PROTOBUF_VERSION                := v1
+OTEL_PROTO_SUBMODULE            := exporters/otlp/internal/opentelemetry-proto
+GEN_TEMP_DIR                    := gen
+SUBMODULE_PROTO_FILES           := $(wildcard $(OTEL_PROTO_SUBMODULE)/opentelemetry/proto/*/$(PROTOBUF_VERSION)/*.proto) $(wildcard $(OTEL_PROTO_SUBMODULE)/opentelemetry/proto/collector/*/$(PROTOBUF_VERSION)/*.proto)
 
-.PHONY: protobuf protobuf-source gen-protobuf copy-protobufs
-protobuf: protobuf-source gen-protobuf copy-protobufs
+ifeq ($(strip $(SUBMODULE_PROTO_FILES)),)
+$(error Submodule at $(OTEL_PROTO_SUBMODULE) is not checked out, use "git submodule update --init")
+endif
 
-protobuf-source: $(SOURCE_PROTO_FILES) | $(PROTO_SOURCE_DIR)/
+PROTOBUF_GEN_DIR   := exporters/otlp/internal/opentelemetry-proto-gen
+PROTOBUF_TEMP_DIR  := $(GEN_TEMP_DIR)/pb-go
+PROTO_SOURCE_DIR   := $(GEN_TEMP_DIR)/proto
+SOURCE_PROTO_FILES := $(subst $(OTEL_PROTO_SUBMODULE),$(PROTO_SOURCE_DIR),$(SUBMODULE_PROTO_FILES))
 
-# Changes go_package in .proto file to point to repo-local location
-define exec-replace-pkgname
-sed  's,go_package = "github.com/open-telemetry/opentelemetry-proto/gen/go,go_package = "go.opentelemetry.io/otel/exporters/otlp/internal/opentelemetry-proto-gen,' < $(1) > $(2)
+.DEFAULT_GOAL := protobuf
 
-endef
+UNAME_S := $(shell uname -s)
+UNAME_M := $(shell uname -m)
+
+ifeq ($(UNAME_S),Linux)
+
+PROTOC_OS := linux
+PROTOC_ARCH := $(UNAME_M)
+
+else ifeq ($(UNAME_S),Darwin)
+
+PROTOC_OS := osx
+PROTOC_ARCH := x86_64
+
+endif
 
-# replace opentelemetry-proto package name by go.opentelemetry.io/otel specific version
-$(SOURCE_PROTO_FILES): $(PROTO_SOURCE_DIR)/%.proto: $(OTEL_PROTO_SUBMODULE)/%.proto
-	@mkdir -p $(@D)
-	$(call exec-replace-pkgname,$<,$@)
+PROTOC_ZIP_URL := https://github.com/protocolbuffers/protobuf/releases/download/v$(PROTOC_VERSION)/protoc-$(PROTOC_VERSION)-$(PROTOC_OS)-$(PROTOC_ARCH).zip
 
-# Command to run protoc using docker image
-define exec-protoc-all
-docker run -v `pwd`:/defs $(PROTOC_IMAGE) $(1)
+$(TOOLS_DIR)/PROTOC_$(PROTOC_VERSION):
+	@rm -f "$(TOOLS_DIR)"/PROTOC_* && \
+	touch "$@"
 
+# Depend on a versioned file (like PROTOC_3.14.0), so when version
+# gets bumped, we will depend on a nonexistent file and thus download
+# a newer version.
+$(TOOLS_DIR)/protoc/bin/protoc: $(TOOLS_DIR)/PROTOC_$(PROTOC_VERSION)
+	echo "Fetching protoc $(PROTOC_VERSION)" && \
+	rm -rf $(TOOLS_DIR)/protoc && \
+	wget -O $(TOOLS_DIR)/protoc.zip $(PROTOC_ZIP_URL) && \
+	unzip $(TOOLS_DIR)/protoc.zip -d $(TOOLS_DIR)/protoc-tmp && \
+	rm $(TOOLS_DIR)/protoc.zip && \
+	touch $(TOOLS_DIR)/protoc-tmp/bin/protoc && \
+	mv $(TOOLS_DIR)/protoc-tmp $(TOOLS_DIR)/protoc
+
+$(TOOLS_DIR)/protoc-gen-gogofast: $(TOOLS_MOD_DIR)/go.mod $(TOOLS_MOD_DIR)/go.sum $(TOOLS_MOD_DIR)/tools.go
+	cd $(TOOLS_MOD_DIR) && \
+	go build -o $(TOOLS_DIR)/protoc-gen-gogofast github.com/gogo/protobuf/protoc-gen-gogofast && \
+	go mod tidy
+
+# Return a sed expression for replacing the go_package option in proto
+# file with a one that's valid for us.
+#
+# Example: $(call get-sed-expr,$(PROTOBUF_GEN_DIR))
+define get-sed-expr
+'s,go_package = "github.com/open-telemetry/opentelemetry-proto/gen/go,go_package = "go.opentelemetry.io/otel/$(1),'
 endef
 
-gen-protobuf: $(SOURCE_PROTO_FILES) | $(PROTOBUF_GEN_DIR)/
-	$(foreach file,$(subst ${PROTO_SOURCE_DIR}/,,$(SOURCE_PROTO_FILES)),$(call exec-protoc-all, -i $(PROTO_SOURCE_DIR) -f ${file} -l gogo -o ${PROTOBUF_TEMP_DIR}))
+.PHONY: protobuf
+protobuf: protobuf-source gen-protobuf copy-protobufs
+
+.PHONY: protobuf-source
+protobuf-source: $(SOURCE_PROTO_FILES)
+
+# This copies proto files from submodule into $(PROTO_SOURCE_DIR),
+# thus satisfying the $(SOURCE_PROTO_FILES) prerequisite. The copies
+# have their package name replaced by go.opentelemetry.io/otel.
+$(PROTO_SOURCE_DIR)/%.proto: $(OTEL_PROTO_SUBMODULE)/%.proto
+	@ \
+	mkdir -p $(@D); \
+	sed -e $(call get-sed-expr,$(PROTOBUF_GEN_DIR)) "$<" >"$@.tmp"; \
+	mv "$@.tmp" "$@"
 
-# requires `pax` to be installed, as it has consistent options for both BSD (Darwin) and Linux
-copy-protobufs: | $(PROTOBUF_GEN_DIR)/
-	find ./$(PROTOBUF_TEMP_DIR)/go.opentelemetry.io/otel/$(PROTOBUF_GEN_DIR) -type f -print0 | \
-      pax -0 -s ',^./$(PROTOBUF_TEMP_DIR)/go.opentelemetry.io/otel/$(PROTOBUF_GEN_DIR),,' -rw ./$(PROTOBUF_GEN_DIR)
+.PHONY: gen-protobuf
+gen-protobuf: $(SOURCE_PROTO_FILES) $(TOOLS_DIR)/protoc-gen-gogofast $(TOOLS_DIR)/protoc/bin/protoc
+	@ \
+	mkdir -p "$(PROTOBUF_TEMP_DIR)"; \
+	set -e; for f in $^; do \
+	  if [[ "$${f}" == $(TOOLS_DIR)/* ]]; then continue; fi; \
+	  echo "protoc $${f#"$(PROTO_SOURCE_DIR)/"}"; \
+	  PATH="$(TOOLS_DIR):$${PATH}" $(TOOLS_DIR)/protoc/bin/protoc --proto_path="$(PROTO_SOURCE_DIR)" --gogofast_out="plugins=grpc:$(PROTOBUF_TEMP_DIR)" "$${f}"; \
+	done
 
-$(PROTO_SOURCE_DIR)/ $(PROTOBUF_GEN_DIR)/:
-	mkdir -p $@
+.PHONY: copy-protobufs
+copy-protobufs:
+	@rsync -a $(PROTOBUF_TEMP_DIR)/go.opentelemetry.io/otel/exporters .
 
 .PHONY: clean
 clean:
-	rm -rf ./gen
+	rm -rf $(GEN_TEMP_DIR)
diff --git a/internal/tools/go.mod b/internal/tools/go.mod
@@ -4,6 +4,7 @@ go 1.14
 
 require (
 	github.com/client9/misspell v0.3.4
+	github.com/gogo/protobuf v1.3.1
 	github.com/golangci/golangci-lint v1.33.0
 	github.com/itchyny/gojq v0.11.2
 	golang.org/x/tools v0.0.0-20201013201025-64a9e34f3752

diff --git a/internal/tools/go.sum b/internal/tools/go.sum
@@ -103,6 +103,8 @@ github.com/gofrs/flock v0.8.0/go.mod h1:F1TvTiK9OcQqauNUHlbJvyl9Qa1QvF/gOUDKA14j
 github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
 github.com/gogo/protobuf v1.2.1 h1:/s5zKNz0uPFCZ5hddgPdo2TK2TVrUNMn0OOX8/aZMTE=
 github.com/gogo/protobuf v1.2.1/go.mod h1:hp+jE20tsWTFYpLwKvXlhS1hjn+gTNwPg2I6zVXpSg4=
+github.com/gogo/protobuf v1.3.1 h1:DqDEcV5aeaTmdFBePNpYsp3FlcVH/2ISVVM9Qf8PSls=
+github.com/gogo/protobuf v1.3.1/go.mod h1:SlYgWuQ5SjCEi6WLHjHCa1yvBfUnHcTbrrZtXPKa29o=
 github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=
 github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc=
 github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A=
@@ -229,6 +231,7 @@ github.com/jtolds/gls v4.20.0+incompatible h1:xdiiI2gbIgH/gLH7ADydsJ1uDOEzR8yvV7
 github.com/jtolds/gls v4.20.0+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfVYBRgL+9YlvaHOwJU=
 github.com/julienschmidt/httprouter v1.2.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w=
 github.com/kisielk/errcheck v1.1.0/go.mod h1:EZBBE59ingxPouuu3KfxchcWSUPOHkagtvWXihfKN4Q=
+github.com/kisielk/errcheck v1.2.0/go.mod h1:/BMXB+zMLi60iA8Vv6Ksmxu/1UDYcXs4uQLJ+jE2L00=
 github.com/kisielk/gotool v1.0.0 h1:AV2c/EiW3KqPNT9ZKl07ehoAGi4C5/01Cfbblndcapg=
 github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
 github.com/klauspost/compress v1.10.7/go.mod h1:aoV0uJVorq1K+umq18yTdKaF57EivdYsUV+/s2qKfXs=
@@ -545,6 +548,7 @@ golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxb
 golang.org/x/tools v0.0.0-20180221164845-07fd8470d635/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20180525024113-a5b4c53f6e8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
+golang.org/x/tools v0.0.0-20181030221726-6c7e314b6563/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20190110163146-51295c7ec13a/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20190221204921-83362c3779f5/go.mod h1:9Yl7xja0Znq3iFh3HoIrodX9oNMXvdceNzlUR8zjMvY=

diff --git a/internal/tools/tools.go b/internal/tools/tools.go
@@ -21,4 +21,5 @@ import (
 	_ "github.com/golangci/golangci-lint/cmd/golangci-lint"
 	_ "github.com/itchyny/gojq"
 	_ "golang.org/x/tools/cmd/stringer"
+	_ "github.com/gogo/protobuf/protoc-gen-gogofast"
 )