-
Notifications
You must be signed in to change notification settings - Fork 5
Add support for macos launcher #18
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
Conversation
| String extraJavaOptions = String.join(" ", GraalPyRunner.getExtraJavaOptions()); | ||
| if (!IS_WINDOWS) { | ||
| if (IS_MAC) { | ||
| var script = formatMultiline(""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you share this code with the Windows branch? Seems like only this line in the script would need parametrization:
vl = os.path.join(venv.__path__[0], 'scripts', 'macos', 'graalpy')
otherwise it is the same?
On Windows we try to avoid regenerating the launcher to speed up the build:
(!Files.exists(launcherArgs.launcherPath)
|| !checkWinLauncherJavaPath(launcherArgs.launcherPath.getParent().resolve("pyenv.cfg"), java)) {
do you see any issue with doing that on MacOS? checkWinLauncherJavaPath should be renamed, but what it does inside should apply just fine to the MacOS launcher AFAICS.
Up to your consideration: since we now have more conditions, I would find this if-else structure more readable:
if (IS_WINDOWS || IS_MAC_OS) {
if (...condition if we need to regenerate the launcher...) {
...
}
} else {
// we do not bother checking if it exists and has correct java home, since it is
// simple to regenerate the launcher
...
}
|
Please disregard GitHub actions for now, I am working on fixing them |
| } catch (IOException e) { | ||
| throw new IOException("failed to create tmp launcher", e); | ||
| } | ||
| System.out.println("Created temporary launcher file: " + tmp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left-out prinlnt?
…vo/graalpy-extensions into ih/add-support-for-macos-launcher
|
Integration continues in #31 |
No description provided.