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

Final work on startJVM #444

Merged
merged 8 commits into from Jun 1, 2019

Conversation

Projects
None yet
2 participants
@Thrameos
Copy link
Contributor

commented May 24, 2019

Our startJVM has made a lot of progress, but still was not very friendly. It didn't report errors and the exception behavior was not well defined. I have corrected this as much as I could.

@Thrameos Thrameos added this to the JPype 0.7 milestone May 24, 2019

@Thrameos Thrameos requested a review from marscher May 24, 2019

@Thrameos Thrameos self-assigned this May 24, 2019

Thrameos added some commits May 24, 2019


if 'ignoreUnrecognized' not in kwargs:
kwargs['ignoreUnrecognized'] = False
# JVM path

This comment has been minimized.

Copy link
@marscher

marscher May 29, 2019

Collaborator

this voodoo is not necessary with the old signature. If jvm is already passed via the named keyword, passing it again via kwargs will raise a TypeError.

Show resolved Hide resolved jpype/_core.py
Show resolved Hide resolved jpype/_core.py Outdated
@@ -95,48 +95,84 @@ def _hasClassPath(args):

_JVM_started = False

def startJVM(jvm=None, *args, **kwargs):
def startJVM(*args, **kwargs):

This comment has been minimized.

Copy link
@marscher

marscher May 29, 2019

Collaborator

wildcard arguments are a pain! Please keep jvm (also for backward compatibility reasons).

This comment has been minimized.

Copy link
@Thrameos

Thrameos May 29, 2019

Author Contributor

I believe it does the same thing here.

I wanted to be able to support

  # old
   jpype.startJVM(getDefaultJVMPath(), "-ea", '-Djava.class.path=%s'%getClassPath())
  # Better
   jpype.startJVM(getDefaultJVMPath(), '-ea', classpath = getClassPath())
   jpype.startJVM(getDefaultJVMPath(), '-ea')
  #  Simple
   jpype.startJVM('-ea')

This comment has been minimized.

Copy link
@Thrameos

Thrameos May 29, 2019

Author Contributor

Just to be clear jvm as the first argument before the ``*args` meant you had to specify the path even if you just wanted to start a jvm with default. Passing in None there is the alternative, but looked really ugly to me. I can change it back.

Also minor issue.... i think the argument should be jvmpath rather than jvm. I want to reserve jvm for when we actually can start more than one machine.

Show resolved Hide resolved jpype/_core.py Outdated
std::stringstream msg;
msg << "Unable to load DLL [" << path << "], error = " << dlerror();
JP_RAISE_RUNTIME_ERROR( msg.str().c_str());
JP_RAISE_OS_ERROR_UNIX( errno, path);

This comment has been minimized.

Copy link
@marscher

marscher May 29, 2019

Collaborator

on windows you have a human readable string, but it seems like on linux people just see an error code?

This comment has been minimized.

Copy link
@Thrameos

Thrameos May 29, 2019

Author Contributor

I was sort of forced to do something non-standard on Windows because the Python version call for windows reporting is optional. I can do the same thing for linux if you would like.

This comment has been minimized.

Copy link
@Thrameos

Thrameos May 29, 2019

Author Contributor

It looks like they should get symmetric errors

      case JPError::_os_error_unix:
      {
        PyObject* val = Py_BuildValue("(iz)", m_Error, (string("JVM DLL not found: ") + mesg).c_str());
        if (val != NULL)
        {
          PyObject* exc = PyObject_Call(PyExc_OSError, val, NULL);
          Py_DECREF(val);
          if (exc != NULL)
          {
            PyErr_SetObject(PyExc_OSError, exc);
            Py_DECREF(exc);
          }
        }
        return;
      }

      case JPError::_os_error_windows:
      {
        PyObject* val = Py_BuildValue("(izzi)", 2, (string("JVM DLL not found: ") + mesg).c_str(), NULL, m_Error);
        if (val != NULL)
        {
          PyObject* exc = PyObject_Call(PyExc_OSError, val, NULL);
          Py_DECREF(val);
          if (exc != NULL)
          {
            PyErr_SetObject(PyExc_OSError, exc);
            Py_DECREF(exc);
          }
        }
        return;
      }
@marscher
Copy link
Collaborator

left a comment

Thanks for taking a shot at this. Please see the line comments for discussion.
Didn't we agree upon not to ship binaries within git? (jpype jar etc.)

@Thrameos

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Sorry the jars slipped in due to a missing .gitignore statement. I will try to resolve it.

Thrameos added some commits May 29, 2019

@Thrameos

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

I am making some progress on the 0.8 core changes right now, but it is a long path. I want to gut the rest of the JNI layer out of C++ so that all the brains and object clean up lives in Java. This is the easy part. The hard part is to make it work, I need to gut every global variable out of the JPype core. Unfortunately, the core _jpype module is full of globals. However, if I succeed in theory we can restart the JVM or even run more than one JVM at a time.

@Thrameos Thrameos requested a review from marscher May 29, 2019

@Thrameos

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

@marscher Assuming this and the exception pull are good then we are ready for release of 0.7.0.

@Thrameos Thrameos merged commit 5b359f1 into jpype-project:master Jun 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Thrameos Thrameos deleted the Thrameos:start_jvm branch Jun 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.