search

r-wasm / rwasm

Build R packages for WebAssembly and create a CRAN-like repo for distribution.
https://r-wasm.github.io/rwasm/
Other
54 stars 5 forks source link

Development s2 version wasm builds are broken #34

Open paleolimbot opened 2 months ago

paleolimbot commented 2 months ago

First off, totally impressive that s2 built for wasm!

With the latest r-universe push (and with the next CRAN version, which won't be for a little while as we sort out various packaging issues), the makevars override in this repo will need updating since there are some changes that won't work with the version here (e.g., we change the files that need compiling, which in the makevars here is hard-coded: https://github.com/r-universe/r-spatial/actions/runs/9551963584/job/26327872443#step:5:521 ).

Ideally we'd like to rig the makevars so that they don't need modifying at all. I'm not great at WASM but it seems like there are maybe just a few include items that need adding?

georgestagg commented 2 months ago

Thanks for letting us know!

I agree, it would be better that we not require the override at all. It looks to me like this should not be too difficult to implement. The changes required look to be:

It's also likely the above flags would be discovered automatically using pkg-config for openssl, which your configure script does seem to be doing.

Then, we need to ensure that you don't compile with the -pthread flag under Wasm. That type of threading currently does not work with webR.

I think the best way to do this is via the configure script that you already have. It should be possible to detect when rwasm is cross-compiling for Wasm by checking the output of uname -s, which will return the value Emscripten. I could take a look at submitting a PR along these lines when I have time, if you'd like. Or feel free to make the changes in a feature branch, and I can then test building s2 in my local development version of rwasm without the override.

Finally, once we're happy, we'll need to update rwasm to remove the override. Then r-universe will use the package's own configure script and Makevars.in directly.

paleolimbot commented 2 months ago

Thanks for the walkthrough! There's one more PR upgrading another one of the dependencies and then I'll put up a PR for this and give you a ping 🙂