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

Development s2 version wasm builds are broken #34

Open
paleolimbot opened this issue Jun 18, 2024 · 2 comments
Open

Development s2 version wasm builds are broken #34

paleolimbot opened this issue Jun 18, 2024 · 2 comments
Labels
feature a feature request or enhancement

Comments

@paleolimbot
Copy link

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
Copy link
Member

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:

  • Under Wasm, include -I$(WASM)/include in PKG_CPPFLAGS.
  • Under Wasm, include -L$(WASM)/lib, and also link with -lssl -lcrypto in PKG_LIBS.

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
Copy link
Author

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 🙂

@georgestagg georgestagg added enhancement feature a feature request or enhancement and removed enhancement labels Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants