#5859 Use python string templates instead of ad hoc for classpath in kernelspecs #5981
Conversation
kernel/base/kernel.json
Outdated
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "argv": [ "java", "-jar", "__PATH__", "{connection_file}" ], | |||
There was a problem hiding this comment.
i think this file should be deleted since the base kernel never runs.
kernel/base/kernel_debug.json
Outdated
| @@ -1,5 +1,5 @@ | |||
| { | |||
kernel/build.gradle
Outdated
| @@ -57,7 +57,7 @@ subprojects { | |||
| ext.executeStaticContent = true | |||
| doLast { | |||
| if (executeStaticContent) { | |||
| def kernelJson = file('kernel.json').text.replace('__MAIN_CLASS__', mainClassName) | |||
| def kernelJson = file('kernel.json').text.replace('$MAIN_CLASS', '"' + mainClassName + '"') | |||
There was a problem hiding this comment.
why do we have a variable for main class at all?
since we have a kernel.json file for each kernel and we know the class for each kernel, this substitution is not needed right? just put the class name into the kernel.json.
beakerx/setupbase.py
Outdated
| @@ -323,14 +324,10 @@ def install_kernel(source_kernelspec='', kernelspec_name=None): | |||
| classpath = (os.path.abspath(os.path.join(target_dir, 'base', 'lib', '*')) + (';' if sys.platform == 'win32' else ':') + os.path.abspath(os.path.join(target_dir, name, 'lib', '*'))).replace('\\', '/') | |||
There was a problem hiding this comment.
this should use json.dumps and OS independent logic the same as install.py does.
beakerx/beakerx/install.py
Outdated
| template = pkg_resources.resource_string( | ||
| 'beakerx', os.path.join('static', 'kernel', kernel, 'kernel.json')) | ||
| contents = template.decode().replace('__PATH__', classpath) | ||
| contents = Template(template.decode()).substitute(PATH="\"" + classpath + "\"") |
There was a problem hiding this comment.
this doesn't look right, json.dumps returns a value with quotes already.
beakerx/beakerx/install.py
Outdated
| @@ -32,10 +33,14 @@ def _all_kernels(): | |||
| 'beakerx', os.path.join('static', 'kernel')) | |||
| return [kernel for kernel in kernels if kernel != 'base'] | |||
|
|
|||
| def convert_path(path): | |||
There was a problem hiding this comment.
this should not be needed, that is the main point of this PR.
the reason for converting backslash to forward slash is because the old code was not adding quotes as needed.
but now that we are using json.dumps, it should insert the quotes for us, so we should not need to have special case handling for windows.
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "argv": [ | |||
| "java", "-cp", "__PATH__", "__MAIN_CLASS__", "{connection_file}" | |||
| "java", "-cp", ${PATH}, "com.twosigma.beakerx.groovy.kernel.Groovy", "{connection_file}" | |||
There was a problem hiding this comment.
thanks these look good now
No description provided.