From 6c8a17b441314aff350baf36a2e51050267b1ad4 Mon Sep 17 00:00:00 2001 From: Krzesimir Nowak Date: Thu, 26 Nov 2020 17:13:15 +0100 Subject: [PATCH 1/2] 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. --- Makefile.proto | 135 ++++++++++++++++++++++++++++------------ internal/tools/go.mod | 1 + internal/tools/go.sum | 4 ++ internal/tools/tools.go | 1 + 4 files changed, 102 insertions(+), 39 deletions(-) diff --git a/Makefile.proto b/Makefile.proto index 417c3b31c47..5e51af3a5c9 100644 --- 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 index a2481bbb42c..8fb7cca4465 100644 --- 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 index 713ea73a563..c4e1682b1cd 100644 --- 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 index f0418c71d3a..306a95e0daf 100644 --- 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" ) From 79702a80a4c44238ee56c16ea605e45bce9413b6 Mon Sep 17 00:00:00 2001 From: Krzesimir Nowak Date: Fri, 4 Dec 2020 09:08:55 +0100 Subject: [PATCH 2/2] Update protogen workflow --- .github/workflows/protogen.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/protogen.yml b/.github/workflows/protogen.yml index a82d841efda..8b8f641170c 100644 --- 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: