[PATCH] D47073: Document and Enforce new Host Compiler Policy

Tue Jan 15 19:06:25 PST 2019

chandlerc added a comment.

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:

In D47073#1358937 <https://reviews.llvm.org/D47073#1358937>, @jyknight wrote:

> 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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D47073/new/

https://reviews.llvm.org/D47073