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

bump crengine: minor fixes, upstream picks #1252

Merged
merged 1 commit into from
Dec 5, 2020

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Dec 5, 2020

Includes:


This change is Reviewable

Includes:
- (Upstream) Fix errors in lString16 to 32 update
- (Upstream) Misc Win32 fixes
- (Upstream) lString32: add 2 additional pos() methods
- (Upstream) Ignore WORD_JOINER when font has no glyph
- (Upstream) FB2 metadata: discard empty genres
- FB2: mark 2nd++ BODYs as non-linear
- Fix support in EPUB for <img src="data:image/png;base64,...>
- XML parsing: correctly parse CDATA
@pazos
Copy link
Member

pazos commented Dec 5, 2020

@poire-z @NiLuJe : for one time the failure in macos is relevant.

otool -L won't list libluajit.so as needed by libkoreader-nnsvg, so the command fails. I wonder if that's because the hidden attribute. I will check

@poire-z
Copy link
Contributor Author

poire-z commented Dec 5, 2020

Thanks @pazos, waiting for your feedback before bumping base into front.
(Merging this PR, as it's unrelated to the nnsvg issue.)

@poire-z poire-z merged commit 3ea2a69 into koreader:master Dec 5, 2020
@poire-z poire-z deleted the bump_crengine branch December 5, 2020 18:55
@pazos
Copy link
Member

pazos commented Dec 5, 2020

@poire-z: nothing to do with -fvisibility=hidden. The following diff fixes the issue

diff --git a/Makefile b/Makefile
index b49b784e..f7ba5656 100644
--- a/Makefile
+++ b/Makefile
@@ -190,11 +190,11 @@ ifdef DARWIN
                $@
 endif
 
-$(OUTPUT_DIR)/libs/libkoreader-nnsvg.so: nnsvg.c \
+$(OUTPUT_DIR)/libs/libkoreader-nnsvg.so: \
                        $(if $(USE_LUAJIT_LIB),$(LUAJIT_LIB),) \
-                       $(NANOSVG_HEADERS)
+                       nnsvg.c
        $(CC) -I$(NANOSVG_INCLUDE_DIR) \
-       $(CFLAGS) $(DYNLIB_CFLAGS) -fvisibility=hidden -Wall -o $@ nnsvg.c
+       $(CFLAGS) $(DYNLIB_CFLAGS) -fvisibility=hidden -Wall -o $@ $^
 ifdef DARWIN
        install_name_tool -change \
                `otool -L "$@" | grep "libluajit" | awk '{print $$1}'` \

@Frenzie
Copy link
Member

Frenzie commented Dec 5, 2020

for one time the failure in macos is relevant.

I thought it was crapping out early these days because it's such a lovely always moving target?

@poire-z
Copy link
Contributor Author

poire-z commented Dec 5, 2020

I think I need that dependancy on $(NANOSVG_HEADERS) to be "built" (actually, copied from the source to some common place NANOSVG_INCLUDE_DIR=/thirdparty/nanosvg/build/include/ so this build can find them here.
That's the reason I did what you're just reverting :) :/
(Or may be it's because I build my little library with another recipe I hijack/add in the Makefile?)

@poire-z
Copy link
Contributor Author

poire-z commented Dec 5, 2020

Precisely, this target;

$(NANOSVG_HEADERS): $(THIRDPARTY_DIR)/nanosvg/*.*
install -d $(NANOSVG_BUILD_DIR)
cd $(NANOSVG_BUILD_DIR) && \
$(CMAKE) $(CMAKE_FLAGS) \
$(CURDIR)/$(THIRDPARTY_DIR)/nanosvg && \
$(CMAKE_MAKE_PROGRAM) $(CMAKE_MAKE_PROGRAM_FLAGS)
install -d $(NANOSVG_INCLUDE_DIR)
cp -fL $(NANOSVG_DIR)/nanosvg/src/nanosvg.h $(NANOSVG_INCLUDE_DIR)/
cp -fL $(NANOSVG_DIR)/nanosvg/src/nanosvgrast.h $(NANOSVG_INCLUDE_DIR)/
cp -fL $(NANOSVG_DIR)/nanosvg/example/stb_image_write.h $(NANOSVG_INCLUDE_DIR)/

@poire-z
Copy link
Contributor Author

poire-z commented Dec 5, 2020

install_name_tool -change \
		`otool -L "build/x86_64-apple-darwin19.6.0/libs/libkoreader-nnsvg.so" | grep "libluajit" | awk '{print $1}'` \
		libs/libluajit.so \
		build/x86_64-apple-darwin19.6.0/libs/libkoreader-nnsvg.so
Usage: /Applications/Xcode_11.3.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/install_name_tool [-change old new] ... [-rpath old new] ... [-add_rpath new] ... [-delete_rpath old] ... [-id name] input
make: *** [build/x86_64-apple-darwin19.6.0/libs/libkoreader-nnsvg.so] Error 1

What's the difference before/after in the output of that ?
otool -L "build/x86_64-apple-darwin19.6.0/libs/libkoreader-nnsvg.so"
I guess just libluajit missing - which feels like it could still be the fault of fvisibility=hidden...

@pazos
Copy link
Member

pazos commented Dec 5, 2020

What's the difference before/after in the output of that ?

Without the change

base/build/x86_64-apple-darwin20.1.0/libs/libkoreader-nnsvg.so:
	build/x86_64-apple-darwin20.1.0/libs/libkoreader-nnsvg.so (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.0.0)

and otool fails as in the log.

with the change

base/build/x86_64-apple-darwin20.1.0/libs/libkoreader-nnsvg.so:
	build/x86_64-apple-darwin20.1.0/libs/libkoreader-nnsvg.so (compatibility version 0.0.0, current version 0.0.0)
	libs/libluajit.so (compatibility version 2.1.0, current version 2.1.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.0.0)

-fvisibility doesn't change that :/

@poire-z
Copy link
Contributor Author

poire-z commented Dec 5, 2020

What does this (vs current master) do ?

-       $(CFLAGS) $(DYNLIB_CFLAGS) -fvisibility=hidden -Wall -o $@ nnsvg.c
+       $(CFLAGS) $(DYNLIB_CFLAGS) -fvisibility=hidden -Wall -o $@ nnsvg.c $(if $(USE_LUAJIT_LIB),$(LUAJIT_LIB),)

I remember it complained when I used $^ - which then included $(NANOSVG_HEADERS) in there - so let's just do as $^ wihout the headers - https://devhints.io/makefile

Our Master Builders @Frenzie & @NiLuJe will probably know the better way to do that.

@pazos
Copy link
Member

pazos commented Dec 5, 2020

I will try in a few minutes as I'm building master right now. Brew changed how luarocks is built and screw our build scripts again :(. osx is worse than android 😢

@poire-z
Copy link
Contributor Author

poire-z commented Dec 5, 2020

Can I still bump base (so, breaking frontend Mac builds I guess, but making nightlies) - or is it best to wait till this is solved?

@pazos
Copy link
Member

pazos commented Dec 5, 2020

yeah, that works, the change was exactly:

diff --git a/Makefile b/Makefile
index b49b784e..185fc082 100644
--- a/Makefile
+++ b/Makefile
@@ -194,7 +194,8 @@ $(OUTPUT_DIR)/libs/libkoreader-nnsvg.so: nnsvg.c \
                        $(if $(USE_LUAJIT_LIB),$(LUAJIT_LIB),) \
                        $(NANOSVG_HEADERS)
        $(CC) -I$(NANOSVG_INCLUDE_DIR) \
-       $(CFLAGS) $(DYNLIB_CFLAGS) -fvisibility=hidden -Wall -o $@ nnsvg.c
+       $(CFLAGS) $(DYNLIB_CFLAGS) -fvisibility=hidden -Wall -o $@ nnsvg.c \
+               $(if $(USE_LUAJIT_LIB),$(LUAJIT_LIB),)
 ifdef DARWIN
        install_name_tool -change \
                `otool -L "$@" | grep "libluajit" | awk '{print $$1}'` \

Can I still bump base (so, breaking frontend Mac builds I guess, but making nightlies) - or is it best to wait till this is solved?

Please update. mac builds are broken in many ways :p

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.

None yet

3 participants