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

Fix Emscripten CI tests #977

Closed
fancycode opened this issue Oct 9, 2023 · 5 comments · Fixed by #1000
Closed

Fix Emscripten CI tests #977

fancycode opened this issue Oct 9, 2023 · 5 comments · Fixed by #1000

Comments

@fancycode
Copy link
Member

#925 broke the Emscripten CI, so adding an issue to bring back testing of Emscripten.

Originally posted by @fancycode in #925 (comment)

@farindk
Copy link
Contributor

farindk commented Oct 9, 2023

Yes, I tried to fix the emscripten thing a couple of days ago, but I failed to understand what is going on in the code. It would be nice if you could have a look.

@jerbob92
Copy link
Contributor

jerbob92 commented Oct 9, 2023

I believe the JS test might not be correct. The functions are not directly available after the import (afaik), so in this code:

    var libheif = require('../libheif.js');
    console.log("Loaded libheif.js", libheif.heif_get_version());

I don't think heif_get_version is defined directly afterwards. It could be that was the case with an older version of Emscripten.

This is also why https://github.com/strukturag/libheif/blob/master/examples/demo.html runs on window.onload.

Another issue is that it cuts off the logging, so its hard to see what is actually wrong.

@fancycode
Copy link
Member Author

So there is a regression which will break existing users code after an update. The old version of the JS code published a global libheif (used by the demo) or exported it if used from require (used in the CI tests).

In any case, that's what this issue is for ;-) The broken behaviour should be fixed.

@jerbob92
Copy link
Contributor

jerbob92 commented Oct 9, 2023

In that case, probably. I only looked at the example.html, and that still worked, didn't know there was a unit test.

Not completely sure if it can be fixed in Emscripten, I think they intentionally changed it to work this way.

Perhaps we should look at how it currently wraps the generated JS too, it does some magic which shouldn't really be necessary anyway: https://github.com/strukturag/libheif/blob/master/pre.js#L35 and https://github.com/strukturag/libheif/blob/master/post.js

@jerbob92
Copy link
Contributor

jerbob92 commented Oct 9, 2023

Reading this page it seems that WASM_ASYNC_COMPILATION=0 would fix it, but it comes with some sidenotes: but that may not work in Chrome due to current limitations there..

Maybe we can add the option when USE_WASM is 0, because in that case it doesn't have to load WebAssembly and that should make it loadable in a sync way, USE_WASM=0 is also what the test uses.

kleisauke added a commit to kleisauke/libheif that referenced this issue Oct 18, 2023
kleisauke added a commit to kleisauke/libheif that referenced this issue Oct 18, 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 a pull request may close this issue.

3 participants