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

Fix for r-lib/later#126 compilation on Android #129

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danieldjewell
Copy link

timespec_get is not available on Android's Bionic libc -- e.g. in
sys/time.h ... This was raised as an issue in
tinycthread/tinycthread#47 and this fix is based on @mcclure's commit
mcclure/lovr@c322ce2
... see also android/ndk#864

Messages

@jcheng5 if you can give this a shot, that'd be great - I ran into this issue when trying to install devtools from CRAN and I'm just sort of learning R itself, so I really have no idea if later is working as intended or not... (see below for notes on testing)

Notes

  • I tested compilation on Termux/aarch64/Android 10 and it does install (Termux currently has R v3.6.3) - since this fix checks for android using a compiler macro (i.e. #ifdef __ANDROID__), the impact on any other platform should be zero.

  • I am unable to fully test on Android - Termux doesn't have a (good/working) implementation of Pandoc (the current package uses a qemu-x86_64 to run an x86_64 binary...) -- and getting a working Pandoc on Termux is really difficult right now as there is (to my knowledge) not a working Haskell compiler available. (If someone can suggest the proper method to run tests on the package without doing the Pandoc part, that'd be great! -- I tried an R CMD build . in the repo root and that's where I ran into the pandoc error...)

timespec_get is not available on Android's Bionic libc -- e.g. in
sys/time.h ... This was raised as an issue in
tinycthread/tinycthread#47 and this fix is based on @mcclure's commit
mcclure/lovr@c322ce2
 ... see also android/ndk#864
@danieldjewell
Copy link
Author

Testing

Whoops, kinda forgot that CI was enabled 😁 - guess we'll know about the impacts on other platforms shortly 😉

@danieldjewell
Copy link
Author

Testing

I did some research and figured out how to run testthat -- see below:


R version 3.6.3 (2020-02-29) -- "Holding the Windsock"
Copyright (C) 2020 The R Foundation for Statistical Computing
Platform: aarch64-unknown-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> library(later)
> library(testthat)
> test_check("later")
══ testthat results  ═══════════════════════════════════════════════════════════
[ OK: 121 | SKIPPED: 2 | WARNINGS: 0 | FAILED: 0 ]

@jcheng5
Copy link
Member

jcheng5 commented Jun 16, 2020

Thanks for the PR!

I'm not sure why the lack of pandoc prevented you from building, R doesn't complain for me if I uninstall pandoc and build. But regardless, you might be able to pass --no-build-vignettes and maybe --no-manual as arguments to R CMD build.

The change looks OK to me but I'll defer to @wch, I'm not sure if there's more ceremony that's required when we modify our copy of tinycthread. (On some other repos we keep a side list of diffs so we can continue to merge from upstream.)

@wch
Copy link
Member

wch commented Jun 22, 2020

@danieldjewell Can you add to src/README.md explaining this change? We've made other modifications to tinycthread and have kept a record of the changes in that file. Otherwise, looks good to me.

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.

None yet

3 participants