This is an archive of the discontinued LLVM Phabricator instance.

Document toolchain update policy
ClosedPublic

Authored by jfb on Jan 16 2019, 3:35 PM.

Details

Summary

Capture the current agreed-upon toolchain update policy based on the following discussions:

  • LLVM dev meeting 2018 BoF "Migrating to C++14, and beyond!" llvm.org/devmtg/2018-10/talk-abstracts.html#bof3
  • A Short Policy Proposal Regarding Host Compilers lists.llvm.org/pipermail/llvm-dev/2018-May/123238.html
  • Using C++14 code in LLVM (2018) lists.llvm.org/pipermail/llvm-dev/2018-May/123182.html
  • Using C++14 code in LLVM (2017) lists.llvm.org/pipermail/llvm-dev/2017-October/118673.html
  • Using C++14 code in LLVM (2016) lists.llvm.org/pipermail/llvm-dev/2016-October/105483.html
  • Document and Enforce new Host Compiler Policy llvm.org/D47073
  • Require GCC 5.1 and LLVM 3.5 at a minimum llvm.org/D46723

See the corresponding RFC: http://lists.llvm.org/pipermail/llvm-dev/2019-January/129241.html

Diff Detail

Repository
rL LLVM

Event Timeline

jfb created this revision.Jan 16 2019, 3:35 PM
jfb edited the summary of this revision. (Show Details)Jan 16 2019, 3:36 PM
jfb edited the summary of this revision. (Show Details)
jfb added subscribers: mehdi_amini, jyknight, rsmith and 3 others.

GettingStarted.rst is probably not the best place to put the policy for updating the required version. I mean, I guess it's sort of convenient because it's right next to the actual versions, but we don't want to force new users to wade through it.

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

GettingStarted.rst is probably not the best place to put the policy for updating the required version. I mean, I guess it's sort of convenient because it's right next to the actual versions, but we don't want to force new users to wade through it.

Where would a better place be?

craig.topper added inline comments.
docs/GettingStarted.rst
270 ↗(On Diff #182174)

Was "fewer" here supposed to be "newer"? Or is just the "fewer ones" doesn't read very well?

jfb marked an inline comment as done.Jan 16 2019, 4:04 PM
jfb added inline comments.
docs/GettingStarted.rst
270 ↗(On Diff #182174)

Maybe it's my ESL, I definitely mean "fewer" as in "fewer versions than a 3 year loopback would otherwise capture". I'm happy to clarify however you think would make sense.

craig.topper added inline comments.Jan 16 2019, 4:19 PM
docs/GettingStarted.rst
270 ↗(On Diff #182174)

Maybe just change "ones" to "versions"?

DeveloperPolicy.rst, maybe?

aprantl added inline comments.
docs/GettingStarted.rst
227 ↗(On Diff #182174)

why currently?

jfb updated this revision to Diff 182188.Jan 16 2019, 4:44 PM
jfb marked 3 inline comments as done.
  • Minor comments.
docs/GettingStarted.rst
227 ↗(On Diff #182174)

Obviously we'll move to JavaScript in the future 🤡

jfb updated this revision to Diff 182189.Jan 16 2019, 4:49 PM
  • Move to DeveloperPolicy
jfb added a comment.Jan 16 2019, 4:49 PM

DeveloperPolicy.rst, maybe?

Oh nice, that's indeed much better. Updated.

jfb updated this revision to Diff 182190.Jan 16 2019, 4:51 PM
  • Remove extra space.
Eugene.Zelenko added inline comments.
docs/DeveloperPolicy.rst
671 ↗(On Diff #182190)

top-of-tree

serge-sans-paille added inline comments.
docs/GettingStarted.rst
234 ↗(On Diff #182190)

Visual Studio is mentionned here, but not in the

Generally, try to support LLVM and GCC versions from the last 3 years at a minimum

Sentence around line 651 in docs/DeveloperPolicy.rst. maybe we should harmonize that?

jfb updated this revision to Diff 182326.Jan 17 2019, 9:48 AM
jfb marked an inline comment as done.
  • Nit
jfb marked 2 inline comments as done.Jan 17 2019, 9:52 AM
jfb added inline comments.
docs/GettingStarted.rst
234 ↗(On Diff #182190)

The prior discussions were mostly worried about 2 things:

  • Distros come with GCC preinstalled, and people want to avoid building a new GCC to build clang. The policy tries to reduce bootstrapping work when possible, and a proposal has to look at which versions of GCC we're phasing out from which distros.
  • Clang bootstrap is another worry: how many times would one need to compile clang?

In general, the feeling from the conversations was that updating MSVC hasn't been too much of an issue for people in the discussion. Further, while recent MSVC has amazing C++ support, older ones don't. That's why there's no guidance on MSVC. It doesn't mean "ignore MSVC", it just means there's no ballpark guidance *right now*, but a proposal to bump minimum MSVC versions should still consider what bumping MSVC means.

To be clear: "the policy doesn't say anything about X" doesn't mean we can ignore X.

A future revision of this policy could obviously have guidance for MSVC.

erichkeane accepted this revision.Jan 17 2019, 10:28 AM
This revision is now accepted and ready to land.Jan 17 2019, 10:28 AM
lhames accepted this revision.Jan 17 2019, 10:40 AM

This looks great. Thanks JF!

hubert.reinterpretcast added inline comments.
docs/DeveloperPolicy.rst
677 ↗(On Diff #182326)

The suggested wording implies that consensus for updating version requirements for some generic toolchains is consensus to update the set of language features to "whatever falls out of the support provided by those toolchain versions". I think that this step in the proposed process should instead be to identify the features that are now available by the minimum versions of the popular toolchains, and begin a discussion on what features we will start using.

jfb marked 3 inline comments as done.Jan 17 2019, 3:08 PM
jfb added inline comments.
docs/DeveloperPolicy.rst
677 ↗(On Diff #182326)

An earlier point says:

  • An RFC is sent to the llvm-dev mailing list <http://lists.llvm.org/mailman/listinfo/llvm-dev>_
    • Detail upsides of the version increase (e.g. allow LLVM to use newer C++ language or library features; avoid miscompiles in particular compiler versions, etc).
    • Detail downsides on important platforms (e.g. Ubuntu LTS status).

If you think that wording is unclear, please provide concrete alternatives.

docs/DeveloperPolicy.rst
657 ↗(On Diff #182326)

<del>allow</del><ins>potential for</ins> LLVM to use newer C++ language or library features

677 ↗(On Diff #182326)

Motivations for a version increase should not be confused with the results of a version increase. If a non-motivating feature is "unlocked" by the version increase, for example, that feature would not necessarily have been mentioned prior to reaching this step. I do not see why such a feature then automatically becomes "fair game".

An explicit discussion of the language support necessary allows users of compilers other than the "popular toolchains" to evaluate the requirements and communicate with their vendors.

jfb updated this revision to Diff 182415.Jan 17 2019, 4:16 PM
jfb marked an inline comment as done.
  • Clarify how new language features get in.
jfb marked 3 inline comments as done.Jan 17 2019, 4:17 PM
jfb added inline comments.
docs/DeveloperPolicy.rst
677 ↗(On Diff #182326)

That's a pretty adversarial reading of the policy... I've clarified the wording to avoid that pitfall.

smeenai added inline comments.
docs/DeveloperPolicy.rst
648 ↗(On Diff #182415)

Grammar nit: You either want a "Since" before the "Requiring", or a "so" after the comma and before the "it", otherwise the sentence flows weirdly.

hubert.reinterpretcast marked an inline comment as done.Jan 17 2019, 7:58 PM
hubert.reinterpretcast added inline comments.
docs/DeveloperPolicy.rst
670 ↗(On Diff #182415)

I believe we should be clear as to whether this means one release cycle where the soft-error has been present since the last release branched, or if we mean that the soft-error made it into one release branch (no matter how late the soft-error was added).

For example: If the soft-error is added late in the development cycle of a release (say, to 10.0 in December), does the proposed process imply that top-of-tree can have the hard-error by next January with a soft-error only added over the holiday season?

674 ↗(On Diff #182415)

This does mean that release-bound developers (who do not look at top-of-tree, release candidates, or the mailing list) will, on discovering that they might not be able to move to the next release (thanks to the warning), find that top-of-tree has already been unusable for them for around 2-3 months (or whatever amount of time it took for the release to ship after being branched from trunk). Which is to say that the warning truly is just telling them, and not really asking them.

677 ↗(On Diff #182326)

Thanks; this addresses the point I raised.

docs/DeveloperPolicy.rst
648 ↗(On Diff #182415)

I believe both since and so would be redundant with therefore. The flow can be fixed by using "; therefore, it will" instead of ", it will therefore".

jfb updated this revision to Diff 182454.Jan 17 2019, 8:27 PM
jfb marked 7 inline comments as done.
  • gramer
jfb added a subscriber: hans.Jan 17 2019, 8:27 PM
jfb added inline comments.
docs/DeveloperPolicy.rst
670 ↗(On Diff #182415)

That's really up to the release manager, I don't think we need to nail down every single eventuality. In particular, to really figure out the right thing to do we'd need to know when people compile clang branches: do they do so early or late, or only when the branch is deemed "done"? If say a Linux distro only takes the branch once "done", then your point is totally moot. And say they get bit by something, and decide to start compiling them earlier, then whatever policy is now based on outdated facts.

There's so many eventualities that I really don't think we need to decide on a policy. The release manager is smart, let's trust them. @hans WDYT?

674 ↗(On Diff #182415)

Correct.

lattner accepted this revision.Jan 17 2019, 10:26 PM
lattner added a subscriber: lattner.

Looks great!

docs/DeveloperPolicy.rst
671 ↗(On Diff #182190)

It still says tip-of-tree. I think tip-of-tree is better as tip has a particular meaning in Git, but either way it should be consistent with "Keep the t[io]p of tree as stable as possible." on line 25.

hans marked an inline comment as done.Jan 18 2019, 2:14 AM
hans added inline comments.
docs/DeveloperPolicy.rst
670 ↗(On Diff #182415)

I think the text as proposed here is fine.

jfb updated this revision to Diff 182541.Jan 18 2019, 9:34 AM
jfb marked an inline comment as done.
  • top
jfb added a comment.Jan 18 2019, 9:34 AM

Address typo correctly this time!

So far we have ✅ from @erichkeane @lhames @lattner on this review, and +1 / LGTM on the llvm-dev RFC from @reames @jyknight @chandlerc @JonasToth @mehdi_amini. I haven't seen any pushback, and I think I've properly addressed all suggestions received so far. I'll leave the patch here over the weekend, and will commit on Monday assuming nobody else chimes in. I'll then send a follow-up RFC proposing that we migrate LLVM past C++11, following the process outlined here. Thanks all!

Looks good to me too. (don't think i should formally accept this though)

docs/DeveloperPolicy.rst
660 ↗(On Diff #182541)

Passing-by remark: it may be a good idea to consider debian stable too.

jfb marked 2 inline comments as done.Jan 18 2019, 9:54 AM
jfb added inline comments.
docs/DeveloperPolicy.rst
660 ↗(On Diff #182541)

I don't want a list here because over time what matters will likely change. "e.g." is just an example. I expect us to be smart about this and not need everything spelled out in details.

In the follow-up RFC I'll send next week, I plan to use the same table of *nix distros that I had in my BoF: RHEL, Debian, OpenBSD, Ubuntu (as well as GCC, LLVM, and MSVC mentions). I'd expect future RFCs to use a similar list, adding / removing as appropriate.

erichkeane added inline comments.Jan 18 2019, 9:59 AM
docs/DeveloperPolicy.rst
660 ↗(On Diff #182541)

No change in this patch, but I'd suggest that the first successful example of this happening be linked here. It would be wonderful if said RFC had a fairly easy to update "Form" that'll make future updates more clear to review.

jfb marked 3 inline comments as done.Jan 18 2019, 10:16 AM
jfb added inline comments.
docs/DeveloperPolicy.rst
660 ↗(On Diff #182541)

That's a great idea. That makes me think that the RFC should conclude with a summary of what was decided, which can be the template for the next time around (and we'd link to it from this doc).

This revision was automatically updated to reflect the committed changes.
jfb marked an inline comment as done.
jfb added a comment.Jan 21 2019, 3:55 PM

Thanks all for the comments! I'll work on sending out and RFC which follows this new policy.

jfb added a comment.Jan 25 2019, 3:58 PM

For future reference: