This is an archive of the discontinued LLVM Phabricator instance.

Differential D123957

Update the developer policy to mention release notes
ClosedPublic

Authored by aaron.ballman on Apr 18 2022, 1:06 PM.

Details

Reviewers
lattner
tstellar
jhenderson
nikic
Commits
rG009048ab314f: Update the developer policy to mention release notes
Summary

As a project, Clang has gotten negative public feedback about our lack of communicating changes to users. There are comments on places like Hacker News or Reddit where users have (rightfully) been confused as to what changes happen in a given release, leading to misinformation like Clang not adding support for C++20 features: https://news.ycombinator.com/item?id=28761464.

This documents the expectation that changes which impact users should have release notes, and it's normal for code reviewers to ask an author to add a release note for a given change.

This addresses: https://github.com/llvm/llvm-project/issues/54965

Diff Detail

Event Timeline

aaron.ballman created this revision.Apr 18 2022, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2022, 1:06 PM
aaron.ballman requested review of this revision.Apr 18 2022, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2022, 1:06 PM
Harbormaster completed remote builds in B160104: Diff 423455.Apr 18 2022, 2:20 PM
tstellar accepted this revision.Apr 18 2022, 4:25 PM
tstellar added a subscriber: tstellar.

LGTM.

This revision is now accepted and ready to land.Apr 18 2022, 4:25 PM
hans added a subscriber: hans.Apr 19 2022, 1:26 AM

If it's possible, maybe this could be worded in a way that strongly suggests the release notes should ideally be updated in the same patch as the code change?
I think we're generally good at doing this for tests. It would be great if we could treat release notes the same.

jhenderson accepted this revision.Apr 19 2022, 2:12 AM
jhenderson added a subscriber: jhenderson.

LGTM, with suggested nits.

llvm/docs/DeveloperPolicy.rst
188
192

I think official coding documentation style guides say to use "command-line" - at least, that's what our internal docs team has told me in the past.

aaron.ballman updated this revision to Diff 423593.Apr 19 2022, 4:26 AM
aaron.ballman marked 2 inline comments as done.

Updating based on review feedback.

Fixed some grammar issues, added a bullet for fixing bugs, modified the bullet about diagnostics to include regrouping under a new diagnostic name.

Harbormaster completed remote builds in B160212: Diff 423593.Apr 19 2022, 4:27 AM
aaron.ballman updated this revision to Diff 423594.Apr 19 2022, 4:29 AM

Something went funny during a rebase, so this is the full update, not just changes since last time.

aaron.ballman added a subscriber: xbolva00.Apr 19 2022, 4:29 AM
In D123957#3458354, @hans wrote:

If it's possible, maybe this could be worded in a way that strongly suggests the release notes should ideally be updated in the same patch as the code change?
I think we're generally good at doing this for tests. It would be great if we could treat release notes the same.

That's a good idea! This was also independently suggested by @xbolva00 on the RFC thread.

hans added a comment.Apr 19 2022, 4:32 AM

Looks great to me!

Harbormaster completed remote builds in B160213: Diff 423594.Apr 19 2022, 5:03 AM
nikic requested changes to this revision.Apr 19 2022, 5:32 AM
nikic added a subscriber: nikic.
nikic added inline comments.
llvm/docs/DeveloperPolicy.rst
195

I disagree with this point. Bug fixes should not make it into the release notes as a matter of course -- there may be specific high-impact bug fixes that may be worth mentioning, but bug fixes should not be included in the release notes as a matter of policy.

I agree that release notes for Clang/LLVM are currently insufficient, but we should also be careful not to over-compensate in the other direction, but including too much irrelevant noise.

This revision now requires changes to proceed.Apr 19 2022, 5:32 AM
aaron.ballman added inline comments.Apr 19 2022, 5:39 AM
llvm/docs/DeveloperPolicy.rst
195

I disagree with this point. Bug fixes should not make it into the release notes as a matter of course -- there may be specific high-impact bug fixes that may be worth mentioning, but bug fixes should not be included in the release notes as a matter of policy.

I disagree (quite strongly) with this and would point out that users have explicitly requested this information be included: https://discourse.llvm.org/t/rfc-update-developer-policy-on-release-notes/61856/3

I agree that release notes for Clang/LLVM are currently insufficient, but we should also be careful not to over-compensate in the other direction, but including too much irrelevant noise.

Bug fixes are generally far from irrelevant, but I agree that reviewer and author discretion is advised (as with any release note). For example, if you introduce a bug in version N and fix the bug in version N, there's no need for a release note in that situation because there's no user-facing change as far as users care about. But I don't want to try to spell that out in precise detail because there will always be edge cases.

lattner added a comment.Apr 19 2022, 10:45 AM

This is awesome, I agree completely we should curate release notes better. That said, I think this should make it more clear that there is a "difference in kind" between user-facing tools like clang/lldb etc and other libraries in LLVM. We don't want release note burden (or noise) for every little thing going into the optimizer or codegen. Do you think it would make sense to point out that this is about user-facing tools?

llvm/docs/DeveloperPolicy.rst
195

I agree with both of you - listing every bug report would be too noisy and pretty irrelevant for most users, but there are high impact ones that are important and valuable to list. How about wording this bullet something like:

  • Fixing high impact bugs, e.g. ones that get discussed on hackernews or comparable forums (please link to the issue fixed in the bug database).
196

This is also too broad IMO, we don't want every new instcombine listed. I'd recommend making this more user-centric, e.g. "Adding or removing optimizations that have widespread impact or enables new programming paradigms"

lattner added a comment.Apr 19 2022, 10:46 AM

Also, when this lands, we should post on the forum about it. Every change to the developer policy warrants broader visibility than just a phab discussion IMO.

aaron.ballman added a comment.Apr 19 2022, 11:00 AM
In D123957#3459557, @lattner wrote:

This is awesome, I agree completely we should curate release notes better. That said, I think this should make it more clear that there is a "difference in kind" between user-facing tools like clang/lldb etc and other libraries in LLVM. We don't want release note burden (or noise) for every little thing going into the optimizer or codegen. Do you think it would make sense to point out that this is about user-facing tools?

Oh! I see now where the disconnect was, thanks! That's a good point about user-facing vs not. I'll try to clarify.

In D123957#3459561, @lattner wrote:

Also, when this lands, we should post on the forum about it. Every change to the developer policy warrants broader visibility than just a phab discussion IMO.

Heh, I linked to the phab patch from the RFC but forgot to link to the RFC from the phab patch. Sorry about that! You can find the RFC discussion here: https://discourse.llvm.org/t/rfc-update-developer-policy-on-release-notes/

llvm/docs/DeveloperPolicy.rst
195

Thanks for clarifying where I was misunderstanding before. I agree there needs to be a change to my wording, but I'd prefer if we went with something more like:

  • Fixing a bug that potentially has significant user-facing impact (please link to the issue fixed in the bug database).

(I mostly want to avoid making it sound like the bug has to be a stop-the-world bug in order to warrant a release note. Functionally, I think fixing a bug in Clang should almost always have a release note, but the same may not be true for fixing a bug in LLVM where the difference in behavior may not be particularly observable to users.)

Would this address your concerns @nikic?

196

+1, good suggestion!

aaron.ballman updated this revision to Diff 423683.Apr 19 2022, 11:14 AM
aaron.ballman marked 3 inline comments as done.

Updated based on review feedback.

  • Made it more clear that adding a release note is discretionary rather than mandatory
  • Clarified that bug fix release notes should be for user-facing impacts rather than any bug fix
  • Clarified that optimization pass release notes should be for user-facing impacts rather than any change
Harbormaster completed remote builds in B160284: Diff 423683.Apr 19 2022, 11:57 AM
nikic accepted this revision.Apr 19 2022, 12:07 PM
nikic added inline comments.
llvm/docs/DeveloperPolicy.rst
195

The new wording looks good to me, thanks!

This revision is now accepted and ready to land.Apr 19 2022, 12:07 PM
xbolva00 added a comment.Apr 19 2022, 12:25 PM

Can you please add link to RFC to commit message?

Otherwise looks good

tschuett added a subscriber: tschuett.Apr 19 2022, 12:37 PM

LGTM and thanks.

Feel free to ignore my comment. Maybe sort entries by importance. I have problems reading longish texts and documents.

lattner accepted this revision.Apr 19 2022, 1:27 PM

Nice, LGTM!

This revision was landed with ongoing or failed builds.Apr 20 2022, 8:36 AM
Closed by commit rG009048ab314f: Update the developer policy to mention release notes (authored by aaron.ballman). · Explain Why
This revision was automatically updated to reflect the committed changes.
aaron.ballman added a commit: rG009048ab314f: Update the developer policy to mention release notes.
aaron.ballman added a comment.Apr 20 2022, 8:37 AM

Thank you to everyone for the great feedback!

In D123957#3459975, @xbolva00 wrote:

Can you please add link to RFC to commit message?

Good call, I added that when landing.

Revision Contents

PathSize
llvm/
docs/
24 lines

Diff 423919

llvm/docs/DeveloperPolicy.rst

Show First 20 Lines • Show All 174 Lines • ▼ Show 20 Lines* Test cases, especially for regressions, should be reduced as much as possible,
burden on all developers. Please keep them short. burden on all developers. Please keep them short.
Note that llvm/test and clang/test are designed for regression and small feature Note that llvm/test and clang/test are designed for regression and small feature
tests only. More extensive test cases (e.g., entire applications, benchmarks, tests only. More extensive test cases (e.g., entire applications, benchmarks,
etc) should be added to the ``llvm-test`` test suite. The llvm-test suite is etc) should be added to the ``llvm-test`` test suite. The llvm-test suite is
for coverage (correctness, performance, etc) testing, not feature or regression for coverage (correctness, performance, etc) testing, not feature or regression
testing. testing.
Release Notes
-------------
Many projects in LLVM communicate important changes to users through release
notes, typically found in ``docs/ReleaseNotes.rst`` for the project. Changes to
a project that are user-facing, or that users may wish to know about, should be
jhendersonUnsubmitted
Done
ReplyInline Actions
notes, typically found in ``docs/ReleaseNotes.rst`` for the project. Changes to
- a project that are user-facing or users may wish to know about should be added
+ a project that are user-facing, or that users may wish to know about, should be added
to the project's release notes. Examples of changes that would typically
jhenderson:
added to the project's release notes at the author's or code reviewer's
discretion, preferably as part of the commit landing the changes. Examples of
changes that would typically warrant adding a release note (this list is not
exhaustive):
jhendersonUnsubmitted
Done
ReplyInline Actions
warrant adding a release note (this list is not exhaustive):
- * Adding, removing, or modifying command line options.
+ * Adding, removing, or modifying command-line options.
* Adding or removing a diagnostic.

I think official coding documentation style guides say to use "command-line" - at least, that's what our internal docs team has told me in the past.

jhenderson: I think official coding documentation style guides say to use "command-line" - at least, that's…
* Adding, removing, or modifying command-line options.
* Adding, removing, or regrouping a diagnostic.
nikicUnsubmitted
Done
ReplyInline Actions

I disagree with this point. Bug fixes should not make it into the release notes as a matter of course -- there may be specific high-impact bug fixes that may be worth mentioning, but bug fixes should not be included in the release notes as a matter of policy.

I agree that release notes for Clang/LLVM are currently insufficient, but we should also be careful not to over-compensate in the other direction, but including too much irrelevant noise.

nikic: I disagree with this point. Bug fixes should not make it into the release notes as a matter of…
aaron.ballmanAuthorUnsubmitted
Done
ReplyInline Actions

I disagree with this point. Bug fixes should not make it into the release notes as a matter of course -- there may be specific high-impact bug fixes that may be worth mentioning, but bug fixes should not be included in the release notes as a matter of policy.

I disagree (quite strongly) with this and would point out that users have explicitly requested this information be included: https://discourse.llvm.org/t/rfc-update-developer-policy-on-release-notes/61856/3

I agree that release notes for Clang/LLVM are currently insufficient, but we should also be careful not to over-compensate in the other direction, but including too much irrelevant noise.

Bug fixes are generally far from irrelevant, but I agree that reviewer and author discretion is advised (as with any release note). For example, if you introduce a bug in version N and fix the bug in version N, there's no need for a release note in that situation because there's no user-facing change as far as users care about. But I don't want to try to spell that out in precise detail because there will always be edge cases.

aaron.ballman: > I disagree with this point. Bug fixes should not make it into the release notes as a matter…
lattnerUnsubmitted
Done
ReplyInline Actions

I agree with both of you - listing every bug report would be too noisy and pretty irrelevant for most users, but there are high impact ones that are important and valuable to list. How about wording this bullet something like:

  • Fixing high impact bugs, e.g. ones that get discussed on hackernews or comparable forums (please link to the issue fixed in the bug database).
lattner: I agree with both of you - listing every bug report would be too noisy and pretty irrelevant…
aaron.ballmanAuthorUnsubmitted
Done
ReplyInline Actions

Thanks for clarifying where I was misunderstanding before. I agree there needs to be a change to my wording, but I'd prefer if we went with something more like:

  • Fixing a bug that potentially has significant user-facing impact (please link to the issue fixed in the bug database).

(I mostly want to avoid making it sound like the bug has to be a stop-the-world bug in order to warrant a release note. Functionally, I think fixing a bug in Clang should almost always have a release note, but the same may not be true for fixing a bug in LLVM where the difference in behavior may not be particularly observable to users.)

Would this address your concerns @nikic?

aaron.ballman: Thanks for clarifying where I was misunderstanding before. I agree there needs to be a change…
nikicUnsubmitted
Not Done
ReplyInline Actions

The new wording looks good to me, thanks!

nikic: The new wording looks good to me, thanks!
* Fixing a bug that potentially has significant user-facing impact (please link
lattnerUnsubmitted
Done
ReplyInline Actions

This is also too broad IMO, we don't want every new instcombine listed. I'd recommend making this more user-centric, e.g. "Adding or removing optimizations that have widespread impact or enables new programming paradigms"

lattner: This is also too broad IMO, we don't want every new instcombine listed. I'd recommend making…
aaron.ballmanAuthorUnsubmitted
Done
ReplyInline Actions

+1, good suggestion!

aaron.ballman: +1, good suggestion!
to the issue fixed in the bug database).
* Adding or removing optimizations that have widespread impact or enables new
programming paradigms.
* Modifying a C stable API.
* Notifying users about a potentially disruptive change expected to be made in
a future release, such as removal of a deprecated feature.
Code reviewers are encouraged to request a release note if they think one is
warranted when performing a code review.
Quality Quality
------- -------
The minimum quality standards that any change must satisfy before being The minimum quality standards that any change must satisfy before being
committed to the main development branch are: committed to the main development branch are:
#. Code must adhere to the `LLVM Coding Standards <CodingStandards.html>`_. #. Code must adhere to the `LLVM Coding Standards <CodingStandards.html>`_.
▲ Show 20 Lines • Show All 1,009 Lines • Show Last 20 Lines