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

Create specialized kernels w/ JDK, JVM args, and environment variables #287

Merged
merged 14 commits into from
Jul 3, 2021

Conversation

rnett
Copy link
Contributor

@rnett rnett commented Jun 28, 2021

Fixes #284.
Fixes #72.

I'd like to note this somewhere in the readme (and that it can be set per-kernel with env in the kernelspec), any preference on where?

I also thought about a few possible further features:

  1. Creating kernels for each detected JDK version on install (presumably in install_user, although I can't find where it gets called on initial install).
  2. Adding a fix-kernelspec-location-like task to create a kernel for a specific JDK.
  3. Adding a kotlin-jupyter specific environment variable that overrides JAVA_HOME, i.e. KOTLIN_JUPYTER_JAVA, incase you want jupyter on a different JDK than say gradle (although that's doable via the kernelspec).

Thoughts on any of them?

@ileasile
Copy link
Member

Hi!
To satisfy CI you may rebase on the latest master
You may mention this feature in Usage section in README

Regarding your questions:

  1. Good idea, it may be done in setup script, I suppose
  2. It's also worth adding
  3. Agree, and it may be even done in this PR.

@rnett
Copy link
Contributor Author

rnett commented Jun 29, 2021

Autodetection is on hold for now since it's non-trivial, I'd probably do it in another PR.

The rest is done, although I did use subprocess.check_output which is from Python 3.1. As far as I can tell you're targeting 3, should I use subprocess.PIPE or is it a non-issue?

@rnett
Copy link
Contributor Author

rnett commented Jun 29, 2021

Also, is there a reason install_user isn't called from the setup script?

@rnett rnett mentioned this pull request Jun 29, 2021
@rnett rnett marked this pull request as draft July 1, 2021 05:21
@rnett
Copy link
Contributor Author

rnett commented Jul 1, 2021

Making this a draft for now, since I was working on #72 and think it's better done as a create_kernel command that can set any of the JDK, JVM args, or environment variables.

@rnett rnett marked this pull request as ready for review July 1, 2021 07:46
@rnett
Copy link
Contributor Author

rnett commented Jul 1, 2021

Went faster than I expected, this fixes #72 too now.

@rnett rnett changed the title Use JAVA_HOME if set Create specialized kernels w/ JDK, JVM args, and environment variables Jul 1, 2021
Copy link
Member

@ileasile ileasile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, @rnett, thank you A LOT!
I have some minor comments. After fixing I'll publish dev packages from your branch to conda and pip so we could test them.

docs/README.md Outdated Show resolved Hide resolved
distrib/kotlin_kernel/constants.py Outdated Show resolved Hide resolved
@ileasile
Copy link
Member

ileasile commented Jul 1, 2021

The rest is done, although I did use subprocess.check_output which is from Python 3.1. As far as I can tell you're targeting 3, should I use subprocess.PIPE or is it a non-issue?

It's ok. I think, 3.4 is the minimal version we should support

@ileasile
Copy link
Member

ileasile commented Jul 2, 2021

@rnett Could you please install and try the kernel built from this PR? If everything is fine, I'll merge it
pip install -i https://test.pypi.org/simple/ kotlin-jupyter-kernel==0.10.0.83.dev1
or
conda install -c jetbrains-dev kotlin-jupyter-kernel==0.10.0.83.dev1

@rnett
Copy link
Contributor Author

rnett commented Jul 2, 2021

These was a typo in the readme, but other than that everything worked. Tested environment variables and kernel creation on Windows and WSL.

@ileasile ileasile merged commit 36ff53e into Kotlin:master Jul 3, 2021
@ileasile
Copy link
Member

ileasile commented Jul 3, 2021

Thank you, Ryan!

ileasile added a commit that referenced this pull request Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting JDK to use Pass args on JVM startup
2 participants