Skip to content

Commit

Permalink
Fix docker-push.sh execution (cadence-workflow#3919)
Browse files Browse the repository at this point in the history
**Why this commit exists:**

"build and push master" buildkite steps are failing, due to both:
- their images not having `unzip` available
- `protoc` failing to run on alpine after adding `unzip`, e.g. `/bin/sh: .build/bin/protoc: not found`
- pip v21 dropped support for python2 entirely, and `cqlsh` hasn't been updated in nearly 4 years and is stuck on 2.7

This fixes that by 1) adding unzip, 2) not executing thrift/proto codegen during those release builds (and in other locations), and 3) preventing pip from going beyond v20.x

---

**Lots more details:**

Protoc is apparently incompatible with alpine images, it has something to do with musl and (g?)libc incompatibilities.
In the alpine images, `sh` complains `.build/bin/protoc: not found`, but after some more digging you can see `ldd` has more info:
```
ldd .build/bin/protoc
	/lib64/ld-linux-x86-64.so.2 (0x7f9b1f366000)
	libm.so.6 => /lib64/ld-linux-x86-64.so.2 (0x7f9b1f366000)
	libpthread.so.0 => /lib64/ld-linux-x86-64.so.2 (0x7f9b1f366000)
	libc.so.6 => /lib64/ld-linux-x86-64.so.2 (0x7f9b1f366000)
Error loading shared library ld-linux-x86-64.so.2: No such file or directory (needed by .build/bin/protoc)
Error relocating .build/bin/protoc: __strftime_l: symbol not found
```

After a bit of experimenting, it seems that the semi-common tricks of "install `libc6-compat` or `gcompat`" do not work for protoc.
I'm not really sure why.

So our options are basically one of:
- Abandon alpine for this build section.  It just builds binaries that are copied to the next step, so it won't affect the final image size anyway.  (this builds successfully, but I have not checked the resulting binaries)
- Keep alpine, build protoc from source, hope it compiles successfully against musl
- Keep alpine, but don't do codegen in this build (this works)

Option 3 seemed worth trying in depth.  After some toying around and (as usual) learning something new about Make, I found this tactic.
It appears to be well-supported and consistent, so I'm OK with using it, though it's rather strange at first glance.
Binaries are still downloaded/built during release builds with this approach, but that seems like a relatively low-risk / low-cost consequence.

This all also means that devs will download and build these tools as part of a normal `make bins`, but similarly will not run codegen unless needed.
Since that kinda helps "prime" the dev environment, that seems totally fine / possibly even a good thing.  Codegen *should* still run when necessary though.
  • Loading branch information
Groxx authored Jan 25, 2021
1 parent 710cd61 commit 88e1b65
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 5 deletions.
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ RUN go install
# Build Cadence binaries
FROM golang:1.13.6-alpine AS builder

RUN apk add --update --no-cache ca-certificates make git curl mercurial bzr
RUN apk add --update --no-cache ca-certificates make git curl mercurial bzr unzip

WORKDIR /cadence

Expand Down
30 changes: 27 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,10 @@ ARCH = $(shell uname -m)
PROTOC_URL = https://github.com/protocolbuffers/protobuf/releases/download/v$(PROTOC_VERSION)/protoc-$(PROTOC_VERSION)-$(subst Darwin,osx,$(OS))-$(ARCH).zip
$(BIN)/protoc: | $(BIN)
@echo "Getting protoc $(PROTOC_VERSION)"
rm -rf $(BIN)/protoc.zip $(BIN)/protoc-zip
curl -sSL $(PROTOC_URL) -o $(BIN)/protoc.zip
unzip -q $(BIN)/protoc.zip -d $(BIN)/protoc-zip
cp $(BIN)/protoc-zip/bin/protoc $(BIN)/protoc
rm $(BIN)/protoc.zip

# any generated file - they all depend on each other / are generated at once, so any will work
PROTO_GEN_SRC = ./.gen/proto/admin/v1/service.pb.go
Expand All @@ -113,9 +113,23 @@ THRIFT_SRCS := $(shell find idls -name '*.thrift')
# idls/thrift/thing.thrift -> thing.thrift -> thing -> ./.gen/go/thing/thing.go
THRIFT_GEN_SRC := $(foreach tsrc,$(basename $(subst idls/thrift/,,$(THRIFT_SRCS))),./$(THRIFT_GENDIR)/$(tsrc)/$(tsrc).go)

# this is a "false" dependency chain, but it convinces make that "need to make thriftrw(-plugin-yarpc)"
# does not mean "need to update generated code" because there is no rule body for the thrift files, i.e.:
# No recipe for 'idls/thrift/admin.thrift' and no prerequisites actually changed.
# No need to remake target 'idls/thrift/admin.thrift'
# therefore it does not cause a remake any of $(THRIFT_GEN_SRC).
# note that this relies on thrift files having NO build rules whatsoever, including anywhere else.
#
# this is a little weird, but useful. it means we download and build the tools in all cases, but do
# not actually run the generators unless the generated files are missing (or older than their .thrift file).
#
# on its own, this would mean that changes to the binaries themselves (i.e. go.mod version changes)
# would not trigger a re-generation pass. that is corrected by making thrift-gen-srcs depend on go.mod.
$(THRIFT_SRCS): $(BIN)/thriftrw $(BIN)/thriftrw-plugin-yarpc

# how to generate each thrift file.
# note that each generated file depends on ALL thrift files - this is necessary because they can import each other.
$(THRIFT_GEN_SRC): $(THRIFT_SRCS) $(BIN)/thriftrw $(BIN)/thriftrw-plugin-yarpc
$(THRIFT_GEN_SRC): $(THRIFT_SRCS) go.mod
@# .gen/go/thing/thing.go -> thing.go -> "thing " -> thing -> idls/thrift/thing.thrift
@echo 'thriftrw for idls/thrift/$(strip $(basename $(notdir $@))).thrift...'
@$(BIN_PATH) $(BIN)/thriftrw \
Expand Down Expand Up @@ -194,11 +208,21 @@ PROTO_DIRS = $(sort $(dir $(PROTO_FILES)))
proto-lint: $(BIN)/buf
cd $(PROTO_ROOT) && ../$(BIN)/buf lint

# same trick as for thrift.
#
# for proto though, it's important that this works because the alpine images used to build release
# binaries are not compatible with the protoc tool (glibc vs musl issues).
#
# for this reason, proto *must not* be executed in those release builds, unless for some reason the
# files are not up to date (in which case a failure is probably correct behavior).
# the tools can still be downloaded and built safely, though it's wasted effort to do so.
$(PROTO_FILES): $(BIN)/protoc $(BIN)/protoc-gen-go $(BIN)/protoc-gen-go-grpc

# this line: -I=$(BIN)/protoc-zip/include
# includes the well-known protobuf types, e.g. timestamp, wrappers, and the language meta-definition for extensions.
# they're part of the protoc zip, and "normally" are installed globally and found implicitly.
# since they're in an abnormal location, passing that path explicitly lets protoc find them.
$(PROTO_GEN_SRC): $(BIN)/protoc $(BIN)/protoc-gen-go $(BIN)/protoc-gen-go-grpc $(PROTO_FILES)
$(PROTO_GEN_SRC): $(PROTO_FILES) go.mod
@mkdir -p $(PROTO_OUT)
$(BIN)/protoc \
--plugin $(BIN)/protoc-gen-go \
Expand Down
3 changes: 2 additions & 1 deletion docker/buildkite/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
unzip \
&& rm -rf /var/lib/apt/lists/*

RUN curl https://bootstrap.pypa.io/get-pip.py | python
# pip 21+ drops support for python2 entirely, we seem to need at least 19 to build cqlsh
RUN pip install -U 'pip<21'
RUN pip install PyYAML==3.13 cqlsh==5.0.4

# https://github.com/docker-library/golang/blob/c1baf037d71331eb0b8d4c70cff4c29cf124c5e0/1.4/Dockerfile
Expand Down

0 comments on commit 88e1b65

Please sign in to comment.