Checkout idls if folder is empty

[longquanzheng]

What changed?
Checkout idls if folder is empty

Why?

After this #4152 , the development is broken for people who start development on Cadence.
The errors is:

proto/public: warning: directory does not exist.
uber/cadence/api/v1/history.proto: File not found.
uber/cadence/shared/v1/history.proto:27:1: Import "uber/cadence/api/v1/history.proto" was not found or had errors.
uber/cadence/shared/v1/history.proto:30:3: "api.v1.HistoryEvent" is not defined.
uber/cadence/shared/v1/history.proto:31:3: "api.v1.HistoryEvent" is not defined.
make: *** [.build/protoc] Error 1

If you checkout Cadence from beginning you will see it.
This should be safe enough because if your idls is already checked out, this target won't do anything.

How did you test it?
Local test

(qlong-make-check-idl)$make cadence-bench
if [ -z "" ]; then \
		git submodule update idls/; \
	fi
Submodule path 'idls': checked out '9cc53eec153e97be0868ece127be8b34e31866ee'
protoc...
.bin/copyright --verifyOnly
goimports...
...

Potential risks
na

Release notes
na

Documentation Changes
na

[coveralls]

Pull Request Test Coverage Report for Build 094d7eeb-603b-4726-8873-85026b441aee

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 37 unchanged lines in 14 files lost coverage.
• Overall coverage increased (+0.07%) to 59.764%

---

Files with Coverage Reduction	New Missed Lines	%
host/archivalTest.go	1	97.81%
client/history/client.go	2	44.78%
client/history/metricClient.go	2	49.43%
common/membership/rpServiceResolver.go	2	73.58%
common/persistence/cassandra/cassandraPersistenceUtil.go	2	92.62%
common/persistence/historyStore.go	2	66.41%
common/task/weightedRoundRobinTaskScheduler.go	2	89.12%
common/types/mapper/thrift/shared.go	2	71.12%
service/history/handler.go	2	46.67%
service/history/queue/timer_queue_processor.go	2	58.77%

Totals
Change from base Build 122a430b-cedb-4927-bfc4-b3ef638d7b34:	0.07%
Covered Lines:	87785
Relevant Lines:	146886

---

💛 - Coveralls

[Groxx]

"definite no" signs:

• this will currently no-op if there's a checkout-idls-if-empty file in the folder, which is almost definitely not what you want. it should either be .PHONY or have a real target file.
• this needs to be FAR before lint. at the moment the only thing these prerequisites say is that you must have files in there at some point before the linter runs... but that's practically the last thing that happens, and these files are needed for codegen that happens much earlier. there's no way that's correct.
• even if you move it earlier in the dependency list, makefile parsing has already completed, and the list of files-to-generate is already wrong and will not be updated. this is a fairly fundamental limitation on anything like this in makefiles.

this isn't really something you can do "inline" like this, and have anything even approaching correct behavior. it might work in some situations, but definitely not all.

since the files need to exist in order to figure out what files need to be generated: there's almost no way to do this without having it set up before the makefile is run. so there are really only 3 options:

• tell people to set up the submodules before making. either in docs, or in an early check on the makefile, or something. they need to do it, and then run make [whatever], because parsing for a build has to happen after the files exist.
• "synchronously" init the folder during parsing, e.g. do that if ... git submodule update near the top and execute it all the time. I'll pretty strongly recommend against this though, it's far worse to troubleshoot for users.
• have the "set up and build" make target do two steps, e.g. recursively make one thing and then the other: git submodule update; $(MAKE) bins. this'll ensure a "clean" parse for the make bins part, since it's started after the submodule is updated. we could perhaps set this up as a make bootstrap that we recommend people run once... but they still need to be told to do that and do it. in that case, why not just tell them to git submodule update.

we could also make a somewhat larger change, to make thrift / protoc codegen prerequisites not depend on specific files at all. then it could be initialized "inline" like this, and the dependency graph will still be correct. I'm not sure off-hand how feasible that is, but it probably isn't too bad. we'd likely be giving up parallelism though.

---

personally i'd probably vote for a "detect empty idl folder, fail early" blocking step, somewhere early-on before codegen runs. we can say "run git submodule update and try again" pretty easily there.

[longquanzheng]

Thanks for the comment. Really appreciate it.

"detect empty idl folder, fail early" blocking step, somewhere early-on before codegen runs. we can say "run git submodule update and try again" pretty easily there.

@Groxx that works for me. Will do that.

[longquanzheng]

@Groxx could you point out where we should put the if check and fail early with some message?

[Groxx]

Yep, definitely. Let me try to craft something since I can't do inline comments outside the small context window, apparently.

[Groxx]

This is the list of high-level dependencies, near the top of the makefile, in roughly-reverse order (maybe I should change that. order doesn't really matter in this part.): https://github.com/uber/cadence/blob/70031dedfef4e13679a09ff8628c395b37c3d37b/Makefile#L37-L43

Since this is something that thrift needs to have in place before it runs, thrift needs to depend on it. So add something like this, to mark that there's a dependency:

$(BUILD)/thrift: $(BUILD)/submodule  # thrift depends on the submodule
$(BUILD)/submodule: # placeholder for the submodule target.  not actually necessary, but might be good for visibility.

Now, before thrift runs, submodule must have already completed. (protoc can run in parallel and ignore it, since that's not in the idl folder apparently)

Somewhere lower (wherever you feel it makes sense), define the recipe as something like this:

$(BUILD)/submodule:
	$(if $(THRIFT_FILES),,$(error Git submodule must be initialized, run `git submodule update --init` and try again))
	@touch $@

^ When THRIFT_FILES found files (it's a $(shell find idls) result), that'll fall into the "if" branch, which is empty (the piece after the first ,).
When it's empty, you end up with $(if ,,$(error ...)) which means the "if" is false and the else branch is chosen (after the second comma).
So that $(if...) turns into $(error ...) and that's what Make will execute. $(error ...) is a "print to stderr and fail" helper, which will prevent it from reaching the next line.

This is effectively the same as your if [ -z block, but it avoids a shell command (and doesn't automatically git submodule update). Which is very minor, but we might as well be efficient when we can, and it's a bit more OS-agnostic (though this isn't a very OS-agnostic makefile anyway, so YMMV).

Lastly, @touch $@ will make sure this whole thing only runs once (per make clean anyway) once it succeeds, by creating the target file since it's not necessary after that point.[1] $@ is just shorthand for "the thing before : that's being built", it's one of the few magic-make-vars I actually use.

Ultimately, that'll print something like this when it fails, and it'll stop the build:

Makefile:51: *** Git submodule must be initialized, run `git submodule update --init` and try again.  Stop.

And when it succeeds, it'll make that $(BUILD)/submodule file, and won't bother to check again. A make clean will check again, since that'll delete that file, but odds are pretty good that the submodule is still around at that point.

---

[1]: probably. Arguably this is something that's meaningful to run on all builds, which would make it a .PHONY target, so you'll see stuff like this sometimes in makefiles:

.PHONY: $(BUILD)/submodule
$(BUILD)/submodule:
	$(if $(THRIFT_FILES),,git submodule update idls/)

Unfortunately, since .PHONY targets are always run, it implies to Make that they always change, so doing this will mean thrift needs to be rerun, and thus anything that depends on thrift... which is nearly everything.

So don't do this.

You could do something like this, anywhere in the makefile:

$(if $(THRIFT_FILES),,$(error Git submodule...))

And that would work... but it would also mean you can't run ANYTHING without the submodule, which sucks. E.g. make help wouldn't work, because it would fail while parsing, before it even executes the help target.

[Groxx]

And! While typing all that out, I realized there's a simpler option that we should probably do. The instructions above are fairly general "how to add another step", so I'll keep them, but for this we can probably just change this:
https://github.com/uber/cadence/blob/70031dedfef4e13679a09ff8628c395b37c3d37b/Makefile#L192-L193
into this:

$(BUILD)/thrift: $(THRIFT_GEN) | $(BUILD)
	$(if $(THRIFT_GEN),,$(error ...))  # thrift_gen or thrift_files, either should work fine
	@touch $@

since $(BUILD)/thrift still gets run whether idl files were found or not. It'll break the build at this point, and (after pulling the submodule) should resume correctly, since none of the earlier thrift files were generated + the $(BUILD)/thrift file still does not exist. Should be reliable + run every time + add effectively zero runtime when it does nothing.

[longquanzheng]

@Groxx I tried what you suggested but does't work with me.
Still error

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.
uber/cadence/shared/v1/history.proto:27:1: Import "uber/cadence/api/v1/history.proto" was not found or had errors.
uber/cadence/shared/v1/history.proto:30:3: "api.v1.HistoryEvent" is not defined.
uber/cadence/shared/v1/history.proto:31:3: "api.v1.HistoryEvent" is not defined.
make: *** [.build/protoc] Error 1
qlong@~/cadence:

I may misunderstood it. I think maybe you can take over to fix it for us. (you can reuse or create other PR, I am totally cool with it)

[longquanzheng]

@Groxx will come up with a better fix, so I will just close this.