lektor / lektor-website

The main lektor website.
https://www.getlektor.com/
Other
160 stars 134 forks source link

Update installer.py to fix an issue when installing on Windows with a… #307

Closed JVanloofsvelt closed 3 years ago

JVanloofsvelt commented 4 years ago

Update installer.py to fix an issue when installing on Windows with an older Python distrubution.

https://github.com/lektor/lektor/issues/808 https://github.com/lektor/lektor-website/issues/306

goanpeca commented 4 years ago

Hi @JVanloofsvelt thanks for the PR!

Left one question, otherwise looks good.

Any chance of adding a test for this ?

Thanks again!

goanpeca commented 4 years ago

Should be good to go?

runfalk commented 4 years ago

I have no Windows computer to test this with but the changes themselves look good.

JVanloofsvelt commented 4 years ago

I did run it on my environment. Python 3.5 on Windows 10, and it ran without a problem. The 'try ... except' and the 'IS_WIN' check makes it unlikely to cause problems.

I also tested it with Python 2.7. The strategy used there is 'use_virtualenv' ('use_venv' is skipped since it doesn't exist in Python 2.7). If the virtualenv module (from site-packages) is up to date, then the installation will run as expected. If it is not up to date, then the pip version used will not recognise the option '--prefer-binary'. The output mentions the unrecognised switch, but finishes printing "All done!", although the installation failed. It would be better if the script informed the user that he needs to upgrade the virtualenv module, or if the script then omitted '--prefer-binary', or if the user is given the choice to omit that switch or not.

Nevertheless, this pull request stands on its own. It's an improvement and it can be merged already. At least on Python 3.5 the user can now update virtualenv if needed (the same was already possible on Python 2.7), whereas before he needed to modify the installer script in order for it to skip the 'use_venv' strategy.

JVanloofsvelt commented 4 years ago

I now understand that omitting '--prefer-binary' is probably not possible for Windows (and it is only used on Windows). I could try extending this change so that it also checks the version of pip that's bundled with virtualenv, so that 'use_virtualenv' too can be skipped if it's an older version that doesn't support the pip switch '--prefer-binary'. Then the last strategy would be used: to download a more recent virtualenv version.

Here is a gist of what that extra change would be (but it needs a little cleaning up): https://gist.github.com/JVanloofsvelt/860c0cfcfea9576897cc49cefc75fb01/revisions

Let me know if you want me to update the pull request @runfalk @goanpeca

andoresuperesu commented 4 years ago

I'm able to test it on 4 Windows 10 PC's. I'll come back and report. Would there be specific python versions you'd want me to test?

runfalk commented 4 years ago

Sweet. I don't think we need to test more than 3.8 (maybe 2.7).

I also thought of an alternative way to fix this I think. We update the pip version using pip install --upgrade pip setuptools if the version is too old (and the user is on Windows).

JVanloofsvelt commented 4 years ago

@runfalk you mean, once the virtualenv is created and pip is installed in it, you would check the pip version and update it, correct? Maybe that's a simpler more intuitive solution, yes. Will you create a pull request for this change then?

runfalk commented 4 years ago

@JVanloofsvelt I think this is OK as it is. I just wanted to document this alternative solution to it.

Since I don't have access to Windows I'd rather not try my hands on this.

JVanloofsvelt commented 4 years ago

I have added one more commit to my branch. Now 'use_virtualenv' may also be skipped (if the bundled pip version < 18.0), so that a more recent virtualenv can be downloaded automatically (in a temporary folder, not overwriting the site-packages). With this change the Windows user doesn't run into an error (after which he would need to upgrade his virtualenv manually, and rerun the installer).

@runfalk @goanpeca please review again.

@andresperezcera With regard to testing, I think this should cover it:

yagebu commented 3 years ago

Closing, the installer script will be deprecated soon