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 #172

Closed
wants to merge 6 commits into from
Closed

Conversation

YannickJadoul
Copy link
Member

Added a 3.8 or 38 for every 3.7 or 37 currently present in master...
Let's see what works; let's see what breaks!

Cfr. #170

Copy link
Member

@mayeut mayeut left a comment

Choose a reason for hiding this comment

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

I started a branch as well, let's keep yours. Tests are failing on linux because dockcross image is not ready yet, maybe skip 3.8 in this test test/06_docker_images/cibuildwheel_test.py:

    utils.cibuildwheel_run(project_dir, add_env={
        'CIBW_MANYLINUX1_X86_64_IMAGE': 'dockcross/manylinux-x64',
        'CIBW_MANYLINUX1_I686_IMAGE': 'dockcross/manylinux-x86',
        'CIBW_SKIP': 'cp38-*',
    })
    expected_wheels = [w for w in utils.expected_wheels('spam', '0.1.0')
                       if '-cp38' not in w]

cibuildwheel/macos.py Outdated Show resolved Hide resolved
@YannickJadoul
Copy link
Member Author

Ah, sorry to be too fast, then; I was just curious to see what would break :)

Thanks for the suggestions! Fixed those just now, but feel free to just push to this branch; I think you should be able to, as maintainer?

@YannickJadoul
Copy link
Member Author

Copy link
Member

@mayeut mayeut left a comment

Choose a reason for hiding this comment

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

Fixed those just now, but feel free to just push to this branch; I think you should be able to, as maintainer?

Yes I should be able to but I'd rather not since you're too fast :)
Let's wait for Appveyor & Azure now, thanks for the links.

@YannickJadoul
Copy link
Member Author

@joerick Just a thought, but this keeps on happening: for Windows, we depend on the CI images being updated, and we have no control over them. For macOS, we manually install the official Python releases; should we perhaps do something similar for Windows?

@Czaki
Copy link
Contributor

Czaki commented Oct 24, 2019

Maybe install python 3.8 with nuget? https://www.nuget.org/packages/python/3.8.0
Like in windows on travis?

@YannickJadoul
Copy link
Member Author

Pffff, I'm not completely convinced of that. It takes more time if there are already Python installations. And then there's choco and nuget and whatelse? Is there any widely-used, semi-default package manager on Windows?

But more importantly, we also don't use homebrew on macOS, but somehow go through the trouble of downloading and installing the official binaries from python.org. I don't fully remember, but there's somehow difference between brew's Python and the official Python version, or what was the reason? Do you perhaps know, @mayeut?
So how's that for Windows?

@Czaki
Copy link
Contributor

Czaki commented Oct 24, 2019

In python docuementation nuget is suggested way to install python for CI. It install reduced version of Python (without upgrade options etc) and without checking i there exist some installation of python in system.

Maybe best option is to wait on introducing new python version by CI provider but add command line option to insert path to user installation. eg. CIBW_WINDOWS_PYTHON_38_PATH. which will be ignored if provider has own installation?

Problem with homebrew is that it compile python against current version of host system. So you cannot create 10.9 compatibile wheel from 10.13 system. And downloading version from python.org allows this.

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Oct 24, 2019

Problem with homebrew is that it compile python against current version of host system.

But so, that's not the case for the Windows binaries from nuget?

Maybe best option is to wait on introducing new python version by CI provider but add command line option to insert path to user installation. eg. CIBW_WINDOWS_PYTHON_38_PATH. which will be ignored if provider has own installation?

I'm not very enthusiast on adding a dozen more options to cibuildwheel, actually. I can maybe be convinced on installing rather than using the CI's version of Python, though. If not for anything else, then at least because it would streamline the code for different CI providers.

@Czaki
Copy link
Contributor

Czaki commented Oct 24, 2019

Form nuget you get binaries, not source code. So there is no compilation.

Moving everything to nuget it force to drop python 3.4.

@YannickJadoul
Copy link
Member Author

Form nuget you get binaries, not source code. So there is no compilation.

The same goes for brew, as far as I know?

@YannickJadoul YannickJadoul force-pushed the python3.8 branch 3 times, most recently from abb792b to 0952aae Compare October 24, 2019 14:47
@YannickJadoul
Copy link
Member Author

Possible temporary solution, since AppVeyor and Azure are taking their time: appveyor/ci#3142 (comment)

@mayeut
Copy link
Member

mayeut commented Oct 24, 2019

I don't fully remember, but there's somehow difference between brew's Python and the official Python version, or what was the reason?

Problem with homebrew is that it compile python against current version of host system. So you cannot create 10.9 compatibile wheel from 10.13 system. And downloading version from python.org allows this.

My 2 cents regarding brew, as @Czaki said, built for a specific system (binaries or built from sources depending on availability of binaries for the specific system), can't build 32/64 bit intel wheels, it's painfully slow (just look at ci and compare the time between python 2 (pre-installed) and python 3 (brew)), there are only 2 formulas, python 2 & 3, installing multiple python 3 versions might be painful. Anyway, that's not the topic here.

In python docuementation nuget is suggested way to install python for CI. It install reduced version of Python (without upgrade options etc) and without checking i there exist some installation of python in system.

https://docs.python.org/3/using/windows.html
Why not use nuget only for unexisting python versions on CI system ?

AppVeyor comes with nuget preinstalled, so nothing to do for the user once cibuildwheel manages this. Travis CI already uses nuget and since choco is available in Azure (don't know about nuget), I don't see why it couldn't work.

@joerick
Copy link
Contributor

joerick commented Oct 25, 2019

Regarding the use of package managers, where possible I'd prefer not to use them in cibuildwheel. There are practical reasons - see the discussion of brew above, but more generally, it's better to use python.org releases, because:

  • if an extension is built against a package manager's version of Python, and doesn't work with python.org, the extension is at fault
  • if an extension is built against python.org Python, and doesn't work with a package manager's Python, it's the package manager's fault.

However, in the Windows on Travis PR, it seems nuget was necessary, since we couldn't figure out a way to install a msi file on CI.

Possible temporary solution, since AppVeyor and Azure are taking their time: appveyor/ci#3142 (comment)

This is a cool solution, for the latest versions of Python. Looking at Python's ftp archive, there are only .exe installers for Python 3.5+. So it would solve the problem for new versions, but we'd still need to rely on Appveyor/Azure's versions for the older ones.

@Czaki
Copy link
Contributor

Czaki commented Oct 25, 2019

The problem with windows installer from python.org is that they are looking if there exist other python installation in the system and not install if found. So you cannot assume that you know the path to python executable. The installers from nuget do not have this property.

Also windows installer are by default with graphical interface.

When cibuildwheel drop python 3.4 support the pep 514 can help with determine python location.

@YannickJadoul
Copy link
Member Author

@mayeut, @joerick Thanks for the refresher on those package managers :-)

The Python docs do advertise nuget as lightweight version for use in CI configurations (as @Czaki noted), but I see your argument against package managers.

I'm just not satisfied with the fact that we seem to have 3 different solutions for all three different platforms (AppVeyor: pre-installed, Azure Pipelines: pre-installed in different locations, and with extra lines in the config; and Travis CI: nuget). And of all these three solutions, the nuget one seems to be the only one already working for this PR, while we're waiting for the pre-installed ones?

@Czaki We should also beware to not make things too complex/complicated either. Querying the Windows registry according to PEP 514 soon becomes a huge mess, I guess.

@Czaki
Copy link
Contributor

Czaki commented Oct 25, 2019

@YannickJadoul If you would like to use standard python installer for windows then you cannot assume that python is under path which you set as installation path. It may end because python is installed under other path.

I meet this problem when I try different approach for using cibuildwheel on travis windows. Best way to found already installed python is PEP 514. But I found that using nuget produce much more clean, and readable code.

@mayeut
Copy link
Member

mayeut commented Nov 1, 2019

It seems to be pre-installed, somehow, downloaded from some image?

I found that but could not figure out where are the scripts that build that cache... Even if it's from an official python install, it won't be found by a PEP514 implementation.

@mayeut
Copy link
Member

mayeut commented Nov 2, 2019

The 4 PR drafts now include python 3.8 on all platforms, and updated readme from this PR. Ready for review.

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Nov 2, 2019

@mayeut Which one do you prefer? And what's the suggestion? To merge one of those PRs and close this? Or to merge your additions into this PR?

@mayeut
Copy link
Member

mayeut commented Nov 2, 2019

I think this one can be closed since all changes have been merged back in the 4 other PRs.
It's up to @joerick to review/merge the one he thinks most fits cibuildwheel I guess. Given his previous comments, I'd say #186 but that might have changed.
My personal preference goes to consistency on all CI so either #180 (nuget on all CIs) or #186 (official install, using PEP514 tools)

@joerick
Copy link
Contributor

joerick commented Nov 2, 2019

Hi both, thank you for this excellent work. I'm away this weekend but I'll try to find time in the next few days to take a proper look and make a decision. What would be your preference @YannickJadoul?

@YannickJadoul
Copy link
Member Author

@joerick As a quick but safe solution to get something working but also compile Python 3.8, I'd go with merging #186. The sooner we could merge that, the better.

Again, I've been thinking that Python 3.8 support should soon by in Azure and AppVeyor, but it seems to still be taking quite long. Even though there are rumours in the microsoft/azure-pipelines-image-generation#1317 discussion that it's rolling out, I haven't been successful in restarting the builds, over the last two days.

On the long run, it would good to figure out if the Nuget installations are official enough to be used by cibuildwheel and think about #180. Doing this might greatly simplify the code that's now using different paths on different Windows CI platforms. But that's still worth a discussion, as I do understand your point of view on non-Python.org versions. But meanwhile, #186 should be harmless to merge soon, I think.

@mayeut, do you agree, or did I miss something?

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Nov 3, 2019

Right, this should work now, I think.

@joerick, yet another suggestion. If you want to take your time deciding what to choose, this current PR pushes the workaround to the appveyor.yml and azure-pipelines.yml, using the solution suggested in appveyor/ci#3142 (comment).

Advantage: No changes to the windows.py code are needed, and once AppVeyor and Azure catch up with the Python release, the workaround is not necessary anymore. All we then need to do is update the README, but no new release needs to be made.
Disadvantage: Users need to add a few lines to their config, before calling cibuildwheel. (Oh yeah, and that very ugly hack on Azure, copying the new installation to the expected location.)

@joerick
Copy link
Contributor

joerick commented Nov 3, 2019

Right, I've looked at all the PRs now!

My feeling is that #180 is my favourite. That solution gives us consistency that we didn't previously have. I hadn't fully considered the provenance of the preinstalled versions we use in CI now. If we have nuget on Travis, Appveyor's version there, Azure doing something different, we're potentially opening ourselves up to a maintenance headache down the road.

I was wondering on the performance hit, but it doesn't seem to make a measurable difference according to our own CI.

CI times Azure Appveyor
#172 this PR 17min 20min
#180 17min 20min
#186 17min 20min

Then there is the question of non-Python.org installer. Well, it's a risk, but I think the reduced complexity is worth it. If it turns out that it creates compatibility problems, at least then we'll know. If that's the case, we can change our mind, revert #180 and adopt #186 instead.

So, any further comments before we move forward with #180?

@mayeut
Copy link
Member

mayeut commented Nov 3, 2019

So, any further comments before we move forward with #180?

Drop python 3.4 #168 ?

@YannickJadoul
Copy link
Member Author

Agreed with both :)
Though it's slightly sad/ironic that now that @mayeut got a nice PEP 514, we're ditching it anyway :-D

@mayeut
Copy link
Member

mayeut commented Nov 3, 2019

Though it's slightly sad/ironic that now that @mayeut got a nice PEP 514, we're ditching it anyway :-

No worries, the goal with the 5 PRs was to get at least one selected to get Python 3.8 ASAP
I also do prefer #180 over #186 but, if at some point compatibility issues arise, as @YannickJadoul said, then #186 it's still an option (I'll keep the branch for reference).

@YannickJadoul
Copy link
Member Author

but, if at some point compatibility issues arise

Perfect! Good to have a quick fallback at hand :)

@joerick
Copy link
Contributor

joerick commented Nov 4, 2019

Closed, because this is included in #180

@joerick joerick closed this Nov 4, 2019
@YannickJadoul YannickJadoul deleted the python3.8 branch November 5, 2019 15:42
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