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

Added minimum compiler version assertions #727

Merged
merged 1 commit into from
Mar 19, 2017

Conversation

jagerman
Copy link
Member

We now require (and enforce at compile time):

  • GCC 4.8+
  • clang 3.3+ (5.0+ for Apple's renumbered clang)
  • MSVC 2015u3+
  • ICC 15+

This also updates the versions listed in the README, and removes a now-redundant MSVC version macro check.

Fixes #706 (by producing a more useful error message citing the required MSVC version).

// Compiler version assertions
#if defined(__INTEL_COMPILER)
// ICC v15 or newer required
static_assert(__INTEL_COMPILER >= 1500, "pybind11 requires Intel C++ compiler v15 or newer");
Copy link
Member

Choose a reason for hiding this comment

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

The offending compiler may not actually have static_assert -- might be better to use #error.

Minor: The comments are a bit redundant as they just say the same thing as the error message and/or assertion statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is #error universally supported?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, it is. I thought it was a non-standard extension. I'll update.

# error pybind11 requires clang 3.3 or newer
# endif
#elif defined(__clang__)
// Apple changes clang version macros to its Xcode version for some reason ("Think different"?)
Copy link
Member

Choose a reason for hiding this comment

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

I sympathize with the rant, but perhaps we don't need it permanently in common.h? ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, fair enough.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Mar 16, 2017

The changes that would be required to support Update 1 of MSVC seem pretty minor.

It would be really nice to still support this version...

Visual Studio 2015 very recent after all.
MSVC Update 1 is from November 2015
MSVC Update 2 is from March 2016

@dean0x7d
Copy link
Member

Like I said in #706:

VS 2015 Update 2 should be able to compile pybind11 with just some minor workarounds, but I think the bigger issue is that it's difficult to test. AFAIK AppVeyor only has a "Visual Studio 2015" option and it's always kept up to date, so there's only Update 3 to test against.

Without continuous testing, the support could break at any point and we wouldn't know about it. It's not just the standard library. The different MSVC compiler updates handle some template code differently which requires specific workarounds. It looks bad to pledge support for Update 1 and 2, when it would most likely break often. The VS updates themselves are free so I don't think it's too much to ask for Update 3.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Mar 16, 2017

Indeed, it seems that this breakage was not intentional in the first place. However, updating a compiler version in large projects / production environments is not a trivial decision...

PS: it appears that appveyor has images for older versions of visual studio.

@wjakob
Copy link
Member

wjakob commented Mar 16, 2017

In the partial order of compiler standards compliance, the service packs are comparable and clearly getting better over time. Hence, if we require a service pack, then I don't think it's unreasonable to ask for SP3. While it would be nice to support the non-patched MSVC2015 release, that seems impossible given the many issues this release has. Thus I'm leaning against supporting earlier MSVC 2015 service packs, particularly if this means adding ugly workarounds somewhere.

@dean0x7d
Copy link
Member

PS: it appears that appveyor has images for older versions of visual studio.

Not the individual updates as far as I can see. There are 2013, 2015 and 2017 images but they all have the latest update of their respective Visual Studio. There are also the "Previous Visual Studio xxxx" images, but those are only the "last known good build" as they update all software continuously (so not necessarily the previous VS update).

@SylvainCorlay
Copy link
Member

This comes from the following thread: http://help.appveyor.com/discussions/questions/3167-visual-studio-2015-update-2

@dean0x7d
Copy link
Member

This comes from the following thread: http://help.appveyor.com/discussions/questions/3167-visual-studio-2015-update-2

Looks like it's a workaround for specific accounts, rather than a regular feature: appveyor/ci#891

@jagerman
Copy link
Member Author

jagerman commented Mar 16, 2017

Aside from the testing issues, update 3 versus update 2 gives us two concrete things:

  1. The all_of and related templates become much simpler than the required workaround under update 2.
  2. We have at least one feature, overload_cast, that appears to be impossible with update 2.

If 1 was the only issue we could address it by reintroducing the (ugly) workaround instead of using std::conjunction/std::disjunction, but 2 is a bigger deal: documented features of pybind simply wouldn't be available under certain compilers. I think this is a very bad thing, and I'd honestly prefer that we remove features that aren't supported in every environment we support. But that means we can't have nice things like overload_cast because of fixed bugs in an older compiler that can't be worked around.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Mar 16, 2017

It seems that on other platforms, you already disable overload_cast when C++14 is not available.

One could disable it for earlier versions than Update 3.

@jagerman
Copy link
Member Author

It comes down to this: is there a strong, pressing need for continuing to support update 2 that outweighs the maintenance burden? Who strongly needs this and cannot upgrade, and are they using, or likely to start using, pybind11 but only if they don't have to install Microsoft's software update for their compiler?

On the flip side, I know that, personally, I've spent considerably longer dealing with MSVC bugs (often ICEs) than I have for bugs in all other compilers and compiler versions combined. It's not a well-loved compiler, to put it mildly, and dealing with its bugs is a pain in the ass; I'm going to need something more than "there might be hypothetical users out there that want to continue using older, buggy software without installing the released, free, bug-fix updates" to be convinced here.

I don't restrict this point of view to Microsoft: I'd have little sympathy for dealing with, say, a gcc error in Ubuntu that is fixed by a released update to that specific version of Ubuntu.

@dean0x7d dean0x7d mentioned this pull request Mar 18, 2017
@jagerman
Copy link
Member Author

I think @dean0x7d, @wjakob and I are in agreement that, at present, the maintenance burden of supporting MSVC 2015 update 2 or earlier is too high relative to the fairly small gains.

I think these two quotes (from #726), with which I'm in complete agreement, essentially sum it up:
@dean0x7d:

Adding support for Update 2 now would require someone to install it locally (can't be installed side-by-side with Update 3), make the necessary workarounds and then hope it doesn't break in the future as there is no CI support.

@wjakob:

Part of the motivation of this project is that it's okay to assume a reasonably correct compiler to avoid having to build complicated abstractions over semi-broken metaprogramming facilities.

If someone feels that support for earlier versions of compilers is sufficiently worthwhile and wants to commit to not only contribute the necessary workarounds for the current code but also to regularly dealing with future issues (because, quite frankly, I think most regular pybind11 contributors hate dealing with MSVC issues, and the older the release the more issues), I'd happily accept such a contribution. But I don't want to deal with a semi-broken compiler, and I don't think it's fair to ask contributors to pybind11 to deal with potential issues; so until/unless such a contribution commitment is made, the list in this PR is going to be the minimum supported.

We now require (and enforce at compile time):
- GCC 4.8+
- clang 3.3+ (5.0+ for Apple's renumbered clang)
- MSVC 2015u3+
- ICC 15+

This also updates the versions listed in the README, and removes a
now-redundant MSVC version check.
@jagerman jagerman added this to the v2.1 milestone Mar 19, 2017
@jagerman jagerman merged commit 5a92478 into pybind:master Mar 19, 2017
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.

None yet

4 participants