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

upgrade macos version for support c++11 #156

Merged
merged 8 commits into from
Jan 27, 2020
Merged

upgrade macos version for support c++11 #156

merged 8 commits into from
Jan 27, 2020

Conversation

Czaki
Copy link
Contributor

@Czaki Czaki commented Aug 15, 2019

macos below 10.6 do not support c++11 features. Next python version of pkg is 10.9. To not break backward compatibility I suggest to manually force 10.9 version

@YannickJadoul
Copy link
Member

@Czaki How is this the case? I've been building wheels using C++17 with the current version of cibuildwheel. Have I been doing something wrong? Could you explain in slightly more detail what the problem is?

@Czaki
Copy link
Contributor Author

Czaki commented Aug 17, 2019

When I try to setup ci build on azure-pipelines. When building wheels for macos I got this error:

In file included from package/PartSeg/utils/multiscale_opening/mso_bind.cpp:645:
  package/PartSeg/utils/multiscale_opening/mso.h:1:10: fatal error: 'array' file not found
  #include <array>
           ^~~~~~~
  1 warning and 1 error generated.
  error: command 'gcc' failed with exit status 1

Here is pipeline: https://dev.azure.com/PartSeg/PartSeg/_build/results?buildId=74

Here is source code: https://github.com/4DNucleome/PartSeg/tree/cibuildwheel_problem

When I upgrade to 10.9 version everything works. Maybe you have some suggestion how workaround this problem in other way.

@YannickJadoul
Copy link
Member

Hmmm, interesting. I haven't worked with azure pipelines yet, so I don't know. But I'm intrigued how a different Python version solves this problem, so I'd like to have a look indeed!

@Czaki
Copy link
Contributor Author

Czaki commented Aug 18, 2019

This and similar questions:
https://stackoverflow.com/questions/14494513/how-do-i-get-back-c0x-c11-support-for-mac-os-x-10-6-deployment-using-xcode-4
suggest that the problem is not in python, but in this that wheel is build against 10.6 sdk which not support c++11. And changing to build against newer macos sdk fix this. Maybe upgrading also python build is not important. I think that most important is changing identifier.(identifier='cp36-macosx_10_9_intel') etc.

@YannickJadoul
Copy link
Member

Thanks! Maybe I should check the validity of my macOS wheels (built on TravisCI, btw), as well, then!

@Czaki
Copy link
Contributor Author

Czaki commented Aug 18, 2019

It may depend of macos version on virtual machine. Maybe I move part of build to travis, but I prefer to have all three system on one service. In my repository I have configured travis for windows test also. Maybe it can be base for cibuildwheel windows build for travis?

@YannickJadoul
Copy link
Member

It may depend of macos version on virtual machine. Maybe I move part of build to travis, but I prefer to have all three system on one service.

No, I understand. You're completely right: this should work on azure as well! (It's already been 8 years since C++11, c'mon, this should not be an issue!) I'd just love to understand this issue and implications for old builds/wheels, and be able to give more advise in the README :-)

In my repository I have configured travis for windows test also. Maybe it can be base for cibuildwheel windows build for travis?

That would be great! There's already an issue open on this (#144), but I guess no one has taken the time to implement it, yet, since we're still trying to fix the manylinux2010 support (#135 and #155).
But any kind of help is welcome, be it a full PR or just a comment on #144 with a link to your configuration.

@YannickJadoul
Copy link
Member

YannickJadoul commented Aug 28, 2019

Update: I tried installing and importing my own library (which C++17) on a Mac OS X 10.6 installation, and I'm getting the following error:

Screenshot from 2019-08-28 15-27-22

I'm guessing the reason is that Apple was still using libstdc++ back then? I'm curious to give it another try, but so, by default, 10.6 is not really supported anyway, it seems. Although that maybe just using C, things might work? I'll try to give that a go.

At any rate, I also realized how old this platform has become (2009-2011), and apparently it is officially not supported anymore since 2014. I also couldn't install Python 3.7.4 (not sure why, but I got an error), but had to go with 3.7.0. And while installing, I also noticed that numpy's latest version (1.17.1) does not provide a 10.6 wheel anymore (but 1.17.0 still did).

So, maybe it's just time to drop support for 10.6, or make 10.9 the default and provide 10.6 only through an explicit option. @joerick, @mayeut, any thoughts?

@Czaki, I need to still look at what's actually causing the compilation error in your case, but thanks for bringing this to our attention! Are you happy working with your own branch, for the moment, or should we strive to get a short-term solution as well?

EDIT:

This seems interesting: https://trac.macports.org/wiki/LibcxxOnOlderSystems. So that's why 10.9 is the next important version, because it uses libc++ instead of libstdc++? But why doesn't delocate solve this?

@Czaki
Copy link
Contributor Author

Czaki commented Aug 28, 2019

I can work with my branch, but I would like to not need port future changes to my repository.

I think that putting libstdc++ inside wheel is risky strategy.

@YannickJadoul
Copy link
Member

I can work with my branch, but I would like to not need port future changes to my repository.

No, sure, this needs to get solved. But then we don't need a temporary solution too soon.

I think that putting libstdc++ inside wheel is risky strategy.

Yeah, probably. Though I am already statically linking libstdc++ to my manylinux1 wheels, since C++14 is not supported on there. So I'm just curious what would happen...

@Czaki
Copy link
Contributor Author

Czaki commented Aug 29, 2019

Thanks.
I`m not sure but maybe solve statistical linking on Linux should be connected with manylinux2010.

How I can got old MacOs images for testing purpose? I'm not very familiar with envirornment.

@YannickJadoul
Copy link
Member

YannickJadoul commented Aug 29, 2019

How I can got old MacOs images for testing purpose? I'm not very familiar with envirornment.

We still had some old Mac around at the lab, that I could use (and apparently Apple still sells images). Since OS X is only allowed to run on Apple hardware, ... which is amazingly annoying, but apparently Apple thinks that's more important than allowing developers to easily target their platform. (That being said, I'm sure there are images and tutorials around, if you insist?)

@joerick
Copy link
Contributor

joerick commented Sep 9, 2019

So, maybe it's just time to drop support for 10.6, or make 10.9 the default and provide 10.6 only through an explicit option.

I tend to agree, but I wasn't sure... Using my favourite new toy I crunched some numbers on the OS version of PyPI downloads-

SELECT
  COUNT(*) AS num_downloads,
  REGEXP_EXTRACT(details.distro.version, r"^(\d+.\d+)") as system_version
FROM `the-psf.pypi.downloads*`
WHERE
  details.system.name = 'Darwin' AND
  _TABLE_SUFFIX
    BETWEEN FORMAT_DATE(
      '%Y%m%d', DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY))
    AND FORMAT_DATE('%Y%m%d', CURRENT_DATE())
GROUP BY system_version
ORDER BY num_downloads DESC
Row num_downloads system_version  
1 36722694 10.14  
2 15164382 10.13  
3 1840143 10.12  
4 588444 10.11  
5 412806 10.15  
6 167678 10.10  
7 38686 10.9  
8 25143 null  
9 3957 10.6  
10 2077 12.4  
11 2041 10.7  
12 1896 10.16  
13 1738 10.8  
14 326 10.5

We see that pre-10.9 versions of macOS make up.... less than 0.02% of the downloaders :)

So, based on this, I'd be happy just to drop 10.6 altogether. If we get a couple separate requests for an option to add it back, we could add an option for 10.6 then.... but I suspect we won't!

@joerick
Copy link
Contributor

joerick commented Sep 9, 2019

If @YannickJadoul is happy to go with this, @Czaki could you remove the references to the 10.6 pkgs from macos.py?

@YannickJadoul since this includes build identifier changes, it would be great to use the same CIBW_BUILD/CIBW_SKIP migration stuff that we used in the manylinux1->manylinux transition. So let's plan to land this after the manylinux2010 PR is merged.

@Czaki Czaki force-pushed the master branch 2 times, most recently from f2b1c08 to 13f1bda Compare September 9, 2019 21:45
@Czaki
Copy link
Contributor Author

Czaki commented Sep 9, 2019

I fail with all test and there is no option for upgrade python 3.4 and 3.5 to macosx10.9. There is no such builds on https://www.python.org/downloads/mac-osx/. I do not know what now. There are also needs for small fix in readme. Do this?

@joerick
Copy link
Contributor

joerick commented Sep 10, 2019

ah, that changes things a bit... python3.5 is still used quite a bit on mac.

SELECT
  COUNT(*) AS num_downloads,
  REGEXP_EXTRACT(details.python, r"^(\d+\.\d+)") as python_version
FROM `the-psf.pypi.downloads*`
WHERE
  details.system.name = 'Darwin' AND
  _TABLE_SUFFIX
    BETWEEN FORMAT_DATE(
      '%Y%m%d', DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY))
    AND FORMAT_DATE('%Y%m%d', CURRENT_DATE())
GROUP BY python_version
ORDER BY num_downloads DESC

image

Regardless, I think that your current solution makes the most sense - use the 10.9 version if it's available.

It does make the build identifiers a little tricky, but really this just highlights our need to have a big list of them available in the readme. That's a docs concern, I can have a look at that later. As for the build identifier migration warnings, we could add a warning on the string *-macosx_10_6_intel - suggest that it is changed to *-macosx_*.

Happy for this to merge @YannickJadoul ?

@YannickJadoul
Copy link
Member

@joerick Hmmm, I'm not sure. I'm a bit annoyed that I still don't fully understand what's going on here. Because, as I mentioned before, I can perfectly build wheels with C++17 with the current cibuildwheel. (Though they don't seem to work, because libc++ is not available on 10.6, but that's a different problem.)

I was still planning to research further, if I get the time (but there's a few conferences going on this month). However, if you're happy merging this, go ahead, and I'll catch up later and make a new PR if I figure out what's going on :-)

@Czaki
Copy link
Contributor Author

Czaki commented Sep 10, 2019

@YannickJadoul The problem with building is not targeting for macos 10.6, but wrong environment. It looks that on azure (i do not have experience from other ci for this case) when targeting build for macos 10.6 it have only access to this level environment. So in this case two fixes are possible. Upgrade build target to have access to c++11 headers (and latest) or modify environment with installing this dependencies in it. I have only access to one macos device and cannot use it for such experiments. My experience with this system is to low to write it without many tests.

@YannickJadoul
Copy link
Member

@Czaki Yeah, I can see the environment of the CI matters, but I'm very baffled how the installation of just Python inside of cibuildwheel affects the compiler/environment :-(
Do these other versions of Python (for 10.9) then come with new C++ stdlib headers?

@Czaki
Copy link
Contributor Author

Czaki commented Sep 10, 2019

Maybe it is connected with compilers. If I good understand it is need to pass minimum sdk version to it. And basing on it it decides which headers are available?
https://stackoverflow.com/questions/25352389/what-is-the-difference-between-macosx-deployment-target-and-mmacosx-version-min/25362535

@YannickJadoul
Copy link
Member

Oh, right, that's an interesting point!

@Czaki
Copy link
Contributor Author

Czaki commented Sep 15, 2019

I think that I found the solution. The MACOSX_DEPLOYMENT_TARGET was the key. But there is bug in wheel package pypa/wheel#312 and it not bump platform tag when compile against newer platform. I will write the patch and create pull request, but it may take a time. This is explanation of @YannickJadoul case of creating c++17 wheel.

Now I use bypass by overwriting platform tag manually.

I also add test for support of c++ standards. (#165)

This changes are also tested on windows on travis from #160.

@Czaki Czaki force-pushed the master branch 2 times, most recently from ad7edc0 to a00b666 Compare September 15, 2019 22:27
@Czaki
Copy link
Contributor Author

Czaki commented Sep 16, 2019

On AppVeyor there is a problem. One can choose image with Visual C++ Compiler for Python 2.7 or newer MSVC version (https://www.appveyor.com/docs/windows-images-software/)
obraz

I do not know how to solve this problem. Create some array? Skip test base on "APPVEYOR_BUILD_WORKER_IMAGE" ?

@YannickJadoul
Copy link
Member

Argh. Mind if I rebase and force-push, @Czaki?

Also, no new on a wheel release? :-(

@Czaki
Copy link
Contributor Author

Czaki commented Jan 16, 2020

I do not know. It is merged to wheel master. But yet no release.

@YannickJadoul
Copy link
Member

Good news: pypa/wheel#328 (comment)

@Czaki
Copy link
Contributor Author

Czaki commented Jan 27, 2020

@joerick @YannickJadoul
Today is that day https://github.com/pypa/wheel/releases/tag/0.34.0

@YannickJadoul
Copy link
Member

HURRAAAAAAY!

MERGE! MERGE! MERGE!

@Czaki
Copy link
Contributor Author

Czaki commented Jan 27, 2020

NVM my bad during code read.
@YannickJadoul

@joerick joerick merged commit 379cf2d into pypa:master Jan 27, 2020
@joerick
Copy link
Contributor

joerick commented Jan 27, 2020

KAPOW! 👏 👏 👏 Congrats @Czaki and @YannickJadoul ! 👏 👏 👏 Only 5 months in the making!

@joerick
Copy link
Contributor

joerick commented Jan 27, 2020

Now to figure out our strategy for the next release! #220 is a must, #194, #185 and #230 would be great too, and we should get in #212 while we can :)

@YannickJadoul
Copy link
Member

Let's see what can be rebased :-)

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.

7 participants