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

"python bin\kalite" should not switch interpreter unless required. #4315

Closed
MCGallaspy opened this issue Aug 28, 2015 · 12 comments
Closed

"python bin\kalite" should not switch interpreter unless required. #4315

MCGallaspy opened this issue Aug 28, 2015 · 12 comments

Comments

@MCGallaspy
Copy link
Contributor

Currently if the environment variable KALITE_PYTHON is set, then bin\kalite will use that interpreter even if invoked with a different one -- does it make sense to have the interpreter set in this order?

  1. The interpreter it's invoked with
  2. The value of KALITE_PYTHON if it exists
  3. Whatever the system does normally when bin\kalite is executed without an interpreter (using the #! or whatever)

This is an issue on windows -- my personal workflow is to use bin\windows\kalite.bat when I wish to use KALITE_PYTHON, but sometimes (in e.g. a virtualenv) I would like to specify the interpreter.

@66eli77
Copy link
Contributor

66eli77 commented Aug 28, 2015

I feel like my wrongly permanently set KALITE_PYTHON and KALITE_DIR is caused from a very early on Mac kalite installer, since then, I need to unset these environment variables very time.
It might be good to capture the error caused by these variables and do a automatic reset for them.

@MCGallaspy
Copy link
Contributor Author

At least in windows, these variables are set during installation and unset during uninstallation -- can we institute a similar thing for mac, if it doesn't already exist? Otherwise it sounds like you just need to unset these variables for good, or set them to more appropriate values for your system. But yeah, it may be possible to have a graceful fallback if they're not set appropriately. Do you have any suggestions for graceful behavior on error, @66eli77?

@66eli77
Copy link
Contributor

66eli77 commented Aug 28, 2015

if these two variables can be auto reset, I think anything wrong with these two variables should not stop the user from starting kalite.

@66eli77
Copy link
Contributor

66eli77 commented Aug 28, 2015

If they cannot be auto reset, prompt and teach the user how to reset these two variables.

@benjaoming
Copy link
Contributor

Well, the thing is that @jamalex once informed me that there were systems where the default python will point to python3, and thus we are forcing python2 if it's there. I admit that we could be doing other things.

But IMO it all boils down to a case of being too clever. Hence the issue.

@MCGallaspy what's the use case for this btw?

I'd like to remove this behaviour, because I think that systems where python is python3 could potentially just do python2 /usr/bin/kalite.

@MCGallaspy
Copy link
Contributor Author

@66eli77 that's not very specific -- what exactly was wrong with the values you had set? That could clue in to what the best behavior on an error is. Rather than unset an environment variable, which takes control of the environment out of the user's hands, I think it's preferable to show a warning and have some fallback behavior.

@benjaoming the use case I stated was this:

my personal workflow is to use bin\windows\kalite.bat when I wish to use KALITE_PYTHON, but sometimes (in e.g. a virtualenv) I would like to specify the interpreter.

Sometimes I would like to use the interpreter in the environment variable, but sometimes I want to specify it like this and override the value of the environment variable: C:\Some\Path\python.exe bin\kalite foo

@66eli77
Copy link
Contributor

66eli77 commented Aug 31, 2015

@MCGallaspy the fallback behavior I mentioned above is to auto reset the values and not stop the user from starting the server.

@cpauya
Copy link
Contributor

cpauya commented Sep 2, 2015

Hi @66eli77, sorry, yes the KALITE_PYTHON and KALITE_DIR env vars have been set by an earlier version of the Mac installer and wasn't cleaned.

Please check and delete the following files:

  1. /Library/LaunchDaemons/org.learningequality.kalite.plist -- This is the destination of the old installer and is most likely causing your issue.
  2. /Library/LaunchAgents/org.learningequality.kalite.plist -- This is where the current installer puts the .plist file.

You can safely delete any of them because the new OSX installer will re-create it on the /Library/LaunchAgents/ directory.

After deleting, please restart OSX and check if those env vars are still there.

The current installer now provides a way to "clean" these env vars thru the Preferences Window -> Advanced tab -> Reset App button.

Just a note that the OSX app will check the KALITE_PYTHON value if it doesn't exist or the path is invalid.

@rtibbles rtibbles modified the milestones: 0.15.0, 0.16.0 Sep 28, 2015
@rtibbles rtibbles modified the milestones: 0.17.0, 0.16.0 Feb 1, 2016
@benjaoming
Copy link
Contributor

benjaoming commented Jun 13, 2016

I think that these workflow-related requests that can be solved with local scripts and aliases should be done so as far as possible as explained in #4312.

It's clear that this one has become a little more annoying that necessary because of the forced overwriting of the Python interpreter as explained above, and it would be great if we could stop doing that... but I think we're steering the ship clear of any such radical changes from now on.

@benjaoming benjaoming changed the title Request -- "python bin\kalite" uses the invoking interpreter "python bin\kalite" should not switch interpreter unless required. Jun 13, 2016
@benjaoming benjaoming reopened this Jun 13, 2016
@benjaoming
Copy link
Contributor

benjaoming commented Jun 13, 2016

Hmm, if it's really important to get PyPy support, we should fix this......

@benjaoming
Copy link
Contributor

This simple and yet possibly breaking change would be nice to get into 0.17 since we're moving to PEX on the OSX installer anyways.

@benjaoming
Copy link
Contributor

Fixed in 0.17, releasing at the end of October. But work on installers can start now with the prerelease of 0.17b1.

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

No branches or pull requests

6 participants