Pin the number of jobs in some Make targets #3640
Conversation
steakhal
requested review from
csordasmarton,
dkrupp and
bruntib
as code owners
Makefile
Outdated
| PIP_DEV_DEPS_CMD = $(MAKE) -C $(CC_ANALYZER) -j1 pip_dev_deps && \ | ||
| $(MAKE) -C $(CC_WEB) -j1 pip_dev_deps && \ |
analyzer/tests/Makefile
Outdated
| @@ -66,7 +66,7 @@ test_functional_in_env: venv_dev | |||
| $(ACTIVATE_DEV_VENV) && $(FUNCTIONAL_TEST_CMD) | |||
|
|
|||
| test_build_logger: | |||
| $(MAKE) -C tools/build-logger test | |||
| $(MAKE) -C tools/build-logger -j1 test | |||
There was a problem hiding this comment.
The problem with the test target is that it depends on the all target:
And what all target will do is to build the package:
This is why your make command failes and gives you an error when you run it on multiple threads.
Can't we remove this dependency from the test target and assume you already built a package from ldlogger? That would solve the race condition.
There was a problem hiding this comment.
Possible, but why should we assume it's already done?
There was a problem hiding this comment.
If we do not assume that and if you run the for example the all target with multiple threads it is not guaranteed that the dependencies will be run in the correct order. For example the all target depends on the pack32bit target:
This target depends on two other targets: 32bit and packbin. The order is important here because the packbin target will assume that 32bit already run. If you run it with multiple threads this is not guaranteed.
There was a problem hiding this comment.
These should not depend on the order. If that's the case then packbin should explicitly depend on 32bit.
There was a problem hiding this comment.
Anyway, I updated the patch to simply omit the all target from the test target deps.
steakhal
force-pushed
the
pin-num-jobs
branch
2 times, most recently
from
2a14888
to
2da8ddc
Compare
There was a problem hiding this comment.
The test cases related to analyzer tools are failing. With your changes now you have to build a package from ldlogger before running the test cases:
codechecker/.github/workflows/test.yml
Lines 42 to 46 in f11eb1a
I think that would solve the problem.
If someone runs `make -j16 test_unit test_analyzer test_web_sqlite`, then the command would spuriously fail due to a filesystem race-condition. The `cp ldlogger build/bin` would basically try to copy a not-yet-existing file, causing a failure immediately, or later in the test pipeline, when it exercises the logger. Causing an error similar to this: ``` make[1]: Leaving directory `codechecker/analyzer' make: *** [test_analyzer] Error 2 make: *** Waiting for unfinished jobs.... ``` Let's work around the issue by omitting the `all` target from the `test*` targets. This way if it's already built, tests should just pass.
If someone runs
make -j16 test_unit test_analyzer test_web_sqlite,then the command would spuriously fail due to a filesystem race-condition.
The
cp ldlogger build/binwould basically try to copy a not-yet-existing file,causing a failure immediately, or later in the test pipeline, when it
exercises the logger. Causing an error similar to this:
Let's work around the issue by omitting the
alltarget from thetest*targets.This way if it's already built, tests should just pass.