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

Updating windows installer for 0.17 #427

Conversation

mrpau-richard
Copy link
Contributor

Summary

  • Use the KA Lite static .whl file.
  • Update the Python version from 2.7.9 to 2.7.11.
  • Set KALITE_PYTHON environment variable after the installation.

Issues encountered.

Having multiprocessing issue when using the python version 2.7.10
temp-1

Issues addressed

Fixes #424

Hi @radinamatic can you test this windows installer for 0.17 release? I built it in my local for now because the build server is still down. Here's the installer.

/cc @cpauya @benjaoming

@mrpau-richard mrpau-richard changed the base branch from master to develop November 16, 2016 13:35
@radinamatic
Copy link
Member

Downloading!

I'll try it on Windows 7, unless you have other preferences...? @mrpau-richard

@radinamatic
Copy link
Member

@mrpau-richard Your installer worked perfectly on W7, no errors or issues whatsoever during the installation, nor in major functions.

All caveats regarding the contentpacks still the same as for the deb installer I tested yesterday.

@benjaoming
Copy link
Contributor

@mrpau-richard great work! Does it mean we have to add Python 2.7.11 as a requirement for Windows environments? We could add it as a release note with an instruction on how to find your python version...

@mrpau-richard
Copy link
Contributor Author

@benjaoming Yeah, We need to add it in the release notes.

@benjaoming
Copy link
Contributor

benjaoming commented Nov 17, 2016

@mrpau-richard I managed to identify the upstream Python issue, it's true that multiprocessing is broken on Windows in < 2.7.11

https://bugs.python.org/issue10128

Moreover, it's related not to m2crypto but to the way that Wheel files are launched on Windows. So basically, it's related to the move to .whl.

But the error can occur at any time a user runs something that was installed with pip using the .whl format.

What's you feeling about this? How many users will be affected? Should we revert back to sdists or embrace the future?

@benjaoming
Copy link
Contributor

benjaoming commented Nov 17, 2016

Allow me to rant :)

Python 2.7.11 is only 11 months old - this makes it slightly sensitive as most users probably haven't upgraded...

However, if someone is already deploying a new version of KA Lite, they should also have access to an updated version of Python. It's within reach of anyone installing KA Lite on Windows to also install Python.

Requiring users to run a newer version of Python also shouldn't do harm, but could come with other benefits as lots of bugs are fixed in each of the 2.7 patch releases.

We may be making our own work much more difficult if we decide to allow users to stay with old versions of Python. Supporting old OS'es is one thing that's already quite a load :)

So in that sense, I would say that we should stick to the changes you've made.. keep on building the Windows installer with wheels and instead require users to read the release notes, where we'll mention that Windows requires a Python update.

@benjaoming
Copy link
Contributor

@mrpau-richard - why not bundle Python 2.7.12 since it's the latest release?

@mrpau-richard
Copy link
Contributor Author

I haven't tested it within python 2.7.12. I will test it tomorrow if I have no issue encountered then we will use it.

@mrpau-richard
Copy link
Contributor Author

Hi @benjaoming & @radinamatic we are now using the python version 2.7.12 in KA Lite windows installer. Here's the installer for testing.

@radinamatic
Copy link
Member

@mrpau-richard Tested the installer on Windows XP this morning, and everything seems a-OK! 👍

One thing I would suggest is to rephrase the wording, since it seems to imply that the Python installation is somehow optional, and not compulsory - users cannot simply skip installation of Python and expect KA Lite to work:

virtualbox_wxp_18_11_2016

Suggested text for the above alert:

KA Lite requires Python to be installed on your system. 
This wizard will proceed to install Python 2.7.12, and then continue with KA Lite installation. 

The proper buttons should be Continue and Cancel, and if Cancel is selected, there should be another alert with the following:

Are you sure you want to cancel the installation of Python 2.7.12? 
If you select "Yes", this installer will close and KA Lite will not be installed on your system. 

@mrpau-richard
Copy link
Contributor Author

Hi @radinamatic done fixing the text for the alert box. Here's the installer

@radinamatic
Copy link
Member

@mrpau-richard Text looks OK, thanks! Two new issues however:

Issue 1:

When user decides not to install Python/KA Lite, and chooses Yes here:

virtualbox_wxp_21_11_2016_cancel

then this Runtime error appears on XP:

virtualbox_wxp_21_11_2016_cancel2

Same on Windows 7:

virtualbox_windows 7_21_11_2016_cancel

Issue 2:

If the user selects No on the alert step/screenshot above, I think the installer should go back to the previous logical step, namely this one, and wait for the user to select OK:

virtualbox_wxp_21_11_2016_accept

Instead, the process remains running in the back and eventually starts the Python installer without the user's explicit acceptance.

@mrpau-richard
Copy link
Contributor Author

Hi @radinamatic I fix the issue you commented above. Here's the installer for another testing.

@radinamatic
Copy link
Member

@mrpau-richard Awesome, no more runtime errors! 👍

I'll give it another cursory glance, and see if I can debug the contentpacks a bit more!

@mrpau-richard
Copy link
Contributor Author

mrpau-richard commented Dec 7, 2016

Hi @radina Can you do a final testing on KA Lite windows installer for 0.17? To merge this PR.
You can download the latest installers at the dungeon server.

I already tested it, on these operating systems Windows 7 32 bit and Windows 8.1 64 bit.

@radinamatic
Copy link
Member

@mrpau-richard Tested the build 59 on both W7 & W8.1 64bits and as far as the fixes in this PR are concerned, merge away! 👍

@mrpau-eduard
Copy link
Contributor

Hi @benjaoming can you take a look on this PR it seems it is ready for merging.

@mrpau-eduard mrpau-eduard added this to the 0.17.0 milestone Dec 9, 2016
@benjaoming benjaoming merged commit 89536d7 into learningequality:develop Dec 12, 2016
@benjaoming
Copy link
Contributor

Skimmed through the code, still looks good, thanks for the tip @mrpau-eduard :)

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.

4 participants