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

Move callr and friends to Suggests #1596

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

lionel-
Copy link
Member

@lionel- lionel- commented Mar 29, 2022

To make webR support easier until we figure out callr.

@lionel- lionel- requested a review from gaborcsardi March 29, 2022 11:14
@gaborcsardi gaborcsardi requested a review from hadley March 29, 2022 11:23
@gaborcsardi
Copy link
Member

This will probably break packages that have

Config/testthat/parallel: true

on CRAN.

@gaborcsardi
Copy link
Member

An alternative would be to qualify all calls with :: and then they are not loaded with testthat. If it is possible to install ps, processx and callr, then this can work, and it does not break anything.

@gaborcsardi
Copy link
Member

An alternative would be to qualify all calls with :: and then they are not loaded with testthat. If it is possible to install ps, processx and callr, then this can work, and it does not break anything.

Which is already the case AFAICT:

❯ R -q --vanilla -e 'library(testthat); loadedNamespaces()'
> library(testthat); loadedNamespaces()
 [1] "compiler"  "magrittr"  "R6"        "cli"       "graphics"  "utils"
 [7] "grDevices" "stats"     "datasets"  "methods"   "brio"      "testthat"
[13] "rlang"     "base"

@lionel-
Copy link
Member Author

lionel- commented Mar 29, 2022

It is currently not possible to install callr, ps, and processx. It might be possible without too many changes but I haven't investigated it yet. At the moment I am trying to make testthat work (it causes webR to freeze).

This will probably break packages that have Config/testthat/parallel: true

Could we fall back to serial testing if callr is not installed?

@gaborcsardi
Copy link
Member

Could we fall back to serial testing if callr is not installed?

Maybe, but they are not interchangeable.

In general it would be better to have a solution that does not break people's current workflows. Which is possible, I think. E.g. we can change processx etc. to install cleanly in webR, and fail for the function calls.

If you don't want to deal with this now, I guess you can use your testthat branch that does not have callr?

@lionel-
Copy link
Member Author

lionel- commented Mar 29, 2022

yes there is no hurry, webr-repo currently uses this branch.

@gaborcsardi
Copy link
Member

Btw. there is this thing: https://browsix.org/ that handles fork(), etc. on top of web workers, so I guess at some point callr etc. will work in webassembly.

@lionel-
Copy link
Member Author

lionel- commented Mar 29, 2022

yup but browsix is basically unmaintained and unused. The WASI folks seem interested in process launching and IPC the long term but there is currently not much interest in the wasm community, so we're probably years away of a working solution. It seems the best approach would be to specially handle the case of calling back to R by instantiating a new emscripten module in a web worker, but it's not clear how we can make them share the same virtual filesystem.

@gaborcsardi
Copy link
Member

Btw. if you set the TESTTHAT_PARALLEL env var to FALSE then you probably don't need a custom testthat branch. You could set it at startup I guess?

@lionel-
Copy link
Member Author

lionel- commented Mar 31, 2022

This branch fixes issues at install time rather than runtime: https://github.com/lionel-/webr-repo/blob/7f0c7bb7337b9071bf3952cb8e779b52b91f201a/install.R#L6

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