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

Added java.exe as alternate binary name for Java #409

Merged
merged 6 commits into from May 27, 2013

Conversation

Projects
None yet
3 participants
@dougalg
Contributor

dougalg commented May 23, 2013

2 Commits:

  1. Fix a bug where setting JAVA_HOME and/or JAVAHOME in Windows does not find the java executable (add java.exe as an alternate name for java).
  2. environment vars were not split and looped

dougalg added some commits May 23, 2013

Fixed path parsing bug
The ENV_VARS paths were not split and looped, so if a user specified
multiple directories in a single path variable, then they would be
incorrectly processed.

Fix: split the ENV_VAR on os.pathsep, loop it and then os.path.join() it
with the filename to create the possible file location.
@stevenbird

This comment has been minimized.

Show comment
Hide comment
@stevenbird

stevenbird May 23, 2013

Member

Won't this modification to internals.py break on non-Windows platforms?

Member

stevenbird commented May 23, 2013

Won't this modification to internals.py break on non-Windows platforms?

@dougalg

This comment has been minimized.

Show comment
Hide comment
@dougalg

dougalg May 23, 2013

Contributor

No because now it searches first for 'java', and if it is found, sets it as the path, if not found, searches for the 'alternative' name 'java.exe'.

Unless you meant something else?

To be fair, I'd like a unix/mac user to test it as well. I couldn't get the JDK to install on my mac, so I've not yet been able to test it on a non-windows environment, but I ran a test against the function and it showed it checking for both 'java' and 'java.exe'.

Contributor

dougalg commented May 23, 2013

No because now it searches first for 'java', and if it is found, sets it as the path, if not found, searches for the 'alternative' name 'java.exe'.

Unless you meant something else?

To be fair, I'd like a unix/mac user to test it as well. I couldn't get the JDK to install on my mac, so I've not yet been able to test it on a non-windows environment, but I ran a test against the function and it showed it checking for both 'java' and 'java.exe'.

@dougalg

This comment has been minimized.

Show comment
Hide comment
@dougalg

dougalg May 24, 2013

Contributor

Actually, while this doesn't break anything in other environments, a more full fix would be to also include 'java' in the binary_names variable so that that will be searched for by find_file. The reason is that for some reason while find_file() searches the JAVAHOME paths for BOTH filename and file_names, it only uses the 'which' command to search for file_names, but not filename.

This is easy to fix, and I will submit another commit and pull request unless there is a reason that it won't work that I am not aware of.

Contributor

dougalg commented May 24, 2013

Actually, while this doesn't break anything in other environments, a more full fix would be to also include 'java' in the binary_names variable so that that will be searched for by find_file. The reason is that for some reason while find_file() searches the JAVAHOME paths for BOTH filename and file_names, it only uses the 'which' command to search for file_names, but not filename.

This is easy to fix, and I will submit another commit and pull request unless there is a reason that it won't work that I am not aware of.

Updated internals.py to make find_file consistant
find_file now consistently searches for the bin name and the alternative file_names
@@ -90,7 +91,7 @@ def config_java(bin=None, options=None, verbose=True):
:type options: list(str)
"""
global _java_bin, _java_options
_java_bin = find_binary('java', bin, env_vars=['JAVAHOME', 'JAVA_HOME'], verbose=verbose)
_java_bin = find_binary('java', bin, env_vars=['JAVAHOME', 'JAVA_HOME'], verbose=verbose, binary_names=['java.exe'])

This comment has been minimized.

@dougalg

dougalg May 24, 2013

Contributor

This adds java.exe as a second optional binary name to search for

@dougalg

dougalg May 24, 2013

Contributor

This adds java.exe as a second optional binary name to search for

Show outdated Hide outdated nltk/internals.py Outdated
if verbose: print('[Found %s: %s]' % (filename, path_to_file))
return path_to_file
else:
for env_dir in os.environ[env_var].split(os.pathsep):

This comment has been minimized.

@dougalg

dougalg May 24, 2013

Contributor

Often environment variables contain multiple directories. They should all be searched, but first they need to be split based on the correct path separator (usually : or ;)

@dougalg

dougalg May 24, 2013

Contributor

Often environment variables contain multiple directories. They should all be searched, but first they need to be split based on the correct path separator (usually : or ;)

Show outdated Hide outdated nltk/internals.py Outdated
Show outdated Hide outdated nltk/internals.py Outdated
# Check environment variables
for env_var in env_vars:
if env_var in os.environ:
path_to_file = os.environ[env_var]
if os.path.isfile(path_to_file):

This comment has been minimized.

@kmike

kmike May 25, 2013

Member

This removes support for passing full path to binary in an environment variable, so this is also backwards-incompatible.

@kmike

kmike May 25, 2013

Member

This removes support for passing full path to binary in an environment variable, so this is also backwards-incompatible.

@kmike

This comment has been minimized.

Show comment
Hide comment
@kmike

kmike May 25, 2013

Member

I like the idea behind these changes, and I agree that we should add java.exe as a secondary option, but some of these changes are backwards-incompatible and it is better to understand what are we breaking and for what reason.

Member

kmike commented May 25, 2013

I like the idea behind these changes, and I agree that we should add java.exe as a secondary option, but some of these changes are backwards-incompatible and it is better to understand what are we breaking and for what reason.

@stevenbird

This comment has been minimized.

Show comment
Hide comment
@stevenbird

stevenbird May 26, 2013

Member

Thanks @dougalg for the contributions, and @kmike for the feedback.

Member

stevenbird commented May 26, 2013

Thanks @dougalg for the contributions, and @kmike for the feedback.

Restoring backwards-compatibility
Based on comments from @kmike

Also added extra comments for clarity
@dougalg

This comment has been minimized.

Show comment
Hide comment
@dougalg

dougalg May 26, 2013

Contributor

@kmike, thank you for the extensive comments! I've restored the backwards compatibility and added your suggested modifications.

Contributor

dougalg commented May 26, 2013

@kmike, thank you for the extensive comments! I've restored the backwards compatibility and added your suggested modifications.

Show outdated Hide outdated nltk/internals.py Outdated
Show outdated Hide outdated nltk/internals.py Outdated
Show outdated Hide outdated nltk/internals.py Outdated
Show outdated Hide outdated nltk/internals.py Outdated
@dougalg

This comment has been minimized.

Show comment
Hide comment
@dougalg

dougalg May 27, 2013

Contributor

Thank you. Sorry about all the broken backwards compatibility, this has been a good lesson for me.

Contributor

dougalg commented May 27, 2013

Thank you. Sorry about all the broken backwards compatibility, this has been a good lesson for me.

@kmike

This comment has been minimized.

Show comment
Hide comment
@kmike

kmike May 27, 2013

Member

The code looks good, and it works for me on Mac OS X, so merging it.

Thanks for contributing! I hope the process was not too tedious and it won't discourage you from contributing in future.

Member

kmike commented May 27, 2013

The code looks good, and it works for me on Mac OS X, so merging it.

Thanks for contributing! I hope the process was not too tedious and it won't discourage you from contributing in future.

kmike added a commit that referenced this pull request May 27, 2013

Merge pull request #409 from dougalg/master
Added java.exe as alternate binary name for Java

@kmike kmike merged commit 3b0e8ee into nltk:master May 27, 2013

@dougalg

This comment has been minimized.

Show comment
Hide comment
@dougalg

dougalg May 28, 2013

Contributor

Absolutely not, if I can find a bug I can fix, I'd like to contribute more. This is very helpful for me, since I've never had a chance to work on a collaborative project before. I'm just glad I was able to help a bit!

Contributor

dougalg commented May 28, 2013

Absolutely not, if I can find a bug I can fix, I'd like to contribute more. This is very helpful for me, since I've never had a chance to work on a collaborative project before. I'm just glad I was able to help a bit!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment