Skip to content

Conversation

jeroen
Copy link
Contributor

@jeroen jeroen commented Dec 15, 2023

Prevents packages from accessing the host LIBS.

This fixes the unrtf package.

Prevents packages from accessing the host LIBS
@georgestagg
Copy link
Member

Great spot, thanks!

A couple of things:

  1. Setting this variable blank is better than using the host LIBS, but can we do better? I took a peak the Makeconf installed to the webR virtual filesystem and based on that perhaps something like this would be more accurate?
override LIBS = -L$(WASM)/lib -lpcre2-8 -llzma -lbz2 -lz -lrt -ldl -lm

OTOH that means keeping the list of libraries in sync, and a blank list might be OK to use here in any case... Do you happen to know if LIBS is used by any other packages?

  1. This will allow the unrtf package to build, but it unfortunately still won't work. It relies on starting a process using sys::exec_wait, and this cannot be done under Emscripten as there is no support for fork().

@jeroen
Copy link
Contributor Author

jeroen commented Dec 18, 2023

Maybe your suggestion is better. I am not sure what the official use of LIBS is actually, it seems a common use is add flags needed to build a an executable: https://github.com/search?q=org%3Acran%20path%3Asrc%2FMakevars%20%24(LIBS)&type=code

And yes sadly it still does not work, but at least it would be available as a dependency

@georgestagg
Copy link
Member

georgestagg commented Dec 18, 2023

I am not sure what the official use of LIBS is actually, it seems a common use is add flags needed to build a an executable

Okay. Since it's not used so much on CRAN (thanks for the search link!), and building a separate executable is not really useful under Emscripten right now in any case, let's leave it blank. It is easy to add these flags later if we really need them.

And yes sadly it still does not work, but at least it would be available as a dependency

True. Hopefully one day we will work something out between R and Emscripten for starting new processes under webR using additional JavaScript Web Workers.

@georgestagg georgestagg merged commit d445736 into r-wasm:main Dec 18, 2023
@jeroen jeroen deleted the patch-1 branch December 18, 2023 11:37
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.

2 participants