Skip to content

feat/wasm emscripten target #171

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat/wasm emscripten target #171

wants to merge 5 commits into from

Conversation

lmmx
Copy link

@lmmx lmmx commented Apr 11, 2025 •
edited
Loading

This is a PR of 1 commit on top of https://github.com/ConSol-Lab/Pumpkin/tree/feat/support-webassembly by @maartenflippo as discussed in #161 (comment)

  • feat: only include OsSignal termination if not in wasm
  • feat(wasm32): use wasm32-unknown-emscripten target to provide C headers needed to build WebAssembly

Update: rebased as requested

@lmmx lmmx mentioned this pull request Apr 11, 2025
Open
@maartenflippo
Copy link
Contributor

I am glad you found a way to make a PR from this. Could you rebase it onto the latest main branch? Then, it is a bit easier to review.

@lmmx lmmx closed this Apr 11, 2025
@lmmx lmmx force-pushed the feat/wasm-emscripten-target branch from 0b143e4 to 952fccf Compare April 11, 2025 14:41
@lmmx lmmx reopened this Apr 11, 2025
@lmmx
Copy link
Author

lmmx commented Apr 11, 2025 •
edited
Loading

@maartenflippo freshly force pushed 👍

edit it's correct again now !

@lmmx
Copy link
Author

lmmx commented Apr 11, 2025 •
edited
Loading

Ah, I came across this forum thread https://users.rust-lang.org/t/emitting-es6-module-for-wasm32-unknown-emscripten/84684

Screenshot from 2025-04-11 16-24-46

It also produces an output that is incompatible with wasm-bindgen (which is the typical way such transpiled JS apps get deployed)

Perhaps it's not the right choice after all. 😞

That said, to the contrary there is active work to support emscripten in wasm-bindgen! rustwasm/wasm-bindgen#4443

So on that grounds I would suggest this is acceptable, and may even soon become more useful :-)

@lmmx
Copy link
Author

lmmx commented Apr 11, 2025

It looks like emscripten disables exceptions by default, and maxsat-checker.cc file uses them, so the build script should pass the -fexeptions flag when building for wasm

@lmmx
Copy link
Author

lmmx commented Apr 11, 2025

I got a working hello world! :-) https://github.com/lmmx/pumpkin-web

Screenshot from 2025-04-11 18-38-00

@maartenflippo
Copy link
Contributor

That is very nice to see. I will ask a few more things before we merge this:

  • Add a CI build for WASM. We want to know when we forget to add a conditional compilation attribute.
  • Everything in pumpkin-solver/build.rs exists only for running tests, so they are not needed when compiling to WASM. Perhaps we can exclude them from the build.
  • Why are we using Emscripten, and not one of the other WASM backends?

Finally, make sure to double-check the changes. At the moment this PR changes the version of the pumpkin-solver crate, which we definitely don't want 😉.

This was linked to issues Apr 15, 2025
[Request] good_lp solver integration #160
Open
WebAssembly compatible? #161
Open
@lmmx
Copy link
Author

lmmx commented May 2, 2025 •
edited
Loading

Hello again! Returning to ship this, so the CI will just need to:

rustup target add wasm32-unknown-emscripten
cargo build -p pumpkin-solver --target wasm32-unknown-emscripten --release

I forgot it was just the solver package I built last time and was wondering why it was erroring - the PyO3 part is incompatible for the record, but I think building just the one package suffices.

I enabled running this on my fork but it won't for some reason

Oh and to answer your 3 points:

  • CI build added, pending check that it works
  • I added an exception, not the entirety of the build script, but this seemed sufficient at the time. Happy to go further
  • As I recall the basic wasm backend can't communicate with C (over FFI, which pumpkin's solver is doing) and when I tried the wasi options emscripten was the one that worked for me, so I went with that. If it'd work with others then we could add other targets.

ImkoMarijnissen
ImkoMarijnissen requested changes May 12, 2025
View reviewed changes
pumpkin-solver/Cargo.toml
@@ -1,6 +1,6 @@
[package]
name = "pumpkin-solver"
version = "0.2.0"
version = "0.1.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

The versioning is incorrect when compared to the current main

pumpkin-solver/Cargo.toml
once_cell = "1.19.0"
downcast-rs = "1.2.1"
drcp-format = { version = "0.2.1", path = "../drcp-format" }
drcp-format = { version = "0.2.0", path = "../drcp-format" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here please

pumpkin-solver/src/bin/pumpkin-solver/flatzinc/mod.rs
@@ -27,6 +27,9 @@ use pumpkin_solver::results::SatisfactionResult;
use pumpkin_solver::results::SolutionReference;
use pumpkin_solver::statistics::StatisticLogger;
use pumpkin_solver::termination::Combinator;
#[cfg(target_arch = "wasm32")]
use pumpkin_solver::termination::Indefinite as OsSignalReplacement;
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming here is a bit confusing; Indefinite is fine here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ImkoMarijnissen ImkoMarijnissen ImkoMarijnissen requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WebAssembly compatible? [Request] good_lp solver integration
3 participants
@lmmx @maartenflippo @ImkoMarijnissen