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

Proposal: Expand allowed paths for specifying sysimage #155

Merged
merged 1 commit into from
Jan 17, 2021

Conversation

JackDunnNZ
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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:

145f11e#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
Copy link
Collaborator

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

@Non-Contradiction Non-Contradiction merged commit 630b9ee into JuliaInterop:master Jan 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants