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

Adding Python 3.8, Always use nuget to install python on Windows #180

Merged
merged 4 commits into from
Nov 5, 2019

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Oct 26, 2019

Just a draft to check if it works on all windows providers.
c.f. #172 for full discussion

@YannickJadoul
Copy link
Member

YannickJadoul commented Oct 30, 2019

EDIT: Argh, sorry, wrong PR. Moved to #184 (comment)

@mayeut mayeut changed the title Always use nuget to install python on Windows Adding Python 3.8, Always use nuget to install python on Windows Nov 2, 2019
@mayeut mayeut marked this pull request as ready for review November 2, 2019 12:11
cibuildwheel/windows.py Outdated Show resolved Hide resolved
Copy link
Member

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! I love the amount of lines that can be removed! :)

README.md Outdated Show resolved Hide resolved
cibuildwheel/windows.py Outdated Show resolved Hide resolved
# no easy and stable way fo installing python 3.4
python_configurations = [c for c in python_configurations if c.version != '2.7.x' and c.version != '3.4.x']
# no easy and stable way fo installing python 3.4
python_configurations = [c for c in python_configurations if c.version != '2.7.x']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still up to date? At least the comment about 3.4 isn't.
Why can't we get nuget 2.7 on Travis?

Copy link
Member Author

@mayeut mayeut Nov 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argh, I forgot I had to drop python 3.4 on this PR (no nuget python 3.4).
The issue on travis with 2.7 is not install python but the tools needed to build native code for python 2.7

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so it might be good to leave merging this PR until after we drop 3.4?

On the 2.7: is it really necessary to keep worrying about these VS tools for 2.7? I'm building C++17 wheels for Python 2.7 (without the ancient compiler VS offers for 2.7). I've tested this and it does work. The only catch is that "Microsoft Visual C++ Redistributable for Visual Studio 2017" needs to be installed, then, so I added a section about that to my docs.

My point being: maybe we could still offer 2.7, but make people aware that the tools won't be installed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, this goes against the generic wheel principles (I think) which is, pip install should always work, no matter what, no need for further installs. My point is that if we want to offer python 2.7 support on travis, then it shall be an opt-in, not the default. This would require a specific PR.
Anyway, python 2.7 is EOL in less than 2 months. Shall we really do specific things for it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, that's also true, yes. And there's still AppVeyor anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so it might be good to leave merging this PR until after we drop 3.4?

yes, if we want to merge it after #186 (official vs nuget). My guess is that in a couple months (after python 2.7/3.4) have been dropped, the only difference between the 2 PRs will be what to install and that, with official installers, we'll still need pep514tools vendored-in. The code in windows.py will be almost the same, whatever the PR.

download('https://dist.nuget.org/win-x86-commandline/latest/nuget.exe', nuget)
# get pip fo this installation which not have.
get_pip_script = 'C:\\get-pip.py'
download('https://bootstrap.pypa.io/get-pip.py', get_pip_script)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: do nuget versions not have pip installed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not redone the test, it's merely a rewrite of the travis-ci PR which was doing this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we hit the conditional statement lower in the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, two lines and a download. Not sure if it's worth testing whether we can remove the lines.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems pip is not installed

Hmmm, two lines and a download. Not sure if it's worth testing whether we can remove the lines.

Not sure I fully understand this comment. If it only refers to get-pip then no, we can't remove those (now tested).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I meant. That's kind of silly. Why do all these distributions not have pip installed?

cibuildwheel/windows.py Show resolved Hide resolved
@mayeut
Copy link
Member Author

mayeut commented Nov 3, 2019

Depends on #168 (sort of), drops python 3.4 on windows.

@joerick
Copy link
Contributor

joerick commented Nov 4, 2019

This is looking good to me! Anything else you'd like to do before merging @mayeut ?

Also replace double-space by a backslash to show intent of a line-break.
@mayeut
Copy link
Member Author

mayeut commented Nov 4, 2019

@joerick, it seemed it was not possible to merge the PR with the merge you did. Instead, I rebased on the new master. Looks good to me, let's wait for CI.

Matt$ diff -ruN -x .git cibuildwheel.bak cibuildwheel
diff -ruN -x .git cibuildwheel.bak/cibuildwheel/windows.py cibuildwheel/cibuildwheel/windows.py
--- cibuildwheel.bak/cibuildwheel/windows.py	2019-11-04 22:00:42.000000000 +0100
+++ cibuildwheel/cibuildwheel/windows.py	2019-11-04 22:07:06.000000000 +0100
@@ -9,9 +9,9 @@
     from pipes import quote as shlex_quote
 
 try:
-    from urllib2 import urlopen
-except ImportError:
     from urllib.request import urlopen
+except ImportError:
+    from urllib2 import urlopen
 
 from .util import prepare_command, get_build_verbosity_extra_flags
 
diff -ruN -x .git cibuildwheel.bak/test/shared/utils.py cibuildwheel/test/shared/utils.py
--- cibuildwheel.bak/test/shared/utils.py	2019-11-04 22:00:42.000000000 +0100
+++ cibuildwheel/test/shared/utils.py	2019-11-04 22:08:34.000000000 +0100
@@ -99,7 +99,7 @@
         raise Exception('unsupported platform')
 
     if IS_WINDOWS_RUNNING_ON_TRAVIS:
-        # Python 2.7 and 3.4 isn't supported on Travis.
+        # Python 2.7 isn't supported on Travis.
         templates = [t for t in templates if '-cp27-' not in t]
 
     return [filename.format(package_name=package_name, package_version=package_version)

@mayeut mayeut force-pushed the nuget branch 2 times, most recently from e82c5f7 to a95e596 Compare November 4, 2019 21:47
@joerick
Copy link
Contributor

joerick commented Nov 4, 2019

Those README updates are okay, I think it will be simpler once @Czaki's PR is merged anyway.

@mayeut
Copy link
Member Author

mayeut commented Nov 4, 2019

The only CI check remaining is AppVeyor which is rebuilding the 4 commits for the README change so it will take around 1h30 to get the all green CI status. (they are not building the commit they claim to be building anyway... otherwise, it would have already failed for the first 3 unexisting commits...)

PS: @joerick, I can cancel/restart jobs on travis-ci but this doesn't seem to be the case on AppVeyor (I wanted to clean-up the build queue but not possible)

@mayeut
Copy link
Member Author

mayeut commented Nov 4, 2019

@joerick, LGTM.

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.

5 participants