Skip to content

Commit b6b70ff

Browse files
authored
Detect when IDL submodule is not initialized, warn user (cadence-workflow#4172)
This is largely a continuation of cadence-workflow#4155 , and is related to / helps resolve cadence-workflow#4162 . When the IDL submodule doesn't have files in it, builds can fail in a variety of obtuse ways. Currently it fails when running protoc, as there's a symlink from proto/ into idls/, e.g. causing error from cadence-workflow#4155 : ``` uber/cadence/matching/v1/service.proto:213:12: "api.v1.TaskListPartitionMetadata" is not defined. proto/public: warning: directory does not exist. uber/cadence/api/v1/history.proto: File not found. ... ``` Since this needs to be fixed *before* running a `make ...`, as its files are used as target prerequisites, it can't be auto-initialized by the makefile. Instead, it now fails the build with a descriptive error, and the user needs to resolve it before trying again. --- While building and testing this I also noticed that the fake protoc/thrift binaries were sometimes surprising me, so I added a warning about them to `make clean`. They *might* be reasonable to automatically clean up as well, but I'm leaving them in place for now, so e.g. a build process doesn't accidentally fake -> clean -> do a full build.
1 parent 3e2ffc3 commit b6b70ff

File tree

1 file changed

+22
-3
lines changed

1 file changed

+22
-3
lines changed

Makefile

+22-3
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ PROTOC_UNZIP_DIR = $(BIN)/protoc-$(PROTOC_VERSION)-zip
166166
# otherwise this must be a .PHONY rule, or the buf bin / symlink could become out of date.
167167
PROTOC_VERSION_BIN = protoc-$(PROTOC_VERSION)
168168
$(BIN)/$(PROTOC_VERSION_BIN): | $(BIN)
169-
@echo "downloading protoc $(PROTOC_VERSION)"
169+
@echo "downloading protoc $(PROTOC_VERSION): $(PROTOC_URL)"
170170
@# recover from partial success
171171
@rm -rf $(BIN)/protoc.zip $(PROTOC_UNZIP_DIR)
172172
@# download, unzip, copy to a normal location
@@ -178,6 +178,15 @@ $(BIN)/$(PROTOC_VERSION_BIN): | $(BIN)
178178
# Codegen targets
179179
# ====================================
180180

181+
# IDL submodule must be populated, or files will not exist -> prerequisites will be wrong -> build will fail.
182+
# Because it must exist before the makefile is parsed, this cannot be done automatically as part of a build.
183+
# Instead: call this func in targets that require the submodule to exist, so that target will not be built.
184+
#
185+
# THRIFT_FILES is just an easy identifier for "the submodule has files", others would work fine as well.
186+
define ensure_idl_submodule
187+
$(if $(THRIFT_FILES),,$(error idls/ submodule must exist, or build will fail. Run `git submodule update --init` and try again))
188+
endef
189+
181190
# codegen is done when thrift and protoc are done
182191
$(BUILD)/codegen: $(BUILD)/thrift $(BUILD)/protoc | $(BUILD)
183192
@touch $@
@@ -190,6 +199,7 @@ THRIFT_GEN := $(subst idls/thrift/,.build/,$(THRIFT_FILES))
190199

191200
# thrift is done when all sub-thrifts are done
192201
$(BUILD)/thrift: $(THRIFT_GEN) | $(BUILD)
202+
$(call ensure_idl_submodule)
193203
@touch $@
194204

195205
# how to generate each thrift book-keeping file.
@@ -217,6 +227,7 @@ PROTO_DIRS = $(sort $(dir $(PROTO_FILES)))
217227
#
218228
# After compilation files are moved to final location, as plugins adds additional path based on proto package.
219229
$(BUILD)/protoc: $(PROTO_FILES) $(BIN)/$(PROTOC_VERSION_BIN) $(BIN)/protoc-gen-gogofast $(BIN)/protoc-gen-yarpc-go | $(BUILD)
230+
$(call ensure_idl_submodule)
220231
@mkdir -p $(PROTO_OUT)
221232
@echo "protoc..."
222233
@$(foreach PROTO_DIR,$(PROTO_DIRS),$(BIN)/$(PROTOC_VERSION_BIN) \
@@ -243,6 +254,9 @@ $(BUILD)/protoc: $(PROTO_FILES) $(BIN)/$(PROTOC_VERSION_BIN) $(BIN)/protoc-gen-g
243254
# this will ensure that committed code will be used rather than re-generating.
244255
# must be manually run before (nearly) any other targets.
245256
.fake-codegen: .fake-protoc .fake-thrift
257+
$(warning build-tool binaries have been faked, you will need to delete the $(BIN) folder if you wish to build real ones)
258+
@# touch a marker-file for a `make clean` warning. this does not impact behavior.
259+
touch $(BIN)/fake-codegen
246260

247261
# "build" fake binaries, and touch the book-keeping files, so Make thinks codegen has been run.
248262
# order matters, as e.g. a $(BIN) newer than a $(BUILD) implies Make should run the $(BIN).
@@ -252,7 +266,10 @@ $(BUILD)/protoc: $(PROTO_FILES) $(BIN)/$(PROTOC_VERSION_BIN) $(BIN)/protoc-gen-g
252266

253267
.fake-thrift: | $(BIN) $(BUILD)
254268
touch $(BIN)/thriftrw $(BIN)/thriftrw-plugin-yarpc
255-
$(if $(THRIFT_GEN),touch $(THRIFT_GEN),) # maybe ignoring empty idl folder
269+
@# if the submodule exists, touch thrift_gen markers to fake their generation.
270+
@# if it does not, do nothing - there are none.
271+
$(if $(THRIFT_GEN),touch $(THRIFT_GEN),)
272+
touch $(BUILD)/thrift
256273

257274
# ====================================
258275
# other intermediates
@@ -385,7 +402,9 @@ release: ## Re-generate generated code and run tests
385402
clean: ## Clean binaries and build folder
386403
rm -f $(BINS)
387404
rm -Rf $(BUILD)
388-
@echo '# rm -rf $(BIN) # not removing tools dir, it is rarely necessary'
405+
$(if \
406+
$(filter $(BIN)/fake-codegen, $(wildcard $(BIN)/*)), \
407+
$(warning fake build tools may exist, delete the $(BIN) folder to get real ones if desired),)
389408

390409
# v----- not yet cleaned up -----v
391410

0 commit comments

Comments
 (0)