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

cre: fix and enable teardown #1617

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Jun 11, 2023

Depends on koreader/crengine#520.


This change is Reviewable

@benoit-pierre benoit-pierre changed the title base/cre: fix and enable teardown cre: fix and enable teardown Jun 11, 2023
@benoit-pierre benoit-pierre marked this pull request as draft June 11, 2023 18:26
@benoit-pierre benoit-pierre force-pushed the pr/fix_cre_teardown branch 2 times, most recently from ac53720 to b6a8cc4 Compare June 11, 2023 19:58
@benoit-pierre
Copy link
Contributor Author

Damn, android ancient toolchain… ;(

@benoit-pierre
Copy link
Contributor Author

So I don't know how to proceed… I updated my meson branch to use the latest android toolchain (Android NDK r25.c, meaning minSdkVersion="14") a long time ago (to be able to update MuPDF). Which means using clang (also supported for building the emulator BTW). I had to patch crengine and build tesseract and libk2pdfopt with -fno-strict-aliasing. I've built and tested for ARM, ARM64, and x86_64.

Android NDK r15c will be 6 years old next month, so maybe it's time to update?

@pazos
Copy link
Member

pazos commented Jun 11, 2023

Android NDK r15c will be 6 years old next month, so maybe it's time to update?

You would make us very happy, specially @NiLuJe :)

So, please go ahead if you will :)
Remember to update LuaJIT build script in https://github.com/koreader/android-luajit-launcher too.

the latest android toolchain (Android NDK r25.c, meaning minSdkVersion="14")

As far as I understand r18 drops ICS support and r24 drops JB support (based on https://developer.android.com/ndk/downloads/revision_history)

We're good with both drops, IMHO.
We're better at r23, preserving JB support. There're a bunch of legacy devices too slow to run any other proper epub/pdf viewer where KO actually makes the difference.

@NiLuJe
Copy link
Member

NiLuJe commented Jun 12, 2023

Yup, I'd be more than okay with that ;p.

@Frenzie
Copy link
Member

Frenzie commented Jun 12, 2023

+1 on r23

Remember to update LuaJIT build script in https://github.com/koreader/android-luajit-launcher too.

And if it's not too much effort also https://github.com/koreader/koreader-base/blob/e3f0c99aaad218c6b26030fbfc229ea5a1e4e887/toolchain/Makefile which we use here: https://github.com/koreader/virdevenv/blob/9052cee45b019180d787d61b6d56b5428f847590/docker/ubuntu/koandroid/fetch_android_tc.sh#L5

Those Android TC scripts have an annoying tendency to change just about every release.

@poire-z
Copy link
Contributor

poire-z commented Jun 19, 2023

Is someone working on the Android toolchain update ? Or expecting someone else to do it ? :)
If nothing will happen in the near future, maybe just remove the constexpr on the crengine side (shouldn't slow it down much, right?), so dev on its side is not blocked.

@NiLuJe
Copy link
Member

NiLuJe commented Jun 19, 2023

As far as I'm concerned, if I ever get to it, that'll be after the Linux TC updates, and that's been somewhat stalled by amdgpu drivers being a mess on my new build, making doing anything productive with it extremely frustrating.

@Frenzie
Copy link
Member

Frenzie commented Jun 20, 2023

I'm certainly not expecting anyone to work on that dumb always pulling the rug out from under you nonsense. ;-)

Anyway, it can be done in steps; it doesn't need to be all at once.

@benoit-pierre
Copy link
Contributor Author

We can't just remove the constexp: it won't compile without it (because of the calls to delete).

As for the NDK update, I'm working on it, current state:

Device / API level v2023.05.1 NDK update monolibtic meson
Simulator (armeabi-v7a):
16 (Jelly Bean) ok crash¹ crash¹
17 (Jelly Bean MR1) ok crash² ok
18 (Jelly Bean MR2) crash² crash² ok
19 (KitKat) crash³ crash³ crash³
21 (Lollipop) ?⁴ ?⁴ ?⁴
22 (Lollipop MR1) ?⁴ ?⁴ ?⁴
23 (Marshmallow) ok ok ok
Simulator (x86):
16 (Jelly Bean) ok broken⁵ broken⁵
17 (Jelly Bean MR1) ok ok ok
18 (Jelly Bean MR2) ok ok ok
19 (KitKat) crash³ crash³ crash³
21 (Lollipop) ok ok ok
22 (Lollipop MR1) ok ok ok
23 (Marshmallow) ok ok ok
Nexus 7 (2013, ARM) / 30 (R) ok ok ok
Moto E6+ (ARM64) / 28 (P) ok ok ok

Notes:

  • monolibtic mode is the only way I can get a fully working meson build on API < 23, without hitting the system dynamic linker bugs or bugs in our android.dl resolver
  • dictionary install is broken on API < 18 due to missing support for java.nio.charset.StandardCharsets (used by org.apache.commons.compress)

Are we okay with dropping support for API < 17, or even < 18?

@NiLuJe
Copy link
Member

NiLuJe commented Jun 22, 2023

I'd be mildly interested in logs from the dl related stuff, for science!

@benoit-pierre
Copy link
Contributor Author

@NiLuJe: here

Includes the output of readelf -d on all libraries, and the logs for:

  • API 23: no issue
  • API 21: starts, but fails to load common/ssl.so
  • API 18: fails to load libmupdf during startup

The sequence of calls to dlopen(…) is the same in all 3 cases.

@NiLuJe
Copy link
Member

NiLuJe commented Jun 22, 2023

Hmm, fun, thanks!

  • API 18: Assuming there are no actual symbol visibility issues in the produced binaries, my best guess for the mupdf/hb thing would be the loader doing dumb things because of the circular HB <-> FT dependency. No clue if there's a sane way to handle that (given how broken that version of the Android loader is); the only thing I can think of to try would be force-loading harfbuzz before freetype.
  • API 21: No idea about that one, the ordering looks sound.

Possibly namespace issues? I can't recall why I kept using RTLD_LOCAL by default instead of RTLD_GLOBAL in there... (Possibly to mimic ffi.load's native behavior, which defaults to local (which also happens to be dlopen's default). Interestingly enough, require defaults to global instead...). (On a related note, ffi.load requests lazy symbol resolution, unlike require).

@pazos
Copy link
Member

pazos commented Jun 23, 2023

Are we okay with dropping support for API < 17, or even < 18?

in exchange for a modern toolchain? Hell yes! :)

@Frenzie
Copy link
Member

Frenzie commented Jun 23, 2023

Yeah, fine by me.

@NiLuJe
Copy link
Member

NiLuJe commented Jun 23, 2023

What they said ;).

@benoit-pierre
Copy link
Contributor Author

* API 18: Assuming there are no actual symbol visibility issues in the produced binaries, my best guess for the mupdf/hb thing would be the loader doing dumb things because of the circular HB <-> FT dependency. No clue if there's a sane way to handle that (given how broken that version of the Android loader is); the only thing I can think of to try would be force-loading harfbuzz before freetype.

But there's no inter-dependency, since FreeType is compiled without HarfBuzz support.

I've managed to check the build works on KitKat after reflashing an old Motorola smartphone (except for networking features, as WIFI wasn't working).

dictionary install is broken on API < 18 due to missing support for java.nio.charset.StandardCharsets (used by org.apache.commons.compress)

I should have written API <= 18, or < 19.

@NiLuJe
Copy link
Member

NiLuJe commented Jun 23, 2023

But there's no inter-dependency, since FreeType is compiled without HarfBuzz support.

Oops, yeah; I mixed up mupdf's DT_NEEDED entries with freetype's ;).

@poire-z
Copy link
Contributor

poire-z commented Jun 25, 2023

Just mentionning this (dunno if you have made a tweak somewhere in your numerous PRs about these std=c++17), when compiling for ARM with some older debian10's arm-linux-gnueabihf-gcc-8, I get these warnings:

lvhashtable.h: In member function 'void LVHashTable<keyT, valueT, own_values>::clear()':
lvhashtable.h:117:20: warning: 'if constexpr' only available with -std=c++17 or -std=gnu++17
                 if constexpr (own_values)

@benoit-pierre
Copy link
Contributor Author

And with this patch:

 thirdparty/kpvcrlib/CMakeLists.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git i/thirdparty/kpvcrlib/CMakeLists.txt w/thirdparty/kpvcrlib/CMakeLists.txt
index 4b28fe4b..4e806675 100644
--- i/thirdparty/kpvcrlib/CMakeLists.txt
+++ w/thirdparty/kpvcrlib/CMakeLists.txt
@@ -202,8 +202,8 @@ set (CRENGINE_SOURCES
     ${CRE_DIR}/src/crconcurrent.cpp
 )
 add_library(crengine SHARED ${CRENGINE_SOURCES})
-# Make sure we'll get C++11 support
-target_compile_features(crengine PRIVATE cxx_constexpr)
+# Make sure we get full `constexpr` support.
+target_compile_features(crengine PRIVATE cxx_std_17)
 
 target_link_libraries(crengine PRIVATE chmlib antiword)
 target_link_libraries(crengine PRIVATE ${THIRDPARTY_LIBS})

?

@poire-z
Copy link
Contributor

poire-z commented Jun 25, 2023

^ yep, this makes these warnings disappear on my side.
At least for the libcrengine.so build. I still get them when building libkoreader-cre.so from cre.cpp.

@benoit-pierre
Copy link
Contributor Author

@poire-z : can you successfully build with this patch?

 Makefile.defs                         | 32 +++++---------------------------
 thirdparty/djvulibre/CMakeLists.txt   |  4 ++++
 thirdparty/kpvcrlib/CMakeLists.txt    |  4 ++--
 thirdparty/libk2pdfopt/CMakeLists.txt |  4 ++++
 4 files changed, 15 insertions(+), 29 deletions(-)

diff --git i/Makefile.defs w/Makefile.defs
index f1ac159b..d4d18410 100644
--- i/Makefile.defs
+++ w/Makefile.defs
@@ -518,33 +518,11 @@ ifdef DARWIN_AARCH64_HOST
 	endif
 endif
 
-# Anything below GCC 6 gets to be forcefully switched to decent standards, because some of our deps require C11 & C++11 support.
-# NOTE: Technically, this also means we require Clang >= 3.2, but that should more or less hold true on our end, because that's truly ancient.
-# Tests heavily inspired from Linux's build system ;).
-CC_IS_CLANG:=$(shell PATH='$(PATH)' $(CC) -v 2>&1 | grep -q "clang version" && echo 1 || echo 0)
-CC_VERSION:=$(shell PATH='$(PATH)' printf "%02d%02d%02d" `echo __GNUC__ | $(CC) -E -x c - | tail -n 1` `echo __GNUC_MINOR__ | $(CC) -E -x c - | tail -n 1` `echo __GNUC_PATCHLEVEL__ | $(CC) -E -x c - | tail -n 1`)
-# Detect Clang's SA, too...
-ifeq "$(CC_IS_CLANG)" "0"
-	ifeq "$(lastword $(subst /, ,$(CC)))" "ccc-analyzer"
-		CC_IS_CLANG:=1
-	endif
-endif
-ifeq "$(CC_IS_CLANG)" "0"
-	# NOTE: GCC 6.1.0 switched to gnu11 & gnu++14, so only tweak older ones.
-	#       c.f., https://stackoverflow.com/q/14737104 for a nice recap.
-	ifeq "$(shell expr $(CC_VERSION) \< 060100)" "1"
-		CSTD_FLAGS:=-std=gnu11
-		CXXSTD_FLAGS:=-std=gnu++11
-	endif
-else
-	# NOTE: Enforce the current default everywhere, otherwise XCode does stupid things on Darwin.
-	#       c.f., https://clang.llvm.org/docs/CommandGuide/clang.html#language-selection-and-mode-options
-	#          &  https://github.com/llvm/llvm-project/blob/1c4b5a8dfed853ca8cbee98f274ff0a173186586/clang/lib/Frontend/CompilerInvocation.cpp#L2136-L2164
-	#       FWIW, the C++ move to gnu++14 by default dates back to Clang 6:
-	#       c.f., https://github.com/llvm-mirror/clang/commit/466d8da5f89b1a780f735c86f414fa69ce63221b
-	CSTD_FLAGS:=-std=gnu11
-	CXXSTD_FLAGS:=-std=gnu++14
-endif
+# Some of our deps require C11 & C++17 support.
+# NOTE: Technically, this also means we require GCC >= 8.0 (2018) or Clang >= 5.0.0 (2017),
+getstd = $(or $(shell $1 -x $2 -E -dM /dev/null | sed -ne 's/#define $3 \([0-9]\+\)L$$/\1/p'),0)
+CSTD_FLAGS := $(intcmp $(call getstd,$(CC),c,__STDC_VERSION__),201100,-std=gnu11)
+CXXSTD_FLAGS := $(intcmp $(call getstd,$(CXX),c++,__cplusplus),201700,-std=gnu++17)
 
 HOSTCFLAGS:=$(HOST_ARCH) $(BASE_CFLAGS) $(QFLAGS)
 
diff --git i/thirdparty/djvulibre/CMakeLists.txt w/thirdparty/djvulibre/CMakeLists.txt
index bfef6c12..edb3d960 100644
--- i/thirdparty/djvulibre/CMakeLists.txt
+++ w/thirdparty/djvulibre/CMakeLists.txt
@@ -19,6 +19,10 @@ ep_get_source_dir(SOURCE_DIR)
 # fix build error due to -Werror under Fedora 26 (and potentially other systems)
 set(CFLAGS "${CFLAGS} -Wno-error")
 
+# Fix build error when compiling with -std=gnu++17: ISO
+# C++17 does not allow 'register' storage class specifier.
+set(CXXFLAGS "${CXXFLAGS} -Wno-error=register")
+
 set(CFG_ENV_VAR "CC=\"${CC}\" CXX=\"${CXX}\" CFLAGS=\"${CFLAGS}\" CXXFLAGS=\"${CXXFLAGS}\" LDFLAGS=\"${LDFLAGS}\"")
 set(CFG_OPTS "-q --disable-desktopfiles --disable-static --enable-shared --disable-xmltools --disable-largefile --without-jpeg --without-tiff -host=\"${CHOST}\"")
 set(CFG_CMD1 sh -c "${CFG_ENV_VAR} ${SOURCE_DIR}/configure ${CFG_OPTS}")
diff --git i/thirdparty/kpvcrlib/CMakeLists.txt w/thirdparty/kpvcrlib/CMakeLists.txt
index af79afd3..de7a4357 100644
--- i/thirdparty/kpvcrlib/CMakeLists.txt
+++ w/thirdparty/kpvcrlib/CMakeLists.txt
@@ -201,8 +201,8 @@ set (CRENGINE_SOURCES
     ${CRE_DIR}/src/crconcurrent.cpp
 )
 add_library(crengine SHARED ${CRENGINE_SOURCES})
-# Make sure we'll get C++11 support
-target_compile_features(crengine PRIVATE cxx_constexpr)
+# Make sure we get full `constexpr` support.
+target_compile_features(crengine PRIVATE cxx_std_17)
 
 target_link_libraries(crengine PRIVATE chmlib antiword)
 target_link_libraries(crengine PRIVATE ${THIRDPARTY_LIBS})
diff --git i/thirdparty/libk2pdfopt/CMakeLists.txt w/thirdparty/libk2pdfopt/CMakeLists.txt
index 21df1b5b..dda89b4c 100644
--- i/thirdparty/libk2pdfopt/CMakeLists.txt
+++ w/thirdparty/libk2pdfopt/CMakeLists.txt
@@ -25,6 +25,10 @@ if ($ENV{DARWIN})
     set(CFLAGS "${CFLAGS} -Wno-error=implicit-function-declaration")
 endif()
 
+# Fix build error when compiling with -std=gnu++17: ISO
+# C++17 does not allow 'register' storage class specifier.
+set(CXXFLAGS "${CXXFLAGS} -Wno-error=register")
+
 ep_get_source_dir(SOURCE_DIR)
 
 set(BUILD_CMD sh -c "${KO_MAKE_RECURSIVE} -j${PARALLEL_JOBS} BUILDMODE=shared HOST=${HOST} CC=\"${CC}\" CFLAGS=\"${CFLAGS} -O3\"")

@poire-z
Copy link
Contributor

poire-z commented Jun 25, 2023

The build went fine, with gcc-12 x64 on Debian12, and with debian10's arm-linux-gnueabihf-gcc-8 on this same debian12 machine.
I just built libcrengine.so libkoreader-cre.so libdjvulibre.so.21 libk2pdfopt.so.2 liblept.so.5 libtesseract.so.3 (too hot today to build the whole stuff).
Copying them on my Kobo, I could open an EPUB, but not a PDF and neither a DJVU:

06/25/23-23:46:50 INFO  opening file blah.pdf
ffi.load (warning): /lib/libm.so.6: version `GLIBC_2.27' not found (required by libs/liblept.so.5)
06/25/23-23:46:51 WARN  cannot open document wblah.pdf ./setupkoenv.lua:30: Not able to load dynamic library: libs/liblept.so.5
06/25/23-23:47:01 WARN  cannot open document djvu3spec.djvu error loading module 'libs/libkoreader-djvu' from file './libs/libkoreader-djvu.so':
    /lib/libm.so.6: version `GLIBC_2.27' not found (required by /mnt/onboard/.adds/koreader/./libs/libdjvulibre.so.21)
06/25/23-23:47:03 INFO  opening file djvu3spec.djvu
06/25/23-23:47:03 WARN  cannot open document djvu3spec.djvu error loading module 'libs/libkoreader-djvu' from file './libs/libkoreader-djvu.so':
    /lib/libm.so.6: version `GLIBC_2.27' not found (required by /mnt/onboard/.adds/koreader/./libs/libdjvulibre.so.21)

Not sure why that :/
I think (I may be wrong) that one of the reason I had to stay with tha old gcc-8 and stuff from debian 10 was maybe to get something that works on that old Kobo system.

@NiLuJe
Copy link
Member

NiLuJe commented Jun 25, 2023 via email

@NiLuJe
Copy link
Member

NiLuJe commented Jun 25, 2023 via email

@poire-z
Copy link
Contributor

poire-z commented Jun 25, 2023

Well, dunno them why libcrengine is ok, doesn't it need/use glibc ?
Also, rm-linux-gnueabihf/bin/readelf -a libdjvulibre.so.21 shows hundred of mentions of GLIBC_2.4, and only a few of GLIBC_2.27 (for some logf() ?!!)

00119300  00008916 R_ARM_JUMP_SLOT   00000000   logf@GLIBC_2.27
   137: 00000000     0 FUNC    GLOBAL DEFAULT  UND logf@GLIBC_2.27 (11)
  7867: 00000000     0 FUNC    GLOBAL DEFAULT  UND logf@@GLIBC_2.27
  088:   3 (GLIBC_2.4)     b (GLIBC_2.27)    3 (GLIBC_2.4)     3 (GLIBC_2.4)

Version needs section '.gnu.version_r' contains 6 entries:
 Addr: 0x000000000002eb74  Offset: 0x02eb74  Link: 4 (.dynstr)
  000000: Version: 1  File: ld-linux-armhf.so.3  Cnt: 1
  0x0010:   Name: GLIBC_2.4  Flags: none  Version: 12
  0x0020: Version: 1  File: libgcc_s.so.1  Cnt: 1
  0x0030:   Name: GCC_3.5  Flags: none  Version: 8
  0x0040: Version: 1  File: libm.so.6  Cnt: 2
  0x0050:   Name: GLIBC_2.27  Flags: none  Version: 11
  0x0060:   Name: GLIBC_2.4  Flags: none  Version: 6
  0x0070: Version: 1  File: libstdc++.so.6  Cnt: 5
  0x0080:   Name: CXXABI_1.3.8  Flags: none  Version: 10
  0x0090:   Name: CXXABI_1.3.9  Flags: none  Version: 9
  0x00a0:   Name: CXXABI_1.3  Flags: none  Version: 7
  0x00b0:   Name: CXXABI_ARM_1.3.3  Flags: none  Version: 5
  0x00c0:   Name: GLIBCXX_3.4  Flags: none  Version: 4
  0x00d0: Version: 1  File: libc.so.6  Cnt: 1
  0x00e0:   Name: GLIBC_2.4  Flags: none  Version: 3
  0x00f0: Version: 1  File: libpthread.so.0  Cnt: 1
  0x0100:   Name: GLIBC_2.4  Flags: none  Version: 2

Fwiw, my "keep" of arm stuff from debian 10 is just an extraction of these packages in a dedicated directory (exporting some of them via LD_LIBRARY_PATH C_INCLUDE_PATH PATH when I do an ARM build):

        binutils-arm-linux-gnueabihf
        cpp-8-arm-linux-gnueabihf
        cpp-arm-linux-gnueabihf
        g++-8-arm-linux-gnueabihf
        g++-arm-linux-gnueabihf
        gcc-8-arm-linux-gnueabihf
        gcc-8-arm-linux-gnueabihf-base
        gcc-8-cross-base
        gcc-arm-linux-gnueabihf
        libasan5-armhf-cross
        libatomic1-armhf-cross
        libc6-armhf-cross      (2.28-7cross1 in debian10)
        libc6-dev-armhf-cross
        libgcc-8-dev-armhf-cross
        libgcc1-armhf-cross
        libgomp1-armhf-cross
        libstdc++-8-dev-armhf-cross
        libstdc++6-armhf-cross
        libubsan1-armhf-cross
        linux-libc-dev-armhf-cross    (4.19.20-1cross1)

@NiLuJe
Copy link
Member

NiLuJe commented Jun 26, 2023

It depends on exactly which symbols the binary pulls, glibc symbols are versioned, so new features for existing symbols just create a new variant with a higher version (e.g., security features).

@poire-z
Copy link
Contributor

poire-z commented Jul 10, 2023

Should we merge this one ? (Still marked as Draft)

@benoit-pierre
Copy link
Contributor Author

I want to fix the issue with the android build on gitlab first.

@benoit-pierre
Copy link
Contributor Author

And to rebase to make sure the (base) CI is passing.

@benoit-pierre benoit-pierre marked this pull request as ready for review July 11, 2023 01:58
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 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @benoit-pierre)

@benoit-pierre benoit-pierre reopened this Jul 11, 2023
@Frenzie Frenzie merged commit e247b65 into koreader:master Jul 11, 2023
@benoit-pierre benoit-pierre deleted the pr/fix_cre_teardown branch July 11, 2023 07:00
benoit-pierre added a commit to benoit-pierre/koreader that referenced this pull request Jul 11, 2023
NiLuJe pushed a commit to koreader/koreader that referenced this pull request Jul 11, 2023
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