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

Libzim wasm compilation #548

Merged
merged 11 commits into from
Nov 22, 2022
Merged

Libzim wasm compilation #548

merged 11 commits into from
Nov 22, 2022

Conversation

mgautierfr
Copy link
Member

@mgautierfr mgautierfr commented Nov 2, 2022

Fixes #503

This PR add support to wasm (for libzim).

It it still WIP has I need to check everything is working for other platform (and on the CI)
compilation of libzim for wasm is working (on my computer) but generated binary has not been tested.

@mgautierfr mgautierfr changed the title Introduce configure and make prefix. Wasm support Nov 2, 2022
@mgautierfr mgautierfr force-pushed the wasm branch 4 times, most recently from fc4b1e5 to 8203d99 Compare November 9, 2022 14:10
@mgautierfr mgautierfr marked this pull request as ready for review November 9, 2022 15:11
@kelson42
Copy link
Contributor

@mgautierfr Do we have a test output blog to share with @Jaifroid to be tested?

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

Note that I skimmed over the emsdk related stuff since I have no experience with emscripten.

kiwixbuild/buildenv.py Outdated Show resolved Hide resolved
kiwixbuild/dependencies/base.py Outdated Show resolved Hide resolved
kiwixbuild/dependencies/base.py Outdated Show resolved Hide resolved
kiwixbuild/dependencies/icu4c.py Show resolved Hide resolved
Comment on lines 12 to 14
archive = Remotefile('xz-5.2.7.tar.gz',
'06327c2ddc81e126a6d9a78b0be5014b976a2c0832f492dcfc4755d7facf6d33',
'https://altushost-swe.dl.sourceforge.net/project/lzmautils/xz-5.2.7.tar.gz'
archive = Remotefile('xz-5.2.6.tar.gz',
'a2105abee17bcd2ebd15ced31b4f5eda6e17efd6b10f921a01cda4a44c91b3a0',
'https://altushost-swe.dl.sourceforge.net/project/lzmautils/xz-5.2.6.tar.gz'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably it doesn't make sense to include in the PR the commit that switches lzma to 5.2.7 (and the commit message of this commit must be slightly updated)

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to make the fallback to 5.2.6 explicit. Ideally, we would move to 5.2.7 but we don't because of compilation issue. By using two commits, it is explicit (and as we will have to update lzma at a moment, better have too much information)

kiwixbuild/dependencies/all_dependencies.py Outdated Show resolved Hide resolved
@kelson42 kelson42 changed the title Wasm support Libzim wasm compilation Nov 13, 2022
@kelson42
Copy link
Contributor

@mgautierfr Does it implement what is requested in #503

Comment on lines 66 to 71
def configure_prefix(self):
return "emconfigure"

@property
def make_prefix(self):
return "emmake"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't fully understand how these prefixes are supposed to work, but my impression was that both prefixes should be simply "em".

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact they are not prefix but wrapper (and I will rename them)

They are tools provided by emsdk to run configure. Something like emconfigure source/configure --options where we would simply run source/configure --options in other case.

They "simply" set the environment before running the real command.

This way, we can use small wrapper tools from sdk to run configure and
make.
On top of using a more recent version (which is good it itself),
version 5.2.7 use a autotools version which knows about wasm.
This way, we can see the applied patch in the log.
@mgautierfr
Copy link
Member Author

@mgautierfr Does it implement what is requested in #503

It implement everything but the compilation of the wrapper (which should be made in https://github.com/openzim/javascript-libzim )
(generated archive still have to be validated, but potential fix could be made in another PR once tested)

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

LGTM

@mgautierfr mgautierfr marked this pull request as draft November 16, 2022 14:12
@mgautierfr
Copy link
Member Author

There is few mistake in the packaging of the wasm dependencies (needed by the CI on libzim side). I move this PR to draft while I'm fixing it.

`icu4c_wasm.patch` is build by :
 - Copying config.sub from liblzma source as new version of config.sub there
   knows about wasm architecture.
 - Copying `mh-linux` on `mh-unknown` as specified in (origin) `mh-unknown`.
   This is because icu4c configure doesn't detect `emscripten` platform and
   "fallback" to `mh-unknown`.
Version 5.2.7 include this commit
https://git.tukaani.org/?p=xz.git;a=commit;h=31d80c6b261b24220776dfaeb8a04f80f80e0a24

With this change, compiling libzim mixed (libzim dynamic and dependencies,
so lzma, statically) fails at libzim linking with a
`src/libzim.so.8.0.1: version node not found for symbol lzma_get_progress@XZ_5.2.2`
error message.

This can be "workaround" by passing `--disable-symbol-versions` to
configure script but then, it is the compilation of kiwix-desktop in
native_dyn which falling with

```
/usr/bin/ld: /usr/lib64/libsystemd.so.0: undefined reference to `lzma_code@XZ_5.0'
/usr/bin/ld: /usr/lib64/libsystemd.so.0: undefined reference to `lzma_end@XZ_5.0'
/usr/bin/ld: /usr/lib64/libsystemd.so.0: undefined reference to `lzma_stream_decoder@XZ_5.0'
/usr/bin/ld: /usr/lib64/libxml2.so.2: undefined reference to `lzma_auto_decoder@XZ_5.0'
/usr/bin/ld: /usr/lib64/libxml2.so.2: undefined reference to `lzma_properties_decode@XZ_5.0'
```

Probably because some native dependencies (Qt ?) use versionned symbols.

This have to be fixed somehow but until then, let's go back to 5.2.6
There is no need to be specific here, we are already in INSTALL_DIR which
contains only things specific to our platform.
We need to be able to test our generated artefacts before we merge the
branches.
@mgautierfr mgautierfr marked this pull request as ready for review November 16, 2022 15:37
@kelson42
Copy link
Contributor

kelson42 commented Nov 16, 2022

@Jaifroid Here is a test wasm libzim http://tmp.kiwix.org/ci/wasm/libzim_wasm-emscripten-2022-11-16.tar.gz. Would you be able please to test it? Once this PR will be merged, it will be in nightly/release folders.

@Jaifroid
Copy link
Member

@Jaifroid Here is a test wasm libzim http://tmp.kiwix.org/ci/wasm/libzim_wasm-emscripten-2022-11-16.tar.gz. Would you be able please to test it? Once this PR will be merged, it will be in nightly/release folders.

OK, great, yes I'll test and report back. Many thanks!

@Jaifroid
Copy link
Member

Jaifroid commented Nov 17, 2022

@kelson42 The file you attached only seems to contain source code (see screenshot). I can't find the wasm in it.... The file should have a file extension .wasm.

image

@rgaudin
Copy link
Member

rgaudin commented Nov 17, 2022

This is include/ folder. The archive (.a) is in lib/ folder

@Jaifroid
Copy link
Member

This is include/ folder. The archive (.a) is in lib/ folder

Ah, I was looking for a file ending .wasm... I'll use that file, thanks.

@kelson42
Copy link
Contributor

This is include/ folder. The archive (.a) is in lib/ folder

Ah, I was looking for a file ending .wasm... I'll use that file, thanks.

This is disturbing for me as well as .a should be an extension reserved for binary code. No way to get it in .wasm?

@Jaifroid
Copy link
Member

Jaifroid commented Nov 17, 2022

Hmm. Seems I can't test properly without the JavaScript that should be produced by Emscripten along with the WASM. Normally they are produced together at compile time. Do we have that? If I just drop the WASM in to the current test implementation, I get the following:

image

@Jaifroid
Copy link
Member

@mossroy Perhaps you can guide us here. Looking at a.out.js, it seems to be a mixture of the Emscripten-produced Web Worker and some hand-crafted code (you have comments in there about split ZIM archives). How is this file produced?

@kelson42
Copy link
Contributor

@mgautierfr macOS CI does not pass!!!

@rgaudin
Copy link
Member

rgaudin commented Nov 17, 2022

@kelson42 it's the same concurrency test we talk about last week

@kelson42
Copy link
Contributor

@rgaudin One test is failing, looks different to me to what we have at libzim/libkiwix level, will let you handle it.

@rgaudin
Copy link
Member

rgaudin commented Nov 17, 2022

@rgaudin One test is failing, looks different to me to what we have at libzim/libkiwix level, will let you handle it.

[  FAILED  ] CreatorError/FaultyDelayedItemErrorTest.faultyUnCompressedItem/6, where GetParam() = 4-byte object <0D-00 00-00>

Nothing I can do. Leaving it to @mgautierfr

@mossroy
Copy link

mossroy commented Nov 18, 2022

@mossroy Perhaps you can guide us here. Looking at a.out.js, it seems to be a mixture of the Emscripten-produced Web Worker and some hand-crafted code (you have comments in there about split ZIM archives). How is this file produced?

So far, it was generated through the (dirty) Makefile from https://github.com/openzim/javascript-libzim
The first step is to compile the libzim (and all its dependencies), to produce the libzim.a file. I suppose the purpose of this ticket is to do that in the CI.

It's true that it's currently not possible for us to test this file alone. I'll elaborate in https://github.com/openzim/javascript-libzim/

@mgautierfr
Copy link
Member Author

So far, it was generated through the (dirty) Makefile from https://github.com/openzim/javascript-libzim
The first step is to compile the libzim (and all its dependencies), to produce the libzim.a file. I suppose the purpose of this ticket is to do that in the CI.

It's true that it's currently not possible for us to test this file alone. I'll elaborate in openzim/javascript-libzim

It is true :)

The purpose of this PR is to build libzim (and only libzim) for wasm "architecture". The wrapper need to be compiled in https://github.com/openzim/javascript-libzim.

It is a the same thing than libzim compile on android platform without the jni wrapper. A necessary but not sufficient step.

I propose we merge this PR as it is and move the work on javascript-libzim. If (when) something has to be changed here, we will see then.

@kelson42
Copy link
Contributor

@mgautierfr So, IMO we are good to merge, module the macOS CI problem. Any idea what goes wrong there?

@mgautierfr
Copy link
Member Author

Let's merge this PR.
macos error is not related (it is mainly the async error being raise too early (!) for the test), I will do a better testing to be more tolerant.
libzim for wasm will be tested on javascript-libzim side and we will see what have to be fixed.

@Jaifroid
Copy link
Member

Jaifroid commented Nov 29, 2022

So, I tried testing an Emscripten compile/bind (with em++) using the libzim.a posted above. I dropped it into the build/lib directory, replacing the libzim.a that was there from a previous successful build. The output is in screenshot: above the red line is the command I ran, below the line are the errors reported.

image

I guess this means that #552 would need to be completed. I don't know if there's another way to link up the dependencies, assuming it is a dependency issue.

@mgautierfr
Copy link
Member Author

I guess this means that #552 would need to be completed.

Yes (And please continue the discussion there)

I don't know if there's another way to link up the dependencies, assuming it is a dependency issue.

You need the dependencies to link with them. No other choices :)

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.

Add a new target "platform" for WebAssembly (compile libzim to Wasm)
6 participants