bump crengine: support RTL/bidi text, adds thirdparty/fribidi #980
Conversation
| set(CFG_OPTS "-q --prefix=${BINARY_DIR} --disable-static --enable-shared --host=\"${CHOST}\"") | ||
| set(CFG_CMD sh -c "${CFG_ENV_VAR} ${SOURCE_DIR}/configure ${CFG_OPTS}") | ||
|
|
||
| # Disable building docs, as it requires c2man |
There was a problem hiding this comment.
And reducing built time by reducing unnecessary stuff is a good idea regardless. ;-)
thirdparty/fribidi/CMakeLists.txt
Outdated
| ko_write_gitclone_script( | ||
| GIT_CLONE_SCRIPT_FILENAME | ||
| https://github.com/fribidi/fribidi.git | ||
| v1.0.5 |
There was a problem hiding this comment.
I've taken to using:
tags/v1.0.5
Because I figured it might be clearer to onlookers. Also you might have a branch and a tag of the same name but that's generally unlikely to be a concern (because even if it is… the 1.0.5 branch would probably be pretty similar to the tag).
thirdparty/fribidi/CMakeLists.txt
Outdated
| ExternalProject_Add( | ||
| ${PROJECT_NAME} | ||
| DOWNLOAD_COMMAND ${CMAKE_COMMAND} -P ${GIT_CLONE_SCRIPT_FILENAME} | ||
| PATCH_COMMAND NOCONFIGURE=1 ./autogen.sh |
There was a problem hiding this comment.
I think using CFG_CMD1 and CFG_CMD2 would be clearer.
Like this, except the example below refers to actual patches:
There was a problem hiding this comment.
Actually since they depend on each other we should just use the && method?
|
Any idea why the android builds fail with: while no other package complains about The pocketbook builds have been failing for some time because: |
Only on Travis, which we can probably remove by now.
It looks like you're passing it as a flag to the linker, but it's only for the compiler. |
|
I can look at the Android build on my local environment this afternoon if you like. Trying to fix it on Travis would just take an eternity. |
|
Actually hold on, is Android even supposed to use /usb/bin/ld? I think it should use the one from the Android TC. |
|
Line 460 in 6b64978 |
|
@pazos Yeah, so it's using the wrong linker. The flags are fine. My bad. :-) |
|
Had a random test by changing CMAKE_CURRENT_SOURCE_DIR to CMAKE_CURRENT_LIST_DIR, as some other package have it - but it doesn't help.
Yes, please, thanks. |
|
@poire-z Am I doing something wrong here btw? I'm getting a bunch of question marks on Hebrew characters:
|
|
Dunno, do you have a font with these glyphs, as main or fallback font? FreeSerif or DejaVuSans should have them. |
|
Oh, indeed. It was set to Noto Serif. I didn't realize Hebrew wasn't in there. |
|
@NiLuJe Could you also take a look? 😊 Forcing ld with a CCLD environment variable fails due to DEFS from the configure script, and either way shouldn't gcc take care of using the correct linker? If I force the Android ld with CCLD=${LD} then I get: In case it's of use: |
|
CCLD is supposed to be CC, not LD. Not quite sure what could be going especially wrong, as it appears to be bog standard autotools + libtools. :? Switch to a verbose build ( (i.e., |
I figured as much but forgot to edit the message. Different failure. :-) |
|
Like so: |
|
I would assume this means something is picking up the wrong (i.e., native) compiler/linker somewhere, still (switching to a verbose build might shed some light on it). |
|
Missing something or other still I guess: |
|
Verbose: |
|
Regarding the first error: On an android build, it could be it/Cmake has created another subdir than |
|
@poire-z I think that's just CMake 3.5 giving off a confusing error message. If that actually failed then it should've quit right there. Looks something like this on slightly more modern CMake: |
|
There's something horribly wrong with the Regular build: Current Android build: But also, does this mean that this stuff needs to be compiled with HOST_CC/CC_FOR_BUILD or something? You can't run ARM stuff on x86… The fact that you can run the x86 Android executables on your regular PC is just a nice convenience. |
|
Well, especially given what's in it: I'd switch to a release tarball, which hopefully should ship this pre-generated? Because dealing with this kind of stuff when x-compiling is extremely unfun... |
|
For testing, you might try to build with their latest commit, instead of the year old v1.0.5. |
|
@poire-z Way ahead of you. ;-D This is the messy diff of my current experiment: diff --git a/Makefile.third b/Makefile.third
index 5ede639..a02712c 100644
--- a/Makefile.third
+++ b/Makefile.third
@@ -45,8 +45,8 @@ $(UTF8PROC_LIB) $(UTF8PROC_DIR): $(THIRDPARTY_DIR)/utf8proc/CMakeLists.txt
$(FRIBIDI_LIB) $(FRIBIDI_DIR): $(THIRDPARTY_DIR)/fribidi/CMakeLists.txt
install -d $(FRIBIDI_BUILD_DIR)
cd $(FRIBIDI_BUILD_DIR) && \
- $(CMAKE) $(CMAKE_FLAGS) -DCC="$(CC)" -DCFLAGS="$(CFLAGS)"\
- -DLDFLAGS="$(LDFLAGS)" -DCHOST=$(CHOST) \
+ $(CMAKE) $(CMAKE_FLAGS) -DCC="$(CC)" -DCXX="$(CXX)" -DCFLAGS="$(CFLAGS)" -DCXXFLAGS="$(CXXFLAGS)" -DLD="$(LD)"\
+ -DLDFLAGS="$(LDFLAGS)" -DCHOST=$(CHOST) -DCC_FOR_BUILD=$(HOST_CC) \
$(CURDIR)/$(THIRDPARTY_DIR)/fribidi && \
$(CMAKE_MAKE_PROGRAM) $(CMAKE_MAKE_PROGRAM_FLAGS)
cp -fL $(FRIBIDI_DIR)/lib/$(notdir $(FRIBIDI_LIB)) $@
diff --git a/thirdparty/fribidi/CMakeLists.txt b/thirdparty/fribidi/CMakeLists.txt
index b1c0af7..ddb10dc 100644
--- a/thirdparty/fribidi/CMakeLists.txt
+++ b/thirdparty/fribidi/CMakeLists.txt
@@ -1,7 +1,7 @@
project(fribidi)
cmake_minimum_required(VERSION 3.5.1)
-set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_LIST_DIR}/../cmake_modules")
+set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_SOURCE_DIR}/../cmake_modules")
include("koreader_thirdparty_common")
include("koreader_thirdparty_git")
@@ -13,7 +13,7 @@ assert_var_defined(LDFLAGS)
ep_get_source_dir(SOURCE_DIR)
ep_get_binary_dir(BINARY_DIR)
-set(CFG_ENV_VAR "CC=\"${CC}\" CXX=\"${CXX}\" CPPFLAGS=\"${CPPFLAGS}\" LDFLAGS=\"${LDFLAGS}\" ")
+set(CFG_ENV_VAR "CC=\"${CC}\" CC_FOR_BUILD=\"${CC_FOR_BUILD}\" CCLD=\"${CC}\" CXX=\"${CXX}\" CFLAGS=\"${CFLAGS}\" LDFLAGS=\"${LDFLAGS}\" ")
set(CFG_OPTS "-q --prefix=${BINARY_DIR} --disable-static --enable-shared --host=\"${CHOST}\"")
set(CFG_CMD sh -c "${CFG_ENV_VAR} ${SOURCE_DIR}/configure ${CFG_OPTS}")
@@ -28,10 +28,12 @@ if($ENV{ANDROID})
set(CFG_CMD "${CFG_CMD} && ${ISED} 's|soname_spec=.*|soname_spec=\"\\\\\$libname\\\\\$release\\\\\$shared_ext\\\\\$major\"|' libtool")
endif()
+set(MAKE_CMD sh -c "${KO_MAKE_RECURSIVE} V=1 CC=\"${CC}\" CC_FOR_BUILD=\"${CC_FOR_BUILD}\" CCLD=\"${CC}\" CFLAGS=\"${CFLAGS}\" LDFLAGS=\"${LDFLAGS}\" LD=\"${LD}\" AR=\"${AR}\" RANLIB=\"${RANLIB}\"")
+
ko_write_gitclone_script(
GIT_CLONE_SCRIPT_FILENAME
https://github.com/fribidi/fribidi.git
- tags/v1.0.5
+ 0f849e344d446934b4ecdbe9edc32abd29029731
${SOURCE_DIR}
)
@@ -42,6 +44,7 @@ ExternalProject_Add(
PATCH_COMMAND NOCONFIGURE=1 ./autogen.sh
BUILD_IN_SOURCE 1
CONFIGURE_COMMAND COMMAND ${CFG_CMD}
- BUILD_COMMAND ${KO_MAKE_RECURSIVE} -j${PARALLEL_JOBS}
+ #BUILD_COMMAND ${KO_MAKE_RECURSIVE} -j${PARALLEL_JOBS} CC=${CC} LDFLAGS="${LDFLAGS}" CCLD=${CC}
+ BUILD_COMMAND ${MAKE_CMD}
INSTALL_COMMAND ${KO_MAKE_RECURSIVE} -j${PARALLEL_JOBS} install
) |
|
@NiLuJe I think they may have deprecated release tarballs? |
|
Minor sidenote, CC_FOR_BUILD seems to be set intelligently by Autotools. In the end I guess that means that |
|
Yup, removing that takes care of the problem. We've got a working Android binary: Now the only question that remains is how to use two different sets of CFLAGS, because even if |
|
PS I should probably point out I know it's a proper Android binary due to the machine and ELF32 vs ELF64. Here's the desktop one with |
|
There are a few notes in |
|
Yes, that's what CC_FOR_BUILD, CFLAGS_FOR_BUILD and LDFLAGS_FOR_BUILD are for afaict. Doesn't seem to be working out for me though. It works for Kobo because there's no weird stuff in CFLAGS/LDFLAGS. So I guess we can make it work by getting rid of that particular flag (if necessary even just here) but that doesn't seem right at all. ;-) |
|
It seems to me that the Makefile actually isn't using the correct LDFLAGS? (By which I'm not talking about the semi-random value I filled LDFLAGS_FOR_BUILD with but about the fact that we end up with --fix-cortex-a8.) Which would be traced over here: https://github.com/fribidi/fribidi/blob/0f849e344d446934b4ecdbe9edc32abd29029731/gen.tab/Makefile.am#L26-L29 Edit: PR Submitted upstream. |
poire-z
force-pushed
the
fribidi_bump_crengine
branch
from
28aea9f
to
6e4c497
Compare
|
(Dunno how much we will wait for upstream merge, so just had a try patching here ... spent too much time with sed and trying to include |
|
@poire-z You could also include a simple patch file (but this solution is fine). Btw, the CMakeLists.txt still needs some touch-ups. It's not passing through the CFLAGS, there's not enough checking if vars are defined (which would show that no CPPFLAGS are passed through, assuming that's even necessary), and some minor stuff like that. :-) |
Please make suggestions! that's not my natural language and culture at all :) --- a/thirdparty/fribidi/CMakeLists.txt
+++ b/thirdparty/fribidi/CMakeLists.txt
@@ -8,6 +8,7 @@ include("koreader_thirdparty_git")
enable_language(C)
assert_var_defined(CC)
+assert_var_defined(CFLAGS)
assert_var_defined(LDFLAGS)
ep_get_source_dir(SOURCE_DIR)
@@ -16,7 +17,7 @@ ep_get_binary_dir(BINARY_DIR)
# Temporary, while waiting for https://github.com/fribidi/fribidi/pull/115 to be merged
set(PATCH_CMD sh -c "${ISED} \"s|^CFLAGS = |LDFLAGS = \\nCFLAGS = |\" gen.tab/Makefile.am")
-set(CFG_ENV_VAR "CC=\"${CC}\" CXX=\"${CXX}\" CPPFLAGS=\"${CPPFLAGS}\" LDFLAGS=\"${LDFLAGS}\" ")
+set(CFG_ENV_VAR "CC=\"${CC}\" CFLAGS=\"${CFLAGS}\" LDFLAGS=\"${LDFLAGS}\" ")
set(CFG_OPTS "-q --prefix=${BINARY_DIR} --disable-static --enable-shared --host=\"${CHOST}\"")
set(CFG_CMD sh -c "${CFG_ENV_VAR} ${SOURCE_DIR}/configure ${CFG_OPTS}")
|
| enable_language(C) | ||
|
|
||
| assert_var_defined(CC) | ||
| assert_var_defined(LDFLAGS) |
|
@poire-z That plus checking for CHOST should do the trick I think. The |
Adds fribidi 1.0.5 (libfribidi.so.0). For use by crengine to render RTL/bidi text. Includes some temporary patching to allow android build while we wait for the fix to be merged upstream (patch to be removed with the version bump).
Includes: - Fix possible jumps in list items numbering - lvfntman.cpp: reformating for better readability - lvtextfm.cpp: upgrade m_flags from lUInt8 to lUInt16 - Fonts: use Freetype embolden API for fake bold - Fonts: have fallback font be italic/bold as the main font - Fonts: handle RTL, rework Harfbuzz measuring and drawing - Text formatting: use fribidi for RTL/bidi text layout - Update XPointers/pt translation methods to handle bidi/rtl words - EPUB: forward dir and lang attributes from the html element - RTL: fix in-page footnotes order - Fix a few clang-tidy warnings
poire-z
force-pushed
the
fribidi_bump_crengine
branch
from
6e4c497
to
a14eb2a
Compare
koreader/koreader-base#980 Also includes: * SDL: add rumble support koreader/koreader-base#979 * Update to FBInk v1.20.0 koreader/koreader-base#978
koreader/koreader-base#980 Also includes: * SDL: add rumble support koreader/koreader-base#979 * Update to FBInk v1.20.0 koreader/koreader-base#978
koreader/koreader-base#980 Also includes: * SDL: add rumble support koreader/koreader-base#979 * Update to FBInk v1.20.0 koreader/koreader-base#978
Adds to thirdparty: https://github.com/fribidi/fribidi
Bump crengine koreader/crengine#309 :