later icon indicating copy to clipboard operation
later copied to clipboard

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

Open danieldjewell opened this issue 4 years ago • 4 comments

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 https://github.com/mcclure/lovr/commit/c322ce2f04e56f6003a2798b5c8b79bf15641bcd ... 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...)

danieldjewell avatar Jun 13 '20 22:06 danieldjewell

Testing

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

danieldjewell avatar Jun 13 '20 22:06 danieldjewell

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 ]

danieldjewell avatar Jun 13 '20 22:06 danieldjewell

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.)

jcheng5 avatar Jun 16 '20 01:06 jcheng5

@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.

wch avatar Jun 22 '20 17:06 wch