search

JuliaInterop / JuliaCall

Embed Julia in R
https://non-contradiction.github.io/JuliaCall/index.html
Other
267 stars 36 forks source link

Proposal: Expand allowed paths for specifying sysimage #155

Closed JackDunnNZ closed 3 years ago

JackDunnNZ commented 3 years ago

Support for passing a custom system image path was added in #140, but it requires that the path be specified relative to JULIA_HOME. This might be overly restrictive as it prevents supplying absolute paths, and it is also complicated to supply paths relative to the current directly as would normally be done when starting Julia (i.e. julia -J path/to/sysimage)

This PR changes the handling of the path to match what is done in pyjulia, where you can pass either an absolute path to the image, or a relative path, in which case the path is interpreted as being relative to the current directory. This is also how Julia interprets arguments passed using -J/--sysimage.

It is still easy to pass a path relative to JULIA_HOME by passing paste(JULIA_HOME, relative_sysimage_path, sep = "/") as was done automatically before.

This is technically breaking, but I think is worthwhile as enables a lot more flexibility with passing paths, and it brings consistency with Julia and pyjulia. I also changed the argument name from relative_sysimage_path to sysimage_path to improve clarity since it would no longer be restricted to relative paths.

Non-Contradiction commented 3 years ago

Thanks! I think it is a reasonable breaking change. One more thing to consider is that currently in the code of julia_setup, specifically for Windows, we actually change the current folder to JULIA_HOME in order to load julia dlls. https://github.com/Non-Contradiction/JuliaCall/blob/719b0372312a6f430d5275648e063ab8bfa0265a/R/zzz.R#L103-L115 And I think this setting of the current directory will make the proposed change has no effect on Windows.

However, on Windows, the searching of julia dlls can also be facilitated by appending JULIA_HOME to path: https://github.com/Non-Contradiction/JuliaCall/blob/719b0372312a6f430d5275648e063ab8bfa0265a/R/zzz.R#L224-L227

So I think to make the proposed change effective, we need to only rely on the second mechanism and remove the first one.

JackDunnNZ commented 3 years ago

Thanks for taking a look!

It is actually working for me right now on Windows. I think the reason it doesn't fall into the problem you pointed out is that normalizePath converts any relative path to absolute, before the current directory is changed:

https://github.com/Non-Contradiction/JuliaCall/pull/155/commits/145f11e4ceb74ca4eef64b81bb98bb49a90b1a12#diff-fbc888a8d95ca11d7f17e5eaa27a08a3d3cb183803ae509deea6aa51f61183fbR68-R77

We then pass this img_abs_path through to juliacall_initialize so it shouldn't be affected by changing the current directory later on.

Non-Contradiction commented 3 years ago

Looks good to me. Thank you very much! I guess we may later need some CI test for this.