Get Rhome and libR from Preferences.jl when provided #496

frankier · 2023-08-04T11:44:41Z

codecov · 2023-08-14T07:42:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5d0ddeb) 75.76% compared to head (1273105) 75.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #496      +/-   ##
==========================================
+ Coverage   75.76%   75.77%   +0.01%     
==========================================
  Files          26       26              
  Lines        1642     1643       +1     
==========================================
+ Hits         1244     1245       +1     
  Misses        398      398

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

frankier · 2023-08-14T08:49:41Z

Hi @palday. This is ready for review now and the tests seem to pass.

An explanation of the approach is given in my documentation changes here. Please let me know if you need any extra info.

frankier · 2023-08-14T15:22:41Z

One aim of this PR is to make it possible to install RCall without an R installation. The reason is that RCall may get installed at the same time as CondaPkg, but we may need CondaPkg to get our R installation.

At first it seems like it would be possible to proceed with precompilation of RCall with just a dummy value for Rhome, however, when other modules which import RCall are precompiled they will run the __init__ . I tried an approach that checked for currently_compiling() = ccall(:jl_generating_output, Cint, ()) != 0 in __init__ and skipped the init in that case, however this will mean an R interpreter is not started, and apparently this is needed even during precompliation time for the R_str macro. The resulting segfault was a bit surprising to me -- but I suppose the R runtime is needed even at this stage for such a close integration.

Therefore the only thing to do is disable precompilation entirely when no Rhome is available. Thanks to the usage of Preferences, the preference is marked as a compile time preference and precompilation will be retriggered. An error message is printed in case the module is actually imported at run-time without having a configured Rhome.

I am interested in whether there is a subset of this module which is safe to precompile without an R interpreter for the future, which might allow for a cleaner solution for people only needing this subset, but for now this PR is as far as I can tell a minimally invasive change to getting things working nicely with Preferences and things that can integrated using Preferences such as CondaPkg.

palday · 2023-08-29T00:36:01Z

@frankier I haven't forgotten you! But I also probably won't review this today, my apologies. 😦

Thanks for your contribution.

ParadaCarleton · 2023-09-07T17:07:34Z

This would definitely be super useful! I'd be very happy to see it merged.

Project.toml

docs/src/installation.md

This is in case the user wants to specify and installation via Preferences.jl after installation. An error message is printed at import time if no installation is available.

frankier · 2023-09-14T11:57:57Z

Updated with your comments. I guess the tests are false negatives? They don't seem to be to do with any of the changes and weren't there before (apart from Windows latest which was on master)?

docs/src/installation.md

…r than install-time in docs

frankier · 2024-01-08T08:15:18Z

Thanks for cleaning up the tests and for the feedback.

It looks like this test is failing https://github.com/JuliaInterop/RCall.jl/actions/runs/7444228626/job/20250301671?pr=496

Last time I looked it was failing due to a problem present on main, but now it seems to be an actual error. I guess the location of R.dll is being guessed incorrectly? Do you have any ideas? I guess it wasn't introduced during the test refactoring? I don't have a convenient way to test with Windows locally, but will try to set-up/fire-up a VM or something to troubleshoot in case the answer to those two questions is a negative.

palday · 2024-01-08T15:44:31Z

Thanks for cleaning up the tests and for the feedback.

It looks like this test is failing https://github.com/JuliaInterop/RCall.jl/actions/runs/7444228626/job/20250301671?pr=496

Last time I looked it was failing due to a problem present on main, but now it seems to be an actual error. I guess the location of R.dll is being guessed incorrectly? Do you have any ideas? I guess it wasn't introduced during the test refactoring? I don't have a convenient way to test with Windows locally, but will try to set-up/fire-up a VM or something to troubleshoot in case the answer to those two questions is a negative.

Same here -- I've got a Windows VM set up, but I'm really slow figuring it out. AFAICT the path is correct, but there's something about loading DLLs not in the system variable %PATH% that doesn't work. I haven't yet figured it out, but I'm working on it when I get a few minutes in the evenings. 🙁

There was also an issue with the CI on macOS, but it looked to be a more fundamental issue with Conda and the availability of md5 vs md5sum vs something else, so I just disabled that check. The last bit to get this merged is understanding the limitations on different platforms (e.g. "Linux can handle all this dynamically, but Windows requires setting XXX") and having the CI reflect that. Once we have that, I think this is good to go! So close!

…nkier/496

…ctivate

frankier · 2024-01-21T18:16:56Z

Hi! I just checked this on a Windows laptop I got access to. Using dependencywalker.exe, I found that BLAS wasn't being found which led me to realise that this was actually a bug in the test code that was only surfacing on Windows. The CondaPkg environment is not activated(!) Apparently on the other systems another version of BLAS just gets picked up. On non-Conda R it's not a problem because everything is compiled into the DLLs I think. I fixed this and added a note to the docs. I also marked the information about using with CondaPkg as experimental.

frankier · 2024-01-21T18:32:50Z

Okay -- another failure on Windows. I feel like it worked correctly locally but I will look into it soon.

palday · 2024-01-21T18:33:56Z

Okay -- another failure on Windows. I feel like it worked correctly locally but I will look into it soon.

I feel like we're heading in the right direction -- thanks for your patience and efforts!

docs/src/installation.md

frankier · 2024-01-30T18:28:11Z

test/installation/swap_to_prefs_and_condapkg.jl

@@ -39,6 +39,7 @@ set_preferences!(RCALL_UUID,
                 "Rhome" => target_rhome, "libR" => locate_libR(target_rhome))
 RCall = nothing
 CondaPkg.withenv() do
+    Pkg.build("RCall")


Is this still needed?

It seemed to be in my testing.

frankier · 2024-01-30T18:30:07Z

src/setup.jl

-    if Rhome == ""
-        error(
+    # This should actually error much sooner, but this is just in case
+    isempty(Rhome) && error(


Ha! Mixed feelings about this sneaky hack to pass coverage. The reason there is no coverage here is because when we shell out in installation.jl coverage stops being measured. It's probably not worth tracking it.

Yeah, it's definitely a hack but it's one I'm comfortable with for such "defense in depth" type measures.

frankier · 2024-01-30T18:31:23Z

Thanks for the docs feedback. I just left a couple of small double-checking comments back and then I guess merge time?

frankier marked this pull request as draft August 4, 2023 11:48

frankier force-pushed the preferences-r-installation branch from f909a95 to 20c3306 Compare August 4, 2023 11:51

frankier marked this pull request as ready for review August 4, 2023 12:12

This was referenced Aug 4, 2023

libR.so in nonstandard location #492

Closed

Use with CondaPkg #480

Closed

Using CondaPkg with R Packages JuliaPy/CondaPkg.jl#100

Open

frankier force-pushed the preferences-r-installation branch from 20c3306 to ef63b63 Compare August 14, 2023 07:38

frankier force-pushed the preferences-r-installation branch from ef63b63 to 2ed4c5f Compare August 14, 2023 11:38

frankier marked this pull request as draft August 14, 2023 14:01

frankier force-pushed the preferences-r-installation branch from 30f51d9 to dc6a135 Compare August 14, 2023 15:07

frankier marked this pull request as ready for review August 14, 2023 15:08

palday self-requested a review September 7, 2023 23:33

palday reviewed Sep 14, 2023

View reviewed changes

Project.toml Show resolved Hide resolved

palday reviewed Sep 14, 2023

View reviewed changes

docs/src/installation.md Show resolved Hide resolved

palday reviewed Sep 14, 2023

View reviewed changes

docs/src/installation.md Outdated Show resolved Hide resolved

frankier added 6 commits September 14, 2023 14:37

Get Rhome and libR from preferences when provided

33b6674

Allow installing package without R installation

902c5d6

This is in case the user wants to specify and installation via Preferences.jl after installation. An error message is printed at import time if no installation is available.

Add docs for Preferences based R customization

92ffbab

Only precompile when Rhome is set

ce9756b

Add compat entry for Preferences

ed5ff4f

Add note about current downsides of installation time R configuration

75b836a

frankier force-pushed the preferences-r-installation branch from a3d2404 to 75b836a Compare September 14, 2023 11:41

palday reviewed Sep 14, 2023

View reviewed changes

docs/src/installation.md Outdated Show resolved Hide resolved

palday reviewed Sep 14, 2023

View reviewed changes

docs/src/installation.md Show resolved Hide resolved

frankier added 3 commits January 8, 2024 08:57

Refer to old R installation method as R_HOME-based as shorthand rathe…

2388328

…r than install-time in docs

Add cookbook snippet for usage with CondaPkg

4e686aa

Clarify precompile-abort case and print explanitory message in this case

9e3fd9f

palday and others added 9 commits January 8, 2024 09:44

Merge branch 'master' into preferences-r-installation

f327698

Merge branch 'master' of github.com:JuliaInterop/RCall.jl into pr/fra…

68f02e3

…nkier/496

potential fix for mac CI

3e8e503

try something on windows

0a52d21

patch bump

74b6120

Fix CondaPkg test to activate the environment

d86f4d4

Refer to RCall by uuid in CondaPkg example

6f47c4d

Proof preferences installation docs a bit

5d89f03

Add note about use with CondaPkg and making sure the environment is a…

ac7ade3

…ctivate

palday added 3 commits January 21, 2024 12:49

try forcing build on Windows

bc86527

scope

9d968d0

coverage

c130c78

palday reviewed Jan 22, 2024

View reviewed changes

docs/src/installation.md Outdated Show resolved Hide resolved

palday reviewed Jan 22, 2024

View reviewed changes

docs/src/installation.md Show resolved Hide resolved

palday reviewed Jan 22, 2024

View reviewed changes

docs/src/installation.md Outdated Show resolved Hide resolved

frankier added 2 commits January 30, 2024 20:27

Clarify the CondaPkg cookbook section

01aa738

Clarify the docs on R_HOME's special values

1273105

frankier commented Jan 30, 2024

View reviewed changes

palday approved these changes Jan 30, 2024

View reviewed changes

palday merged commit 31e7859 into JuliaInterop:master Jan 30, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get Rhome and libR from Preferences.jl when provided #496

Get Rhome and libR from Preferences.jl when provided #496

frankier commented Aug 4, 2023

codecov bot commented Aug 14, 2023 •

edited

Loading

frankier commented Aug 14, 2023

frankier commented Aug 14, 2023

palday commented Aug 29, 2023

ParadaCarleton commented Sep 7, 2023

frankier commented Sep 14, 2023

frankier commented Jan 8, 2024

palday commented Jan 8, 2024

frankier commented Jan 21, 2024

frankier commented Jan 21, 2024

palday commented Jan 21, 2024

frankier Jan 30, 2024

palday Jan 30, 2024

frankier Jan 30, 2024

palday Jan 30, 2024

frankier commented Jan 30, 2024

Get Rhome and libR from Preferences.jl when provided #496

Get Rhome and libR from Preferences.jl when provided #496

Conversation

frankier commented Aug 4, 2023

codecov bot commented Aug 14, 2023 • edited Loading

Codecov Report

frankier commented Aug 14, 2023

frankier commented Aug 14, 2023

palday commented Aug 29, 2023

ParadaCarleton commented Sep 7, 2023

frankier commented Sep 14, 2023

frankier commented Jan 8, 2024

palday commented Jan 8, 2024

frankier commented Jan 21, 2024

frankier commented Jan 21, 2024

palday commented Jan 21, 2024

frankier Jan 30, 2024

Choose a reason for hiding this comment

palday Jan 30, 2024

Choose a reason for hiding this comment

frankier Jan 30, 2024

Choose a reason for hiding this comment

palday Jan 30, 2024

Choose a reason for hiding this comment

frankier commented Jan 30, 2024

codecov bot commented Aug 14, 2023 •

edited

Loading