Page MenuHomePhabricator

Document and Enforce new Host Compiler Policy
AbandonedPublic

Authored by erichkeane on May 18 2018, 8:25 AM.

Details

Summary

As proposed here:
http://lists.llvm.org/pipermail/llvm-dev/2018-May/123238.html

This patch enforces the 3/1.5 year error/warning policy for
the host compiler, as well as documents it in the getting
started guide.

Note that most of the patch is cribbed from (thanks JF!):
https://reviews.llvm.org/D46723

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
erichkeane added inline comments.Jan 9 2019, 6:23 AM
cmake/modules/CheckCompilerVersion.cmake
10

The # refers to a previous version of the review where I'd screwed up a merge from svn. The # has been done. That said, I DO need to update this to say JUST

24

I'll reword fatal to be similar to this.

docs/GettingStarted.rst
286–290

I'd not updated this since I was expecting feedback here. I personally think that actually removing support for older compilers is necessary, however if we want the '3 year warning' in effect (GCC5.1, Clang 3.6) for 8.0, I'll refactor this to be the warning only for now (and revert the error messages to 4.8/3.1).

Guidance would be appreciated.

erichkeane updated this revision to Diff 180831.Jan 9 2019, 6:24 AM

Reworded a thing. Still needs direction feedback for 8.0.

jfb added inline comments.Jan 9 2019, 9:32 AM
docs/GettingStarted.rst
286–290

I think for the LLVM 8 release it makes sense to have a hard error for compilers older than 3 years old, with a cmake flag to force-override. That means the error can't just be ignored, but we're being sensible because LLVM 8 doesn't technically need a newer compiler.

That way:

  • A developer with a compiler older than 3 years old who compiles LLVM 8 will get an error. They'll either update their compiler, or use the cmake flag.
  • A developer with a compiler older than 1.5 years just gets a warning.
  • After LLVM 8, we'll hopefully be able to upgrade the C++ version LLVM uses, and the error would now become a hard error because those older compilers are actually unsupported.

Change language to make this a 'guideline' instead of a hardline policy.

Also, add an option to disable the checking.

jfb added inline comments.Jan 9 2019, 10:33 AM
CMakeLists.txt
28 ↗(On Diff #180875)

"HOST_COMPILER" instead of "HOST".

cmake/modules/CheckCompilerVersion.cmake
5

This is still pretty dogmatic:

For GCC and Clang, LLVM's guideline is to support all
major compiler versions released within 3 years from the date in which the
previous version was branched. Additionally, a CMake warning is issued for
versions older than 1.5 years.

I'd say:

For GCC and Clang, LLVM's rough guideline is to support major compiler versions released about 3 years from the date in which the previous versions was branched. This time-based guideline isn't strict: we want to keep some discretion when supporting older compilers still makes sense, or when newer compilers inflict significant pain.
Additionally, a CMake warning is issued for versions older than roughly 1.5 years.

6

I'd move this to the error message, not the comment.

22

Move the instructions on silencing this error here.

28

Ditto.

docs/CMake.rst
578 ↗(On Diff #180875)

Also say: this flag will be removed for LLVM 9, since we'll likely bump the required C++ version.

erichkeane marked 6 inline comments as done.
jfb added a comment.Jan 9 2019, 11:27 AM

You'll also need to treat AppleClang different from Clang, since version numbering is different.

A good list of versions is: https://en.wikipedia.org/wiki/Xcode#Latest_versions
3 years puts you at AppleClang 7.3.0, and 1.5 years at 8.1.0.

cmake/modules/CheckCompilerVersion.cmake
25

The name of the macro is horrible wrong here. I like misdirection ;)

32

Ditto

docs/CMake.rst
576 ↗(On Diff #180881)

Update name.

docs/GettingStarted.rst
290

I think you want to update this text as in the comment you just updated.

erichkeane marked 3 inline comments as done.

Few more fixes suggested by @jfb . Also, added AppleClang to the check.

jfb added inline comments.Jan 9 2019, 12:22 PM
cmake/modules/CheckCompilerVersion.cmake
25

Macro name is still wrong :)

32

Wrong here too.

57

Probably better to have this next to the clang code, so they don't diverge.

63

Name wrong.

jfb added a subscriber: arphaman.Jan 9 2019, 12:22 PM
erichkeane marked 5 inline comments as done.

Fixing my awful inattention to spelling in a few places, moving the AppleClang code to be more clear for future updates.

mehdi_amini added inline comments.
docs/GettingStarted.rst
290

I'm in strong agreement with Chandler here. I am not against the warning, that's probably harmless, I can even go with the "rough guideline of 3 years", even though I don't see much benefit to it since it won't affect the process much.

However I'd like to see language inserted here to mention what Chandler described (the three points) about the process for dropping any toolchain from our supported list. I would also add that the process should include an RFC thread on the LLVM-dev and CFE-dev mailing lists.

Added Policy guidance for the change process to CheckCompilerVersion.cmake.

Looks good, that is more in line with what i have been suggesting in IRC.
Some nits:

docs/CodingStandards.rst
187

Should this point to gcc-5.2, since that is the required version as per the check?
https://gcc.gnu.org/onlinedocs/gcc-5.2.0/libstdc++/manual/manual/status.html#status.iso.2011

docs/GettingStarted.rst
225

Should this be 5.2?

355–371

Should all this point to 5.2, since that is the required version as per cmake check?

erichkeane marked 3 inline comments as done.

Fixed @lebedev.ri s comments.

jfb accepted this revision.Jan 11 2019, 12:29 PM

Update is still LGTM.

mehdi_amini requested changes to this revision.Jan 11 2019, 12:58 PM

I'd like to see docs/GettingStarted.rst updated to include some language from what Chandler mentioned. In particular upgrading a toolchain has to be *motivated* by explicit benefits, and the toolchains dropped must be evaluated with respect to the general availability for the users.

This revision now requires changes to proceed.Jan 11 2019, 12:58 PM
mehdi_amini added inline comments.Jan 11 2019, 1:00 PM
cmake/modules/CheckCompilerVersion.cmake
16

Has the choice of these versions be motivated in a thread on the mailing list? I.e. why go from clang 3.1 to clang 3.6 and not clang 3.5 or 3.7 for example?

I'd like to see docs/GettingStarted.rst updated to include some language from what Chandler mentioned. In particular upgrading a toolchain has to be *motivated* by explicit benefits, and the toolchains dropped must be evaluated with respect to the general availability for the users.

I think this is implied by "This time-based guideline is not strict; we want to keep some discretion when supporting older compilers still makes sense, or when newer compilers inflict significant pain."

The first part of your comment "motivated by explicit benefits" may not be implied though, so what about just adding "when supporting older compilers still make sense or is there is no significant benefit to upgrading" and leave the rest the same?

cmake/modules/CheckCompilerVersion.cmake
16

I think the idea is to accept the lowest version that supports all of the features we need.

I'd like to see docs/GettingStarted.rst updated to include some language from what Chandler mentioned. In particular upgrading a toolchain has to be *motivated* by explicit benefits, and the toolchains dropped must be evaluated with respect to the general availability for the users.

I think this is implied [...]

Right, I would make it more explicit:

For GCC and Clang, LLVM's guideline is to support all major compiler
versions released at least 3 years from the date in which the previous version was
branched. This time-based guideline is not strict; we will balance the benefits to
drop support for older compilers in terms of features available for development
with the availability of more recent versions on the most widely used platforms
(for example considering Ubuntu LTS, etc.).

cmake/modules/CheckCompilerVersion.cmake
16

As per the discussion about the guideline, there should be an email thread mentioning what are these features, what is the minimum version of compiler for each, and what is the availability (i.e. for example: gcc 5.2 means available by default on FreeBSD W and Ubuntu X and with PPA in Ubuntu Y. Gcc 6.3 means warning in Red Hat Z, etc.).

hans added a comment.Jan 15 2019, 1:00 AM
In D47073#1346562, @jfb wrote:

I think we want to start warning developers before the LLVM 8 branch point on January 16th. I'm assuming @hans will want to chime in.

Apologies for being slow to reply, I was out on vacation.

I like this patch, but trying to get it in right before the release branch (which happens tomorrow) makes me a little nervous. I'd be more comfortable if it got to bake on trunk to surface any issues developers have with it, before it goes to a release branch.

Is there any particular reason why we'd need this for LLVM 8?

jfb added a comment.Jan 15 2019, 9:25 AM
In D47073#1346562, @jfb wrote:

I think we want to start warning developers before the LLVM 8 branch point on January 16th. I'm assuming @hans will want to chime in.

Apologies for being slow to reply, I was out on vacation.

I like this patch, but trying to get it in right before the release branch (which happens tomorrow) makes me a little nervous. I'd be more comfortable if it got to bake on trunk to surface any issues developers have with it, before it goes to a release branch.

Is there any particular reason why we'd need this for LLVM 8?

Yes: we want to warn folks early. This is just warning them, and they can opt-out. Getting it in the release branch is exactly the right place: people try out releases much more than tip-of-tree.
We want it in LLVM 8 because then there will have been enough advanced warning that, when we're ready to move away from C++11 we can do so. Tentatively, the target is end of the first quarter of 2019, so right after LLVM 8.

In D47073#1358039, @jfb wrote:

Yes: we want to warn folks early. This is just warning them, and they can opt-out.

The current version of the patch would issue a fatal error when using a host clang 3.5, not just a warning
(note that clang-3.4 is supposed to support C++14)

jfb added a comment.Jan 15 2019, 9:45 AM
In D47073#1358039, @jfb wrote:

Yes: we want to warn folks early. This is just warning them, and they can opt-out.

The current version of the patch would issue a fatal error when using a host clang 3.5, not just a warning
(note that clang-3.4 is supposed to support C++14)

Yes it's a CMake error. The point is: you won't see it and will ignore it if it's just a CMake warning. What I meant was: we're giving everyone with older compilers a warning that they won't be supported soon. They can opt out and compile this once. Won't be true soon (because we'll move away from C++11). I want them to know ASAP.

In D47073#1358067, @jfb wrote:
In D47073#1358039, @jfb wrote:

Yes: we want to warn folks early. This is just warning them, and they can opt-out.

The current version of the patch would issue a fatal error when using a host clang 3.5, not just a warning
(note that clang-3.4 is supposed to support C++14)

Yes it's a CMake error. The point is: you won't see it and will ignore it if it's just a CMake warning. What I meant was: we're giving everyone with older compilers a warning that they won't be supported soon. They can opt out and compile this once. Won't be true soon (because we'll move away from C++11). I want them to know ASAP.

OK but then this patch is not correct. It should not claim that the compilers aren't supported but that they are deprecated and support will be dropped in the next release.
Also the two levels (warnings vs errors) is not appropriate to accomplish what you're describing.
Finally, that still left open the choice of which compiler version the "warning" is issued.

One release cycle with deprecating the version we're about to remove support for using a discardable cmake error seems like a good idea in general: we should encode this in the policy as well.

jfb added a comment.Jan 15 2019, 12:39 PM
In D47073#1358067, @jfb wrote:
In D47073#1358039, @jfb wrote:

Yes: we want to warn folks early. This is just warning them, and they can opt-out.

The current version of the patch would issue a fatal error when using a host clang 3.5, not just a warning
(note that clang-3.4 is supposed to support C++14)

Yes it's a CMake error. The point is: you won't see it and will ignore it if it's just a CMake warning. What I meant was: we're giving everyone with older compilers a warning that they won't be supported soon. They can opt out and compile this once. Won't be true soon (because we'll move away from C++11). I want them to know ASAP.

OK but then this patch is not correct. It should not claim that the compilers aren't supported but that they are deprecated and support will be dropped in the next release.

Rewording along those lines sounds fine.

Also the two levels (warnings vs errors) is not appropriate to accomplish what you're describing.

I don't understand what you're asking to change here.

Finally, that still left open the choice of which compiler version the "warning" is issued.

~1.5 years, which per policy can be updated after an llvm-dev discussion.

One release cycle with deprecating the version we're about to remove support for using a discardable cmake error seems like a good idea in general: we should encode this in the policy as well.

Sure.

Also the two levels (warnings vs errors) is not appropriate to accomplish what you're describing.

I don't understand what you're asking to change here

The implementation in CMake has:

set(CLANG_MIN 3.6)
set(CLANG_WARN 4.0)

If your intent is to warn people that their host compiler won't be supported in the next release, we don't need *two* levels here.

Finally, that still left open the choice of which compiler version the "warning" is issued.

~1.5 years, which per policy can be updated after an llvm-dev discussion.

Thought it was *3* years before removing the support? (Also should be *motivated* by *benefits*)

lhames added a subscriber: lhames.Jan 15 2019, 12:52 PM
In D47073#1358039, @jfb wrote:

Is there any particular reason why we'd need this for LLVM 8?

Yes: we want to warn folks early. This is just warning them, and they can opt-out. Getting it in the release branch is exactly the right place: people try out releases much more than tip-of-tree.

Yep: It would be great to see this make it in to 8.0: I know a lot of JIT clients only check out releases.

jfb added a comment.Jan 15 2019, 12:59 PM

Also the two levels (warnings vs errors) is not appropriate to accomplish what you're describing.

I don't understand what you're asking to change here

The implementation in CMake has:

set(CLANG_MIN 3.6)
set(CLANG_WARN 4.0)

If your intent is to warn people that their host compiler won't be supported in the next release, we don't need *two* levels here.

We absolutely do. For now, as we phase this in, you can opt out of the error. This is the one and only time you can do this. In the future, it'll be a hard error you can't opt out of, because we'll have moved past C++11. The "MIN" is *for now* a suggested minimum, but after LLVM 8 it'll be mandatory. This patch sets up our process, we need it.

Finally, that still left open the choice of which compiler version the "warning" is issued.

~1.5 years, which per policy can be updated after an llvm-dev discussion.

Thought it was *3* years before removing the support? (Also should be *motivated* by *benefits*)

~3 years for "MIN", ~1.5 years for "WARN". This hasn't changed since months ago in the discussion, I don't want to re-open it for debate.

Update branch date to match the 7.0.0 from 8.0.0 branch date.

jfb accepted this revision.Jan 15 2019, 1:19 PM

OK I think we've had more than enough bikeshed, over months, and in-person at the LLVM dev meeting. This is good enough, we want to warn people for the LLVM 8 branch which is tomorrow. Let's get this checked in today. We'll start a discussion on LLVM dev when anything *actually* changes, so nobody needs to panic just yet.

In D47073#1358593, @jfb wrote:

OK I think we've had more than enough bikeshed, over months, and in-person at the LLVM dev meeting. This is good enough, we want to warn people for the LLVM 8 branch which is tomorrow. Let's get this checked in today. We'll start a discussion on LLVM dev when anything *actually* changes, so nobody needs to panic just yet.

I strongly disagree with this right now: this is *not* OK to rush this as is.

If you need something in the release, then it can be done in a much less controversial way.

jfb added a comment.Jan 15 2019, 1:35 PM
In D47073#1358593, @jfb wrote:

OK I think we've had more than enough bikeshed, over months, and in-person at the LLVM dev meeting. This is good enough, we want to warn people for the LLVM 8 branch which is tomorrow. Let's get this checked in today. We'll start a discussion on LLVM dev when anything *actually* changes, so nobody needs to panic just yet.

I strongly disagree with this right now: this is *not* OK to rush this as is.

If you need something in the release, then it can be done in a much less controversial way.

What makes you say this is being rushed? I agree your participation to the discussion is new, but consider that the RFC was *months* ago (and wasn't the first or second time it was discussed), and so was the LLVM developer meeting discussion. There's agreement that we'll move away from C++11, soon. We'd be irresponsible if we didn't tell developers who build LLVM. That's the only thing the patch does: tell people what we've agreed to do (admittedly we don't have a fixed timeline because we're waiting on significant contributors being ready to move, but there's agreement nonetheless, tentatively end of 2019 Q1).

In D47073#1358640, @jfb wrote:
In D47073#1358593, @jfb wrote:

OK I think we've had more than enough bikeshed, over months, and in-person at the LLVM dev meeting. This is good enough, we want to warn people for the LLVM 8 branch which is tomorrow. Let's get this checked in today. We'll start a discussion on LLVM dev when anything *actually* changes, so nobody needs to panic just yet.

I strongly disagree with this right now: this is *not* OK to rush this as is.

If you need something in the release, then it can be done in a much less controversial way.

What makes you say this is being rushed?

The way you're trying to get this checked in with two reviewers requiring major changes that haven't been addressed.
The fact that the process of discussing which version of the compiler to bump to should happen *before* adding a check, and it hasn't happened for this patch which *actually* change the minimum versions.

There's agreement that we'll move away from C++11, soon. We'd be irresponsible if we didn't tell developers who build LLVM. That's the only thing the patch does: tell people what we've agreed to do?

  1. I asked *multiple* times in this patch *why* and *how* these particular version of the compiler where picked. I still don't know why clang-3.5 would trigger a cmake error.
  2. The two levels: cmake warn vs cmake error are *not* required to achieve what you're describing: i.e. warning people in LLVM 8 that LLVM 9 won't support their compiler. *One* level (cmake error) is plenty enough.
In D47073#1358039, @jfb wrote:

Yes: we want to warn folks early. This is just warning them, and they can opt-out.

The current version of the patch would issue a fatal error when using a host clang 3.5, not just a warning
(note that clang-3.4 is supposed to support C++14)

jfb added a comment.Jan 15 2019, 1:47 PM
In D47073#1358640, @jfb wrote:
In D47073#1358593, @jfb wrote:

OK I think we've had more than enough bikeshed, over months, and in-person at the LLVM dev meeting. This is good enough, we want to warn people for the LLVM 8 branch which is tomorrow. Let's get this checked in today. We'll start a discussion on LLVM dev when anything *actually* changes, so nobody needs to panic just yet.

I strongly disagree with this right now: this is *not* OK to rush this as is.

If you need something in the release, then it can be done in a much less controversial way.

What makes you say this is being rushed?

The way you're trying to get this checked in with two reviewers requiring major changes that haven't been addressed.

s/two/one/ : I've talked to Chandler already, and he was leaning on others (in large parts, me) to help Erich with the review. Further, Erich addressed your comments repeatedly. It seems like you're asking for more?

The fact that the process of discussing which version of the compiler to bump to should happen *before* adding a check, and it hasn't happened for this patch which *actually* change the minimum versions.

It happened months ago in the LLVM dev RFC, and was re-discussed in-person at the dev meeting.

There's agreement that we'll move away from C++11, soon. We'd be irresponsible if we didn't tell developers who build LLVM. That's the only thing the patch does: tell people what we've agreed to do?

  1. I asked *multiple* times in this patch *why* and *how* these particular version of the compiler where picked. I still don't know why clang-3.5 would trigger a cmake error.

Read the email thread?

  1. The two levels: cmake warn vs cmake error are *not* required to achieve what you're describing: i.e. warning people in LLVM 8 that LLVM 9 won't support their compiler. *One* level (cmake error) is plenty enough.

This is setting up something new. Yes, we could just have one level for now, but we'll add the second level as soon as we truly move off C++11 (this is clear in the policy!). The patch sets things up properly for the policy we're establishing. It's a boundary value problem, and like all good BVPs we're carving out a small boundary (here, at the initial impulse) to solve the wider problem. The policy is encoded in CMake.

In D47073#1358039, @jfb wrote:

Yes: we want to warn folks early. This is just warning them, and they can opt-out.

The current version of the patch would issue a fatal error when using a host clang 3.5, not just a warning
(note that clang-3.4 is supposed to support C++14)

OK I think we've had more than enough bikeshed, over months, and in-person at the LLVM dev meeting. This is good enough, we want to warn people for the LLVM 8 branch which is tomorrow. Let's get this checked in today. We'll start a discussion on LLVM dev when anything *actually* changes, so nobody needs to panic just yet.

I strongly disagree with this right now: this is *not* OK to rush this as is.

If you need something in the release, then it can be done in a much less controversial way.

Given how long this has been discussed I don't feel like it is being rushed. On the contrary: I would really like to see us land this (or something very much like it in 8.0) to get this process moving.

The two levels: cmake warn vs cmake error are *not* required to achieve what you're describing: i.e. warning people in LLVM 8 that LLVM 9 won't support their compiler. *One* level (cmake error) is plenty enough.

This is setting up something new. Yes, we could just have one level for now, but we'll add the second level as soon as we truly move off C++11 (this is clear in the policy!). The patch sets things up properly for the policy we're establishing. It's a boundary value problem, and like all good BVPs we're carving out a small boundary (here, at the initial impulse) to solve the wider problem. The policy is encoded in CMake.

The specifics of how the warning/error are implemented seem like something we could revisit. I would much rather have some mechanism make it in to 8.0 to warn people about these changes, rather than put off any warnings for another release cycle.

Firstly -- I agree we ought to put something in the LLVM 8.0 release. However, we do not need to get it committed *today* for it to make it into the 8.0 release. It can be cherry-picked to the branch after the branch-cut.

Secondly -- there seems to be some confusion still over what the new policy is. My understanding is that the time-based proposal is *rejected*, and that we're using "3 years old" only as a guideline for what compilers to support, not any sort of date cutoff. Therefore, I think we should be encoding two toolchain version requirements in the buildsystem:

  1. The *actual* minimum version required. For the GCC toolchain, that is currently GCC 4.8.
  2. The intended minimum version required for the next release of LLVM. (GCC 5, because we want C++14, and that's the first version that supports it).

I'd call those two values GCC_MIN and GCC_WARN -- and both should be cmake-level errors by default, because, as you say, a warning message would simply be ignored. The former is be an unconditional error (just as the existing error for GCC < 4.8 is), while the latter must be able to be disabled. This patch, as it stands, is missing #1, puts #2 in "GCC_MIN", and uses "GCC_WARN" as a non-fatal warning message, set to GCC 6.3. I don't think that GCC 6.3 is relevant to anything, so that last is unnecessary.

Finally, as the policy expressed in this commit states, and as mehdi has requested, an email should be sent to llvm-dev, setting out concretely what the new minimum compiler versions for LLVM 9 are intended to be, and why (comparing value vs harm). The most-recent email thread linked from this commit does NOT have this. I do recall seeing such things in other threads in the past, but the conclusion should be set out concretely, now.

In D47073#1358663, @jfb wrote:

s/two/one/ : I've talked to Chandler already, and he was leaning on others (in large parts, me) to help Erich with the review

I'm not gonna speak for him, I just still see a red mark on this patch, admittedly the patch has been updated since.

Further, Erich addressed your comments repeatedly. It seems like you're asking for more?

Yes, not everything has been addressed AFAICT: like how the specific version of the compilers have been picked.

The fact that the process of discussing which version of the compiler to bump to should happen *before* adding a check, and it hasn't happened for this patch which *actually* change the minimum versions.

It happened months ago in the LLVM dev RFC, and was re-discussed in-person at the dev meeting.

No: the RFC does not include this discussion. If you want to claim this, then please source it precisely.

There's agreement that we'll move away from C++11, soon. We'd be irresponsible if we didn't tell developers who build LLVM. That's the only thing the patch does: tell people what we've agreed to do?

  1. I asked *multiple* times in this patch *why* and *how* these particular version of the compiler where picked. I still don't know why clang-3.5 would trigger a cmake error.

Read the email thread?

It does not motivate the reason to pick clang 3.6, and basing it *only* on the arbitrary time-based choice: this is not acceptable IMO.

This was mentioned in the RFC as well but unanswered AFAICT: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123261.html

I think there is a lot of confusion here, and frustration, and that is making it hard to make progress.

To help, I want to reiterate what James said: we do not have to land this patch *now* for it to be in the release. In fact, Hans already said he would prefer that this spend some time in trunk before being cherrypicked for the release (and I agree with his reasoning, and ultimately think it is his call).

While I agree with Lang that this has already dragged on a long time, I also think it *can* wait until we sort out the confusion without any harm. =D

A question came up about whether my concerns have been addressed. I did talk with JF, and he was working to get my primary concern addressed which was making it clear that the time based strategy isn't a rule. I think this has been mostly addressed.

However, I think Mehdi has raised some very valid points that I do agree with. The first concern is that the "minimum" and "warning" distinction is confusing, and I really do agree with him here. I was having trouble understanding it, but I think James summarized perfectly a good approach:

Secondly -- there seems to be some confusion still over what the new policy is. My understanding is that the time-based proposal is *rejected*, and that we're using "3 years old" only as a guideline for what compilers to support, not any sort of date cutoff. Therefore, I think we should be encoding two toolchain version requirements in the buildsystem:

  1. The *actual* minimum version required. For the GCC toolchain, that is currently GCC 4.8.
  2. The intended minimum version required for the next release of LLVM. (GCC 5, because we want C++14, and that's the first version that supports it).

    I'd call those two values GCC_MIN and GCC_WARN -- and both should be cmake-level errors by default, because, as you say, a warning message would simply be ignored. The former is be an unconditional error (just as the existing error for GCC < 4.8 is), while the latter must be able to be disabled.

I would draw a direct analogy to Clang's own diagnostics. One of these is an error, and it cannot be ignored. The other is a warning, but it is a warning that users will be unlikely to notice, so we use the CMake-analog to -Werror to make it an error, and if users need to move past it, they can disable the warning (much like you can with Clang).

This seems like a really reasonable strategy and to provide concrete benefits.

On the flip side, having a CMake warning for compilers 1.5y old doesn't (to me) provide any concrete benefit. As you say, the users will ignore them. So why implement logic for them? I think it just adds confusion to what we're trying to do here.

The second concern Mehdi raised James clearly seconded here:

Finally, as the policy expressed in this commit states, and as mehdi has requested, an email should be sent to llvm-dev, setting out concretely what the new minimum compiler versions for LLVM 9 are intended to be, and why (comparing value vs harm). The most-recent email thread linked from this commit does NOT have this. I do recall seeing such things in other threads in the past, but the conclusion should be set out concretely, now.

I agree as well. The old thread, while it has been out for some time, has likely not communicated this effectively. A fresh mail that concisely and succinctly lays out the goals/plans seems like a reasonable request and easily satisfied.

Mehdi raised a third concern that has not been addressed: add some of the criteria I suggested for how to make the actual decision to upgrade to the documentation so that it doesn't have to be repeated in each -dev thread discussing this every N releases. This doesn't seem as critical to me, but also seems useful and easily done.

Perhaps there are other things worth discussing, but I think at least these things currently have not been addressed and we don't have consensus here, so I think landing the patch remains a bit premature. I *also* am not worried about getting the resolution to the above worked out and into LLVM-8, so while I'm urging taking some more time, I do *not* think this will materially delay anything.

For what it's worth, the stricter version of this patch received quite overwhelming support in May. We delayed it due to Google's limitations and yet we still don't seem to have made progress. This current patch is a result of a continued set of compromises to try to satisfy a few in the vocal minority. I'd suggest that we all agree to have an ACTUAL decision made on llvm-dev, rather than this constant state of consensus derailed by vocal minority.

For what it's worth, the stricter version of this patch received quite overwhelming support in May. We delayed it due to Google's limitations and yet we still don't seem to have made progress. This current patch is a result of a continued set of compromises to try to satisfy a few in the vocal minority. I'd suggest that we all agree to have an ACTUAL decision made on llvm-dev, rather than this constant state of consensus derailed by vocal minority.

This *patch* does not matter to Google in any real sense that I'm aware of.... *All* of what I wrote in my latest comment is about this patch, and has nothing to do with the overall strategy of the LLVM project. And that feedback was given as a very active individual contributor to the project.

Does Google care a great deal about the overall state of host compiler support and C++ version support of LLVM? Yes, it does. We expressed that, and so did others. That was done on the -dev list as well as in the BoF. I'm not really interested in re-arguing it here.

But please try to take my feedback on this *patch* as just that. It is code review and feedback on the specific changes you are making. I'm genuinely trying to help get the CMake and documentation implementation *right* and useful for the project and the community, not for Google. I'm sorry that you're frustrated, I really am. But I am actually trying to help, and comments like this, IMO, are not constructive.

I don't want to give the impression I'm stalling this just for the sake of being difficult, and since it is easier to talk "with code" I quickly wrote this: https://reviews.llvm.org/D56770 . This *not* a patch intended to be submitted, it is just a stripped-down version of how I'd phrase/structure it intended to support my arguments in this thread. Note that I would intentionally left out any compiler minimum bump out of the policy upgrade change, as these are conceptually separate change anyway.

jfb added a comment.Jan 15 2019, 9:44 PM

No: the RFC does not include this discussion. If you want to claim this, then please source it precisely.

This patch's description links to: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123238.html

Which says:

The 3-years/1.5 years would result in our minimum GCC/Clang becoming: GCC5.1/Clang3.6. We would WARN on anything older than GCC7.1/Clang3.8

That 3 / 1.5 cutoff was discussed on this patch, e.g. http://lists.llvm.org/pipermail/llvm-dev/2018-May/123249.html
and came from this discussion: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123182.html
It started off as "which compilers give us the features we want" and modified itself into "for now we get what we want with a 3 year lookback, let's use that as a hand wavy policy going forward".

So I'd say that it was sourced quite precisely, and re-listing the entire discussion will lose all the bikeshedding minutia which led to where we are. At this point Erich is patiently trying to add nuance on top of nuance to a warning about a policy, pertaining to a discussion that's been going on for 7 months... So yes I totally see where the frustration comes from, all of that to migrate away from C++11 which has overwhelming support. I think it's critical to hard-warn (i.e. error with opt-out option) people in the LLVM 8 timeframe. Sure we can cherry-pick a patch, but the later we do so the more potential hurt there is. I'd really rather not shoulder that hurt, because I think it's better to just tell people compiling LLVM 8 now: we're going to move away from older compilers, get ready.

For what it's worth, the stricter version of this patch received quite overwhelming support in May. We delayed it due to Google's limitations and yet we still don't seem to have made progress. This current patch is a result of a continued set of compromises to try to satisfy a few in the vocal minority. I'd suggest that we all agree to have an ACTUAL decision made on llvm-dev, rather than this constant state of consensus derailed by vocal minority.

This *patch* does not matter to Google in any real sense that I'm aware of.... *All* of what I wrote in my latest comment is about this patch, and has nothing to do with the overall strategy of the LLVM project. And that feedback was given as a very active individual contributor to the project.

Does Google care a great deal about the overall state of host compiler support and C++ version support of LLVM? Yes, it does. We expressed that, and so did others. That was done on the -dev list as well as in the BoF. I'm not really interested in re-arguing it here.

But please try to take my feedback on this *patch* as just that. It is code review and feedback on the specific changes you are making. I'm genuinely trying to help get the CMake and documentation implementation *right* and useful for the project and the community, not for Google. I'm sorry that you're frustrated, I really am. But I am actually trying to help, and comments like this, IMO, are not constructive.

I acknowledge that you have different concerns than you did previously. I would simply like us to once and for all define what the acceptance criteria are for a patch like this. The fact that we have had a dozen llvm-dev conversations with overwhelming consensus blocked by a few vocal people is concerning.

This patch for example has been watered down excessively from a version with extensive support (the community minus vaguely 3) in an atempt to gain unanimous consent. However, despite multiple consessions, we've gained no support.

Is "a significant majority of the community" sufficent (in which case where we foolish to try to placate nay Sayers), or do we require unanimous consent?

Many of us conceded to delay any such decision on something like this due to Google's concerns, which was my point earlier. Now that they seem less concerned, I'd imagined we would be less blocked. Instead, I notice that the consessions we made to placate a few have resulted in yet another denial of progress.

I am admittedly quite discouraged and frustrated, since this effort is constantly derailed by a vaguely defined and frequently changing process.

So I guess what I want is: how do we meaningfully progress this project? How do we prevent ourselves from being derailed by making consessions to the vocal minority? This patch had a meaningful amount of support in May as a much stronger and sane patch. Yet, these consessions made to the few critics are now the reason to reject it. How do I avoid that next time?

I can't add much more than what @jyknight wrote here: https://reviews.llvm.org/D47073#1358937 ; this summarize perfectly my view of the situation.

In D47073#1359315, @jfb wrote:

No: the RFC does not include this discussion. If you want to claim this, then please source it precisely.

This patch's description links to: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123238.html

Which says:

The 3-years/1.5 years would result in our minimum GCC/Clang becoming: GCC5.1/Clang3.6. We would WARN on anything older than GCC7.1/Clang3.8

I thought you were OK with the fact that a pure time-based motivation wasn't enough?

That 3 / 1.5 cutoff was discussed on this patch, e.g. http://lists.llvm.org/pipermail/llvm-dev/2018-May/123249.html
and came from this discussion: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123182.html

Thanks for this last link, I couldn't find it nowhere in this review, and it isn't in the RFC thread that is linked from here. This last discussion is spot-on what I asked for (multiple times) to justify the versions!
(EDIT: I just figured that it is linked *from your abandoned revision*, but not directly from the RFC or from here)

Unfortunately, the thread ends up with: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123268.html ; which is very close to what I'm asking for.

I feel the work of summarizing the choice of the version of compilers has *not* been done and I'm still under the impression. the versions in this patch has been picked on the 3-y time-based and not on the support they provide.

(you mentioned clang 3.4 here by the way: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123247.html )

jfb added a comment.Jan 16 2019, 9:15 AM

I can't add much more than what @jyknight wrote here: https://reviews.llvm.org/D47073#1358937 ; this summarize perfectly my view of the situation.

In D47073#1359315, @jfb wrote:

No: the RFC does not include this discussion. If you want to claim this, then please source it precisely.

This patch's description links to: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123238.html

Which says:

The 3-years/1.5 years would result in our minimum GCC/Clang becoming: GCC5.1/Clang3.6. We would WARN on anything older than GCC7.1/Clang3.8

I thought you were OK with the fact that a pure time-based motivation wasn't enough?

That 3 / 1.5 cutoff was discussed on this patch, e.g. http://lists.llvm.org/pipermail/llvm-dev/2018-May/123249.html
and came from this discussion: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123182.html

Thanks for this last link, I couldn't find it nowhere in this review, and it isn't in the RFC thread that is linked from here. This last discussion is spot-on what I asked for (multiple times) to justify the versions!
(EDIT: I just figured that it is linked *from your abandoned revision*, but not directly from the RFC or from here)

Unfortunately, the thread ends up with: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123268.html ; which is very close to what I'm asking for.

I feel the work of summarizing the choice of the version of compilers has *not* been done and I'm still under the impression. the versions in this patch has been picked on the 3-y time-based and not on the support they provide.

(you mentioned clang 3.4 here by the way: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123247.html )

This review has unfortunately devolved into something totally unhelpful and unlikely to go anywhere. I don't think this is how an open-source project should work. I suggest we abandon this thread. I'll start a separate llvm-dev thread and new patches to get this back on track.

erichkeane abandoned this revision.Jan 16 2019, 9:17 AM

This patch has run its course.

In D47073#1359998, @jfb wrote:

I suggest we abandon this thread. I'll start a separate llvm-dev thread and new patches to get this back on track.

I concur that going back to the mailing list thread is appropriate. From my POV, the core problem here seems to be that the change from "date-based" to "requirements-based" was not truly agreed upon, despite the wording in this patch being updated to say it.

That needs to be resolved before reviving a patch. Are we:

  1. Choosing versions based on being 3 years old, or,
  2. Choosing versions based on their feature-set (e.g. C++14 support right now).

From the last few comments, it would appear that both Erich and JF both continue to feel that the former should be the policy. And arguing about details when there seems to remain disagreement over that issue is obviously not working.

jfb added a comment.Jan 16 2019, 9:52 AM
In D47073#1359998, @jfb wrote:

I suggest we abandon this thread. I'll start a separate llvm-dev thread and new patches to get this back on track.

I concur that going back to the mailing list thread is appropriate. From my POV, the core problem here seems to be that the change from "date-based" to "requirements-based" was not truly agreed upon, despite the wording in this patch being updated to say it.

That needs to be resolved before reviving a patch. Are we:

  1. Choosing versions based on being 3 years old, or,
  2. Choosing versions based on their feature-set (e.g. C++14 support right now).

    From the last few comments, it would appear that both Erich and JF both continue to feel that the former should be the policy. And arguing about details when there seems to remain disagreement over that issue is obviously not working.

That's incorrect. I don't care what the policy is as long as it's sane. 3 years was proposed on the RFC, we went with it. Then nuance was asked for on this review, we went with it. Then it was asked that time was made a soft constraint, we went with it. Then more bikeshed happened. The version was absolutely chosen based on agreement that we're moving off C++11, and C++14 is the next version over. My updated email and patches will be clear on this, and I urge you Mehdi, and Chandler to try to drive this to a resolution ASAP. I think it would be unacceptable if LLVM 8 didn't have a soft-error, and I think the longer we want to cherry-pick such soft-error the more irresponsible we are.

In D47073#1360077, @jfb wrote:

That's incorrect. I don't care what the policy is as long as it's sane. 3 years was proposed on the RFC, we went with it. Then nuance was asked for on this review, we went with it. Then it was asked that time was made a soft constraint, we went with it. Then more bikeshed happened.

This is a misrepresentation of what happened.
I can give you an alternative version but your attitude in this thread does not lead me to think you'd listen.

The version was absolutely chosen based on agreement that we're moving off C++11, and C++14 is the next version over.

This is not true.

If all you want is C++14 and a warning in LLVM 8.0, the path was fairly clear to me: properly address reviews, don't ignore questions when they don't serve your agenda, and finally don't rush patches when there are controversial points unadressed.
If you think that's how an open source project should work, I think we'll run into more dead-ends.

jfb added a comment.Jan 16 2019, 10:31 AM
In D47073#1360077, @jfb wrote:

That's incorrect. I don't care what the policy is as long as it's sane. 3 years was proposed on the RFC, we went with it. Then nuance was asked for on this review, we went with it. Then it was asked that time was made a soft constraint, we went with it. Then more bikeshed happened.

This is a misrepresentation of what happened.
I can give you an alternative version but your attitude in this thread does not lead me to think you'd listen.

The version was absolutely chosen based on agreement that we're moving off C++11, and C++14 is the next version over.

This is not true.

If all you want is C++14 and a warning in LLVM 8.0, the path was fairly clear to me: properly address reviews, don't ignore questions when they don't serve your agenda, and finally don't rush patches when there are controversial points unadressed.
If you think that's how an open source project should work, I think we'll run into more dead-ends.

This is not a constructive response, and I don't intend to keep replying to this thread. As I've said, this review is past its usefulness. In the follow-up, please avoid ascribing ill-intent to what I'm doing. Specifically, "misrepresentation", "you'd listen", "ignore questions", "serve your agenda", "rush patches", "controversial", are all phrases that I find don't lead to a productive discussion.

jfb added a comment.Jan 16 2019, 3:35 PM

Please see https://reviews.llvm.org/D56819 for the policy discussion.