Skip to content

Fix parallel builds and use them in gh workflow #797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 19, 2022

Conversation

ndim
Copy link
Member

@ndim ndim commented May 13, 2022

The old gettext 0.18.3 po/Makefile.in.in has problems when used with parallel make (make -jN with N>1). Running make -j$(nproc) -l$(nproc) distcheck on a 12 core system fails reproducibly.

gettext 0.19.1 has apparently fixed those problems with parallel builds, as running make -j$(nproc) -l$(nproc) distcheck on the same 12 core system reproducibly succeeds with gettext 0.19.1's po/Makefile.in.in file

When parallel builds work, we can now also make use of them during the CI check github Action workflow, so we change all make commands there to make -j$(nproc) -l$(nproc).

This is a PR to trigger and test the CI builds, and to allow discussion about requiring gettext 0.19.1 which is a release not even 8 years old, making it the youngest build tool release required to build libgphoto2.

@ndim ndim force-pushed the fix-parallel-builds branch 2 times, most recently from 7156224 to eb63ef9 Compare May 13, 2022 23:22
@ndim
Copy link
Member Author

ndim commented May 13, 2022

While we do not have proper benchmarks from the Github Actions, we can see that adding -j2 -l2 to the make command lines consistently decreases times, sometimes significantly. As the -j2 checks have run

step -j1 -j2 same sleep iff isatty(1) same
SHA 82251c0 eb63ef9 eb63ef9 d26577c d26577c
CI LOGS 725 727 731 728 730
autoreconf 24 20 24 18 26
configure 20 16 20 15 21
make all 61 27 35 27 34
make check 4 2 2 1 3
make install 5 5 5 4 5
make installcheck n/a n/a n/a n/a n/a
make distcheck 137 87 104 83 104
TOTAL 278 180 219 176 225

The isatty column produces about the same values as the -j2 column as it only affects builds with non-standard camlib set. However, the -j2 column and the isatty column appear to have run on a faster runner than the -j1 and same column. The same column is the same code as the isatty column, but run again, apparently on a similar runner as the -j1 column. So the best columns to compare for evaluating this PR's effect are the -j1 column vs the same column.

The few seconds here and there are not significant, but reducing make all to about 55%, reducing make distcheck to about 75%, and the total runtime to about 80% is a lot better than nothing, keeping in mind we are only running with an nproc value of just 2 on our Github Action runners.

@ndim
Copy link
Member Author

ndim commented May 14, 2022

step -j1 -j2 sleep iff isatty(1) --with-camlibs=everything
SHA 82251c0 eb63ef9 d26577c 36a3666
CI LOGS 725 727 728 729
autoreconf 24 20 18 19
configure 20 16 15 16
make all 61 27 27 42
make check 4 2 1 1
make install 5 5 4 12
make installcheck n/a n/a n/a n/a
make distcheck 137 87 83 118
TOTAL 278 180 176 235

FWIW, on the faster runners (autoreconf about 20 seconds; the slower runners take about 25 seconds for autoreconf) with -j2 we could even build with --with-camlibs=everything and still be faster than we were with -j1 and the default camlib set. Within one set of runners, we would probably be about even, or still a bit slower.

I am not advocating for or against adding --with-camlibs=everything to the CI workflow here. If anything, I would add DISTCHECK_CONFIGURE_FLAGS=--with-camlibs=everything to the make distcheck command. But this is probably also not worth the CPU cycles.

@ndim ndim force-pushed the fix-parallel-builds branch 2 times, most recently from d26577c to eb63ef9 Compare May 14, 2022 00:56
@ndim ndim marked this pull request as ready for review May 14, 2022 01:09
@msmeissn
Copy link
Contributor

change looks good to me.

@ndim ndim self-assigned this May 19, 2022
@ndim ndim force-pushed the fix-parallel-builds branch from eb63ef9 to 30aad2e Compare May 19, 2022 14:32
ndim added 2 commits May 19, 2022 16:59
Require gettext >= 0.19.1 (from 2014-06-10), which both works properly
when building on many cores, and is also the first gettext release
whose po/Makefile.in.in allows disabling rebuilding of the *.po files
on normal (non dist target) builds.

The new po/Makefile.in.in rules fix "make distcheck" consistently
failing to remove ../../../po/libgphoto2-6.pot when running
"make -j$(nproc) distcheck" on a 12 core system:

    rm: cannot remove '../../../po/libgphoto2-6.pot': Permission denied

The "make dist" (and "make distcheck") targets will still update the
po files by default, but that could also be disabled in po/Makevars
if we wanted to separate updating the *.po files for translation from
"make dist" or "make distcheck" at any time in the future.

As gettext-0.19.1 was released almost 8 years ago, this is the newest
build tool release requirement for bulding libgphoto2.

If you ever need to build a 2022 or later libgphoto2 from git on a
system with pre 0.19.1 gettext, you can can always prepare a tarball
running "make dist" on a system with gettext 0.19.1 or later, and
build that tarball on the older machine.
Use parallel make for builds. We use the GNU coreutils'
"nproc" utility to determine the number of cores available
for us to use. We then tell make to launch up to that number
of processes (-j$(nproc)) in parallel, but only if the system
load is below $(nproc) (-l$(nproc)).

That should make reasonably good use of however many cores github
makes available to us, while not overloading the build server
or the build server VM.
@ndim ndim force-pushed the fix-parallel-builds branch from 30aad2e to d1650f1 Compare May 19, 2022 15:00
@ndim ndim merged commit d1650f1 into gphoto:master May 19, 2022
@ndim ndim deleted the fix-parallel-builds branch May 19, 2022 15:01
@ndim ndim restored the fix-parallel-builds branch May 19, 2022 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants