Skip to content
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

build system improvements #1750

Merged
merged 162 commits into from
May 29, 2024

Conversation

benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Mar 25, 2024

Rework the build system to make it faster and better.

TODO:

  • PR fbink build system patches
  • resolve libk2pfopt situation
  • [ ] update docker images (chrpath+meson)

Highlights:

  • move most of it to CMake: allow for parallelize each external project build
    steps as much as possible (e.g. all download / patch steps can be executed in
    parallel).
  • add convenience targets for building koreader libraries and each external project
    build steps (e.g.: make koreader-cre to build only libkoreader-cre.so and all
    its dependencies, make leptonica-re to rebuild leptonica, etc…). Top level targets
    can be serialized and are forwarded to CMake (so its possible to do make clean zlib
    for example).
  • change the build tree layout: keep the downloads in thirdparty/*/build/…,
    move the external projects build directories under the machine specific build
    directory: reduce the depth of the build tree, and simplify clean rules.
  • “install” all external projects to the same staging directory under the build
    directory: simplify dependency lookups in external projects (including CMake).
  • always use a CMake toolchain file: including when not cross-compiling, as way
    to pass parameters to CMake and simplify building CMake based external projects.
  • use CMake to build external projects when possible (faster).
  • use Ninja as default generator (faster).
  • add missing luajson dependency, don't use luarocks for building/installing LUA
    external projects: faster builds, no need for network, all dependencies explicitly
    accounted for, simpler install tree (no need for the rocks directory anymore).
  • use -l as as well as -j when running make / ninja to manage system load.
  • rework libk2pdfopt / leptonica / tesseract situation: leptonica & tesseract are
    build by their respective external projects.
  • changing an external project source file will automatically trigger its build step
    during the next build.

Build times (on my machine: -j8 / -j8 -l8, from scratch: after make distclean):

master PR
emulator-debug (from scratch) 11:05 4:30 (÷ 2.5)
emulator-debug (from cache) 4:04 0:38 (÷ 6.4)
android-arm (from scratch) 9:58 4:27 (÷ 2.2)
android-arm (from cache) 3:44 0:34 (÷ 6.6)
kindlepw2 (from scratch) 15:50 6:51 (÷ 2.3)
kindlepw2 (from cache) 5:21 0:50 (÷ 6.4)

Disk usage (downloads + build tree(s))

master PR
emulator-debug 291M + 2.3G = 2.6G 155M + 1.6G = 1.7G
android-arm 286M + 1.4G = 1.6G 150M + 1.0G = 1.2G
kindlepw2 326M + 1.5G = 1.9G 176M + 1.2G = 1.3G

Notes:

  • minimum supported CMake version 3.15 (already mandated by zsync2's cpr submodule).
  • using make as a CMake generator is still supported (with some limitations: no proper build by-products support), but since meson does not support it, I think we might as well drop support for it.
  • need this patch to the top koreader project:
 Makefile | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git i/Makefile w/Makefile
index e3092b174..bdd6bfcaf 100644
--- i/Makefile
+++ w/Makefile
@@ -77,7 +77,7 @@ DOCKER:=$(shell grep -q docker /proc/1/cgroup 2>/dev/null && echo 1)
 INSTALL_FILES=reader.lua setupkoenv.lua frontend resources defaults.lua datastorage.lua \
 		l10n tools README.md COPYING
 
-all: $(if $(ANDROID),,$(KOR_BASE)/$(OUTPUT_DIR)/luajit)
+all:
 	$(MAKE) -C $(KOR_BASE)
 	install -d $(INSTALL_DIR)/koreader
 	rm -f $(INSTALL_DIR)/koreader/git-rev; echo "$(VERSION)" > $(INSTALL_DIR)/koreader/git-rev
@@ -85,12 +85,12 @@ ifdef ANDROID
 	rm -f android-fdroid-version; echo -e "$(ANDROID_NAME)\n$(ANDROID_VERSION)" > koreader-android-fdroid-latest
 endif
 ifeq ($(IS_RELEASE),1)
-	$(RCP) -fL $(KOR_BASE)/$(OUTPUT_DIR)/. $(INSTALL_DIR)/koreader/.
+	bash -O extglob -c '$(RCP) -fL $(KOR_BASE)/$(OUTPUT_DIR)/!(cache|cmake|history|staging|thirdparty) $(INSTALL_DIR)/koreader/'
 else
 	cp -f $(KOR_BASE)/ev_replay.py $(INSTALL_DIR)/koreader/
 	@echo "[*] create symlink instead of copying files in development mode"
 	cd $(INSTALL_DIR)/koreader && \
-		bash -O extglob -c "ln -sf ../../$(KOR_BASE)/$(OUTPUT_DIR)/!(cache|history) ."
+		bash -O extglob -c 'ln -sf ../../$(KOR_BASE)/$(OUTPUT_DIR)/!(cache|cmake|history|staging|thirdparty) .'
 	@echo "[*] install front spec only for the emulator"
 	cd $(INSTALL_DIR)/koreader/spec && test -e front || \
 		ln -sf ../../../../spec ./front
@@ -130,9 +130,6 @@ ifeq ($(IS_RELEASE),1)
 	rm -rf $(INSTALL_DIR)/koreader/data/{cr3.ini,cr3skin-format.txt,desktop,devices,manual}
 endif
 
-$(KOR_BASE)/$(OUTPUT_DIR)/luajit:
-	$(MAKE) -C $(KOR_BASE)
-
 $(INSTALL_DIR)/koreader/.busted: .busted
 	ln -sf ../../.busted $(INSTALL_DIR)/koreader
 
@@ -143,7 +140,9 @@ $(INSTALL_DIR)/koreader/.luacov:
 testfront: $(INSTALL_DIR)/koreader/.busted
 	# sdr files may have unexpected impact on unit testing
 	-rm -rf spec/unit/data/*.sdr
-	cd $(INSTALL_DIR)/koreader && ./luajit $(shell which busted) \
+	eval "$$($(LUAROCKS_BINARY) path)" && cd $(INSTALL_DIR)/koreader && \
+		env TESSDATA_DIR=$(INSTALL_DIR)/koreader/data \
+		./luajit "$$(which busted)" \
 		--sort-files \
 		--output=gtest \
 		--exclude-tags=notest $(BUSTED_OVERRIDES) $(BUSTED_SPEC_FILE)
@@ -629,4 +628,5 @@ static-check:
 doc:
 	make -C doc
 
+.NOTPARALLEL:
 .PHONY: all clean doc test update

This change is Reviewable

@benoit-pierre
Copy link
Contributor Author

For libk2pdfopt, I think the external project should switch to the official source archive + an overlay directory (similarly to how lunarsvg is handled). That's how I handle it on my meson branch too.

@benoit-pierre
Copy link
Contributor Author

Right, emulator builds don't go through docker, so meson is missing…

@benoit-pierre benoit-pierre marked this pull request as draft March 25, 2024 03:15
@Frenzie
Copy link
Member

Frenzie commented Mar 25, 2024

fixup commits aren't supported by GH rebase btw (unless they added support in the last few months), so for that a force push will be required.

Edit: at a glance this looks great, but I don't know if I'll have time to look at it properly this week. Hopefully I'll find the time to give it a quick try tonight though. :-)

@poire-z
Copy link
Contributor

poire-z commented Mar 25, 2024

Not competent and experienced enough to judge.
I just hit n180 times, and it didn't look to scary (to read, but real scary to write and test).

Some comments and questions:

40b323b leptonica: are these new patches ? Taken from libkopt2pdf project? Is there some history?

934627b zlib: drop of patches intentional?

(I've seen here and there some added comments that sounded (wording, vocabulary) like @NiLuJe 's :) I've seen some of these removed from other commits. You did just moved things around? Or are you beginning to sound like @NiLuJe ? :))

About using tarball and specifying URL_MD5 : will we need to download the tarball and md5sum it ourselves?

733bbbb target_include_directories(crengine PUBLIC vs. PRIVATE) Why/when to use PUBLIC and when PRIVATE ?

8ff3898 add soname to our helper libraries. Just curious, what does this bring ? (I guess this won't get us libkoreader-cre.so.0, it just adds some string in the lib itself.)

ae75343 5919035 any reason for the reordering of $(LIBXYZ), or just for balance of the text?

ae75343 some ellipsis ..., dunno if we want to keep all that ASCII

043d3c9 So, what's our official policy ? Do we love CMake or not ? :)
(At least I can understand a bit of Make - CMake has a lot of new vocabulary and needs us to read docs. But anyway, not caring, I'll continue to just copy and paste from existing stuff :))
Having to list all the headers in the BYPRODUCTS stuff feels ugly and like a step back ?

8499524 libiconv: only compile on Android & macOS. On other platforms, rely on glibc' support.
Just highlighting this to be sure @Frenzie @NiLuJe agree.

669045d So, this moves some bits to (and requires?) meson.
Just because these projects propose building with meson?
Or is it a first step to a better meson-only world?
And why, these PR being about reducing diversity and moving everything to CMake, re-add some diversity by needing meson for some bits (I assume these project don't require meson, as we used to build them without).

And asking again #1697 (comment) :

Also, why, while happy with your meson branch, would you spend that much time in CMake hell, fixing/improving/tidying all that ?!

Morbid curiosity? Refreshing my make-fu, confirming my feelings on CMake (bleh).

Why all the pain of this PR?
Will you keep having your own full-meson branch?
Is this PR just a first step into going full-meson?
Are you convinced we should here stick to CMake?
Are you convinced you won't be able to convince us to switch to meson?
What's the future? :)

For libk2pdfopt, I think the external project should switch to the official source archive + an overlay directory (similarly to how lunarsvg is handled). That's how I handle it on my meson branch too.

I must say it's quite painful to update LunaSVG. I have a local git clone of the upstream repo (not pushed anywhere) where I keep my history of changes, and just upload the extended.patch here. I'm not certain it's the best way to handle that.

@Frenzie
Copy link
Member

Frenzie commented Mar 25, 2024

8499524 libiconv: only compile on Android & macOS. On other platforms, rely on glibc' support.
Just highlighting this to be sure @Frenzie @NiLuJe agree.

I believe that's simply a port of the current behavior, not a behavioral change.

669045d So, this moves some bits to (and requires?) meson.
Just because these projects propose building with meson?
Or is it a first step to a better meson-only world?

The projects themselves have introduced meson. (I didn't double check but I know at least some of them did.)

@benoit-pierre
Copy link
Contributor Author

40b323b leptonica: are these new patches ? Taken from libkopt2pdf project? Is there some history?

I just extracted the changes to leptonica from the version of k2pdfopt being used. No relevant changes to tesseract (only patches for old Windows versions, e.g. XP, or debug traces).

934627b zlib: drop of patches intentional?

Yes: those are autotools specific.

(I've seen here and there some added comments that sounded (wording, vocabulary) like @NiLuJe 's :) I've seen some of these removed from other commits. You did just moved things around?

Some.

Or are you beginning to sound like @NiLuJe ? :))

Probably for the same reasons: PTSD from to much CMake :P

About using tarball and specifying URL_MD5 : will we need to download the tarball and md5sum it ourselves?

You have to fill-in the md5sum manually, yes. Note however that it's optional, so you can:

  • update the version
  • comment the URL_MD5 argument
  • re-download (e.g.: make zlib-{clean,download})
  • and then update the MD5

733bbbb target_include_directories(crengine PUBLIC vs. PRIVATE) Why/when to use PUBLIC and when PRIVATE ?

  • PRIVATE: only used when compiling the library
  • PUBLIC: also needed by users of said library

8ff3898 add soname to our helper libraries. Just curious, what does this bring ? (I guess this won't get us libkoreader-cre.so.0, it just adds some string in the lib itself.)

CMake automatically adds it, to this was done to reduce differences with the final version (produce the same binaries as much as possible to avoid regressions, or at least the same readelf -n output).

ae753435919035 any reason for the reordering of $(LIBXYZ), or just for balance of the text?

Same as above.

ae75343 some ellipsis ..., dunno if we want to keep all that ASCII

Wrong commit?

043d3c9 So, what's our official policy ? Do we love CMake or not ? :)

I certainly don't, but it still better than make or autotools (for some definition of better).

(At least I can understand a bit of Make - CMake has a lot of new vocabulary and needs us to read docs. But anyway, not caring, I'll continue to just copy and paste from existing stuff :))

Yeah, it can be hell. But to be fair, the current build system is not doing itself any favor by installing each external project to a dedicated directory, instead of a common staging directory that can be more easily searched by CMake. That simplify things a lot. Even if you still have to contend with the numerous ways a CMake project has to find particular dependency (and the potential differences between CMake versions, as in how the initial call to cmake_minimum_required(…) can influence things).

@benoit-pierre
Copy link
Contributor Author

8499524 libiconv: only compile on Android & macOS. On other platforms, rely on glibc' support.
Just highlighting this to be sure @Frenzie @NiLuJe agree.

I believe that's simply a port of the current behavior, not a behavioral change.

No, it's a change. Same with not compiling gettext, and then relying on proxy-libintl instead. On my meson branch, I always use proxy-libintl (because we don't need i18n support in sdcv). I also switched from libiconv to miniconv, as FWIU, we don't really need full iconv support, if even, with the patches to sdcv (UTF-8 is always used).

669045d So, this moves some bits to (and requires?) meson.
Just because these projects propose building with meson?
Or is it a first step to a better meson-only world?

The projects themselves have introduced meson. (I didn't double check but I know at least some of them did.)

Yes, meson is the default build system of those projects.

@benoit-pierre
Copy link
Contributor Author

For libk2pdfopt, I think the external project should switch to the official source archive + an overlay directory (similarly to how lunarsvg is handled). That's how I handle it on my meson branch too.

I must say it's quite painful to update LunaSVG. I have a local git clone of the upstream repo (not pushed anywhere) where I keep my history of changes, and just upload the extended.patch here. I'm not certain it's the best way to handle that.

Right, in lunasvg's case, there's some patching to the source involved, but not for libk2pdfopt: the overlay is independent. Of course you still need to update the leptonica / tesseract patches (if any), and the overlay code itself.

@benoit-pierre
Copy link
Contributor Author

Having to list all the headers in the BYPRODUCTS stuff feels ugly and like a step back ?

I don't like it either, but it's required for proper dependencies handling (particularly on incremental parallel builds).

And there's no dependency file equivalent for by-products.

Initially, I had setup every external project to always build (see BUILD_ALWAYS documentation). This was done so changes to an external project sources would get picked up automatically. But this resulted in a slow no-change make (about ~2 seconds): because with BUILD_ALWAYS, the build and external step of each external project is always executed, and the build steps are never run in parallel (they are the ninja console pool: max 1 job).

But thanks to the sources depfile trick, there's no need for BUILD_ALWAYS, a build step will automatically be triggered on source change.

And there might be a way to avoid the need for using by-products by moving the crengine library, koreader libraries, and miscellaneous executables builds to dedicated external projects. I need to investigate.

@benoit-pierre
Copy link
Contributor Author

I also need someone with access to macOS to test. I checked the resulting executable/libraries have the right runtime path / soname information, and that's it. I think all the install_name calls in do_mac_bundle.sh are not necessary anymore. Same with building the gettext and libsodium libraries from brew.

@poire-z
Copy link
Contributor

poire-z commented Mar 26, 2024

ae75343 some ellipsis ..., dunno if we want to keep all that ASCII

Wrong commit?

Indeed :/ Can't find it, and not looking again at these 180 commits :)
Anyway, there are four new and 1 already there moved around, probably from some @NiLuJe comment. So, well, not minding, just mentionning it in case someone else cares.

@Frenzie
Copy link
Member

Frenzie commented Mar 26, 2024

No, it's a change. Same with not compiling gettext, and then relying on proxy-libintl instead. On my meson branch, I always use proxy-libintl (because we don't need i18n support in sdcv). I also switched from libiconv to miniconv, as FWIU, we don't really need full iconv support, if even, with the patches to sdcv (UTF-8 is always used).

Once upon a time we had a GLIB (without libiconv and gettext, except on MacOS) and a GLIB_STATIC (for Android, with libiconv). If that's no longer the case it would require investigation into why that was changed. At a glance it looks like the relevant conditions were dropped in #1690, meaning that was the change in behavior, not this, imho.

@benoit-pierre benoit-pierre force-pushed the pr/build_system_improvements branch 2 times, most recently from 17df59c to 2bec084 Compare April 1, 2024 07:51
@Frenzie
Copy link
Member

Frenzie commented Apr 1, 2024

  • update docker images (chrpath+meson)

I assume that for chrpath anything will do. That is, it's been at 0.16 for years, unless the new 0.17 offers something we need. But is the same true for Meson?

@benoit-pierre
Copy link
Contributor Author

Those projects that use meson are usually pretty conservative (same with WrapDB):

▹ grep meson_version build/x86_64-pc-linux-gnu-debug/cmake/*/source/meson.build
build/x86_64-pc-linux-gnu-debug/cmake/fribidi/source/meson.build
2:  meson_version : '>= 0.54')

build/x86_64-pc-linux-gnu-debug/cmake/freetype2/source/meson.build
27:  meson_version: '>= 0.55.0',

build/x86_64-pc-linux-gnu-debug/cmake/harfbuzz/source/meson.build
2:  meson_version: '>= 0.55.0',

build/x86_64-pc-linux-gnu-debug/cmake/proxy-libintl/source/meson.build
3:  meson_version : '>= 0.46.0',

build/x86_64-pc-linux-gnu-debug/cmake/glib/source/meson.build
3:  meson_version : '>= 0.47.0',

@benoit-pierre
Copy link
Contributor Author

benoit-pierre commented Apr 1, 2024

I assume that for chrpath anything will do.

With that last commit, it's actually not needed anymore: no need to list all build by-products for each external projects anymore, each project actual install rules are used. I moved the crengine build to a dedicated external project. Same with the koreader libraries / executable. (But I kept support for things like make koreader-cre to only build libkoreader-cre and its dependencies).

chrpath was only needed to cleanup meson build runtime path before manually "installing" to staging, that's unnecessary now that meson install rules are used.

I'll have to check if the macOS runtime path is still correct however: from past experience with my meson branch, custom runtime path entries are stripped on install.

@Frenzie
Copy link
Member

Frenzie commented Apr 1, 2024

pretty conservative

Not conservative enough for the Ubuntu 20.04 repos. :-) But perhaps the time is right to switch to 22.04, if Android wants to participate.

@benoit-pierre
Copy link
Contributor Author

We can always install from pip, even if that will not always give us the last version (e.g. the kindle image uses Python 3.6, so meson 0.61.5 is the latest supported version).

@Frenzie
Copy link
Member

Frenzie commented Apr 1, 2024

Of course we can, but I'd like to avoid that if we don't have to.

@benoit-pierre
Copy link
Contributor Author

What about the appimage? What's the oldest target?

@Frenzie
Copy link
Member

Frenzie commented Apr 1, 2024

(e.g. the kindle image uses Python 3.6, so meson 0.61.5 is the latest supported version).

This is close to the same statement ftr. I want them on 20.04 (or maybe 22.04) but the Android build mysteriously crashes in CI. We have yet to test if the update from a while back solves that though. The 0.3.0 image (with 20.04) has been available for immediate use for all but Android for a year. ^_^

@benoit-pierre
Copy link
Contributor Author

benoit-pierre commented Apr 1, 2024

OK. One thing I'd like to PR, is the test suite improvements I made on my meson branch: isolating tests, and then using meson as a test runner (each spec file run is isolated to its own process, tests run in parallel).

It's much faster, and you don't have to work around the lack of real isolation in busted.

@Frenzie
Copy link
Member

Frenzie commented Apr 1, 2024

What about the appimage? What's the oldest target?

That's basically what I was referring to above by saying maybe the time has come for 22.04. For anything else it really doesn't matter all that much, but I consider lower versions (when reasonable) more advantageous since they ensure it's easy for people to start hacking. Keeping them in line with the AppImage is two birds one stone.

@benoit-pierre
Copy link
Contributor Author

Ubuntu 20.04 has meson 0.53.2, 22.04: 0.61.2.

@Frenzie
Copy link
Member

Frenzie commented Apr 1, 2024

Right, but FreeType would like 0.55.

@benoit-pierre
Copy link
Contributor Author

Yep, so update to 22.04, or pip install. ¯\(ツ)

Don't build `SDL2main` library (we don't need it).
Ensure concurrent access to the clone directory work as expected
(either from compiling for different platforms in parallel, or
when using the same source for different external projects,
like fbdepth / fbink).
Since it does not rely on MuPDF dependencies.
Fix corner case when using `make curl-download` from scratch.
Add a dedicated `generate` step, rather than use `patch`: allow
for manual patching before actually generating the required files.
Needed for some rare DJVU files using JPEG encoding.
Necessary if ninja is still used by some external project build
systems (e.g. meson) even when using CMake's make generator.
Reduce the toolchain file to its essentials, as setting a `CXX`
variable breaks detection of the C++ compiler supported features
(Cf. https://gitlab.kitware.com/cmake/cmake/-/issues/22125#note_947613).
@benoit-pierre
Copy link
Contributor Author

Welp, one zsync2 bump and we're off to the races I guess? Many thanks for taking on this endeavor!

Done.

@Frenzie
Copy link
Member

Frenzie commented May 29, 2024

@NiLuJe You said you wanted a merge commit?

Copy link
Member

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 102 files at r11, 2 of 34 files at r12, 99 of 99 files at r13, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @benoit-pierre)

@NiLuJe NiLuJe merged commit c7e1f6b into koreader:master May 29, 2024
2 checks passed
@benoit-pierre benoit-pierre deleted the pr/build_system_improvements branch May 29, 2024 17:44
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.

5 participants