makefile: rework the dependency graph for faster test runs startup (#… #175
Conversation
| printf './%s\n' $(filter-out $(PARALLEL_TEST),$(TESTS)); \ | ||
| find t -name 'run-*' -print; \ | ||
| } \ | ||
| printf './%s\n' $(filter-out $(PARALLEL_TEST),$(TESTS)) $(PARALLEL_TEST:%=t/run-%-*) \ |
There was a problem hiding this comment.
Can you explain why the "find t -name" line is no longer required?
There was a problem hiding this comment.
find t -name 'run-*' is replaced by a shell glob expansion with the $(PARALLEL_TEST:%=t/run-%-*) part, which turns the printf call into something that looks like printf './%s\n' [list of non parallel tests] t/run-<parallel-test-name>-* ....
So if TESTS contains a b c and PARALLEL_TESTS is set to a b, we'll get printf './%s\n' c t/run-a-* t/run-b-* which the shell will expand to the run-* files for the a and b tests. This way we ensure that we only grab files for what's currently defined in PARALLEL_TESTS and not some leftovers that happen to begin with run-, and we'll also fail if any of the parallel tests didn't get any run-* files generated for it (because the shell glob expansion will fail, so we'll end up passing a non-existing file to GNU Parallel, and it will error out).
Makefile
Outdated
| # Don't regenerate when doing a simple `make clean` invocation and make_config.mk | ||
| # already exists, becuase we will be using the variables from the existing one and | ||
| # deleting it immediately afterwards. | ||
| ifneq ($(strip $(and $(wildcard make_config.mk),$(filter-out clean,$(MAKECMDGOALS) make_config.mk))),make_config.mk) |
There was a problem hiding this comment.
This is a complex statement. It took me a bit to parse to determine what you were attempting to do. Perhaps a better comment?
There was a problem hiding this comment.
I reworded it a bit. Let me know if the new phrasing is better.
Makefile
Outdated
| export USE_CLANG="$(USE_CLANG)"; \ | ||
| export LIB_MODE="$(LIB_MODE)"; \ | ||
| $(info * GEN make_config.mk) | ||
| dummy := $(shell (export ROCKSDB_ROOT="$(CURDIR)" && \ |
There was a problem hiding this comment.
Is there a reason you changed the ";" to "&&" ?
There was a problem hiding this comment.
No real reason. I'll revert.
| build_tools/format-diff.sh -c | ||
| buckifier/check_buck_targets.sh | ||
| build_tools/check-sources.sh | ||
| endif |
There was a problem hiding this comment.
Why not just have the check rule depend on the other rules?
There was a problem hiding this comment.
Because then they will execute before the check rule, and that means that e.g. formatting issues will prevent us from running the unit tests until they are fixes, which is undesirable during development.
Makefile
Outdated
| # (restarts are caused by Makefiles being updated during the parsing of the Makefile, | ||
| # which is exactly what happens when make_config.mk is regenerated and included). | ||
| ifeq ($(MAKE_RESTARTS),) | ||
| # Don't regenerate when doing a simple `make clean` invocation and make_config.mk |
There was a problem hiding this comment.
There are likely other rules that do not require a make_config, such as make watch-log
There was a problem hiding this comment.
I'll try to go over the targets and add such rules to the check.
isaac-io
force-pushed
the
174-makefile-speed-up-test-runs-startup-time
branch
from
0846390
to
ec3f7e3
Compare
Makefile
Outdated
| .PHONY: check_0 | ||
| check_0: gen_parallel_tests | ||
| .PHONY: check_0 check_1 | ||
| check_0: all $(parallel_tests) |
There was a problem hiding this comment.
- How do the non-parallel tests get executed in this rule?
- Why does it depend on "all" -- why not TESTS like check_1?
There was a problem hiding this comment.
- The test executable is executed once and runs all of the tests serially, instead of a
run-*script that executes only a single test case from an executable. - That's... actually a bug.
checkneeds to depend onallfor running the ldb tests, but I mistakenly moved it tocheck_0instead of making it depend on$(TESTS). Thanks for catching this.
| @@ -2310,7 +2312,7 @@ rocksdbjavastaticnexusbundlejar: rocksdbjavageneratepom | |||
|
|
|||
| # A version of each $(LIBOBJECTS) compiled with -fPIC | |||
|
|
|||
| jl/%.o: %.cc | |||
| jl/%.o: %.cc make_config.mk | |||
There was a problem hiding this comment.
Just curious if these make more sense as "order-only" rules or if you mean for them to now be dependencies (they were not previously)?
If more things were in this file (like LITE, DEBUG, etc), it might make more sense as a true dependency...
There was a problem hiding this comment.
I mean for them to be actual dependencies, because make_config.mk contains compilation flags, so if it gets updated (e.g. due to a library being installed or removed; and the changes in build_detect_platform ensure that this only happen if there was an actual change in contents) the object files need to be rebuilt. This wasn't the case previously, but I believe it was in error, so I tacked this change on while I was fixing the dependency graph.
There was a problem hiding this comment.
I wonder if the %.d rule should also depend on make_config.mk then?
There was a problem hiding this comment.
Nice catch! I completely overlooked the %.d targets.
| @@ -2310,7 +2312,7 @@ rocksdbjavastaticnexusbundlejar: rocksdbjavageneratepom | |||
|
|
|||
| # A version of each $(LIBOBJECTS) compiled with -fPIC | |||
|
|
|||
| jl/%.o: %.cc | |||
| jl/%.o: %.cc make_config.mk | |||
There was a problem hiding this comment.
I wonder if the %.d rule should also depend on make_config.mk then?
isaac-io
force-pushed
the
174-makefile-speed-up-test-runs-startup-time
branch
2 times, most recently
from
34c1650
to
871ff30
Compare
) The Makefile has many places where it needlessly invokes a sub-make instead of declaring the dependencies correctly. Rework the dependency graph so that at least running tests doesn't involve invoking make three times. This also avoids pollution in /dev/shm caused by the needless creation of temporary directories on make restarts and sub-make invocations.
isaac-io
force-pushed
the
174-makefile-speed-up-test-runs-startup-time
branch
from
871ff30
to
cb4e649
Compare
…174)
The Makefile has many places where it needlessly invokes a sub-make instead of declaring the dependencies correctly. Rework the dependency graph so that at least running tests doesn't involve invoking make three times.
This also avoids pollution in /dev/shm caused by the needless creation of temporary directories on make restarts and sub-make invocations.