Skip to content

Add test forcing newer MSVC/VC++ compilers supporting modern C++ in Python 2.7/3.5 #333

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

Merged
merged 10 commits into from
May 4, 2020

Conversation

YannickJadoul
Copy link
Member

@YannickJadoul YannickJadoul force-pushed the win-python2.7-c++11 branch 2 times, most recently from aee1b59 to 95ac408 Compare May 3, 2020 20:57
…SDK and MSSdk to not use vcpython27 or MSVC 10
@YannickJadoul YannickJadoul force-pushed the win-python2.7-c++11 branch 3 times, most recently from 450fb5b to 832e0d2 Compare May 3, 2020 22:52
@YannickJadoul YannickJadoul force-pushed the win-python2.7-c++11 branch from 8d10ec5 to b71f514 Compare May 3, 2020 23:34
@YannickJadoul
Copy link
Member Author

I don't know why I'm doing this, nor why I'm doing this to myself, but it seems to (finally) be working! 😮

There are a few thing I'd still like to check, then I'll add some extra docs, and then we can discuss if this is actually worth including (given that Python 2.7 and 3.5 are reasonably dead; but then PyPy 2.7 isn't, so ...)

@YannickJadoul YannickJadoul changed the title Adapt modern C++ tests to use newer MSVC/VC++ compilers Add test forcing newer MSVC/VC++ compilers supporting modern C++ in Python 2.7/3.5 May 3, 2020
@YannickJadoul YannickJadoul mentioned this pull request May 3, 2020
@YannickJadoul YannickJadoul force-pushed the win-python2.7-c++11 branch from ba92364 to c7173b2 Compare May 4, 2020 13:09
@YannickJadoul
Copy link
Member Author

YannickJadoul commented May 4, 2020

So, @Czaki: Python 3.5 perfectly builds C++17 code (agreeing with what https://wiki.python.org/moin/WindowsCompilers says). Do you remember why you skipped this, in #156? The comment that it uses MSVC 10 does not seem to be correct.

(EDIT: GH Actions seems to be confused, possibly by the amended commit where I forgot to update the tests. Azure shows that it works, though.)

@Czaki
Copy link
Contributor

Czaki commented May 4, 2020

I do not remember.

@YannickJadoul
Copy link
Member Author

I do not remember.

Alright. Let's see if this works, then.
Should we still test this on AppVeyor as well, to be sure? Currently this test is skipped to speed up AppVeyor.

@YannickJadoul
Copy link
Member Author

YannickJadoul commented May 4, 2020

I do not remember.

Found this comment in #185: #185 (comment) So I do not really understand, because cp35 and pp36 both seem to be passing now?

@YannickJadoul YannickJadoul force-pushed the win-python2.7-c++11 branch from 4dd3b3a to 14c54ff Compare May 4, 2020 14:36
@Czaki
Copy link
Contributor

Czaki commented May 4, 2020

If I good remember. MSVC 14 is minimal version. extension can be compiled with more modern ones. C++11 is supported from MSVC 2015
https://docs.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance?view=vs-2019

So maybe configuration of CI provider changed? Or some update of setuptools change default compilers used by python 3.5.

As you see in pointed comment. MSVC 14 is invoked and it does not support C+11/14/17.

@YannickJadoul YannickJadoul force-pushed the win-python2.7-c++11 branch from 22c8f41 to 2d75c3d Compare May 4, 2020 15:47
@YannickJadoul
Copy link
Member Author

If I good remember. MSVC 14 is minimal version. extension can be compiled with more modern ones. C++11 is supported from MSVC 2015
https://docs.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance?view=vs-2019

So maybe configuration of CI provider changed? Or some update of setuptools change default compilers used by python 3.5.

As you see in pointed comment. MSVC 14 is invoked and it does not support C+11/14/17.

Ah, I see the confusion. But MSVC 14 corresponds to Visual Studio 2015, 2017, and 2019: https://en.wikipedia.org/wiki/Microsoft_Visual_C%2B%2B#Internal_version_numbering. Microsoft's versioning isn't much clearer/less messy than Apple/macOS/Xcode's, it seems. They even skipped MSVC 13, for sóme reason (I guess MSVC ánd number 13 would really be a double nightmare?).

Anyway, I think that everything built with MSVC 14 could be compatible and that things built with VS 2019 are still compatible with VS 2015 (there's just one redistributable: https://support.microsoft.com/en-us/help/2977003/the-latest-supported-visual-c-downloads), so that would explain why Python 3.5 can be built with VS 2019.

Does that make sense? That would explains why things now work, right?
I still don't understand why CPython 3.5 and PyPy 3.6 failed before, but maybe something else was wrong.

@Czaki
Copy link
Contributor

Czaki commented May 4, 2020

I still don't understand why CPython 3.5 and PyPy 3.6 failed before, but maybe something else was wrong.

Nice that it works now.

@YannickJadoul
Copy link
Member Author

I still don't understand why CPython 3.5 and PyPy 3.6 failed before, but maybe something else was wrong.

Nice that it works now.

That's true, yes :-) I just wish I understood what went wrong before. But maybe I should just be happy that it now works!

@YannickJadoul YannickJadoul requested a review from joerick May 4, 2020 16:17
@YannickJadoul
Copy link
Member Author

@Czaki, @henryiii, I don't know how to invite you through GitHub to review this, but I'd be grateful if you have a bit of time to have a look. @henryiii, particularly at the addition to the docs: is this clear enough, or do you have suggestions to make it better?

@Czaki
Copy link
Contributor

Czaki commented May 4, 2020

Looks ok for me.

@henryiii
Copy link
Contributor

henryiii commented May 4, 2020

Looks good to me, thanks!

@YannickJadoul
Copy link
Member Author

YannickJadoul commented May 4, 2020

Thanks for checking! :-)

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Well, I must say, good job. It sure looks like you enjoy fighting Windows toolchains @YannickJadoul ! :P Also the docs are good and clear 👍

@mayeut
Copy link
Member

mayeut commented May 4, 2020

I still don't understand why CPython 3.5 and PyPy 3.6 failed before, but maybe something else was wrong.

Nice that it works now.

That's true, yes :-) I just wish I understood what went wrong before. But maybe I should just be happy that it now works!

I'm almost sure it has to do with setuptools.
When trying to get GitHub Actions working, the toolchain was not detected properly on python 3.5 so I pushed a PR to setuptools so that for Python 3.5+, it always uses the same discovery mechanism as Python 3.8
Before that, the discovery mechanism only found VS2015 even when 2017/2019 were installed.
The mechanism in python 3.8 returns the most recent one.

@YannickJadoul
Copy link
Member Author

Well, I must say, good job. It sure looks like you enjoy fighting Windows toolchains @YannickJadoul ! :P Also the docs are good and clear +1

Pffffffffffffffffffffffffffffffffffffffffffffff, sure ;-)

I'm almost sure it has to do with setuptools.
When trying to get GitHub Actions working, the toolchain was not detected properly on python 3.5 so I pushed a PR to setuptools so that for Python 3.5+, it always uses the same discovery mechanism as Python 3.8
Before that, the discovery mechanism only found VS2015 even when 2017/2019 were installed.
The mechanism in python 3.8 returns the most recent one.

Aaaah, right, amazing! You're a hero! :-) Thanks for the explanation; now I can sleep again ;-)

@YannickJadoul
Copy link
Member Author

Well, if everyone's happy, I'm merge-merge-merging! Thanks for all the (really necessary) sanity checks, everyone!

@YannickJadoul YannickJadoul merged commit 185a1c0 into pypa:master May 4, 2020
@YannickJadoul YannickJadoul deleted the win-python2.7-c++11 branch May 4, 2020 21:40
@henryiii
Copy link
Contributor

I take it this is not in a released version? I was trying to convert the rest of my build to cibuildwheel and this was the one part that still wasn't building; I t think it might be if there was a change since 1.4.1 (I should of known when I had to change the docs off of stable to read the suggestion, but I thought it was just a docs change and not a code change).

By the way, does this look about right? I was not able to figure out how to use the Github Action to select 32 bit vs. 64 bit.

https://github.com/scikit-hep/boost-histogram/blob/90ce1eb7afe8bef6d8e46d61b2ed83448f1e154d/.github/workflows/wheels.yml#L78-L119

@henryiii
Copy link
Contributor

Still doesn't work, I get this error:

    File "c:\cibw\python\python2x86.2.7.18\tools\lib\distutils\msvc9compiler.py", line 473, in compile
3260
      self.initialize()
3261
    File "c:\users\runner~1\appdata\local\temp\pip-build-env-3nf95o\overlay\Lib\site-packages\numpy\distutils\msvccompiler.py", line 53, in initialize
3262
      os.environ['lib'] = _merge(environ_lib, os.environ['lib'])
3263
    File "c:\cibw\python\python2x86.2.7.18\tools\lib\os.py", line 425, in __getitem__
3264
      return self.data[key.upper()]
3265
  KeyError: 'LIB'

Any ideas? I'll try to compare it to what I manually do tomorrow.

@henryiii
Copy link
Contributor

Ahhhh, didn't know powershell used dot sourcing like bash! Switched to cmd and used call and it made it past this, so that was the problem. I might suggest some docs updates later. @YannickJadoul do you know if there's a way to improve it with the GH Action you (I assume) mention in the docs? Would like to remove the manual path.

@YannickJadoul
Copy link
Member Author

Sorry for the delay, @henryiii!

I suppose you have figured this out by now, but this PR did not add anything to cibuildwheel, but just adds the workaround to the docs and adds tests for it.

Ahhhh, didn't know powershell used dot sourcing like bash! Switched to cmd and used call and it made it past this, so that was the problem. I might suggest some docs updates later.

Great, please do! I haven't really needed this in a project myself, yet (since my setup.py merely runs CMake), so I don't know how clear the instructions are and if there are things to still look out for.

@YannickJadoul do you know if there's a way to improve it with the GH Action you (I assume) mention in the docs? Would like to remove the manual path.

Not really, actually. I merely encountered this while researching the vcvarsall.bat location and thought I'd mention it. Is it not possible to use the action for this?

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