doc: Update the CONTRIBUTING.md to follow Conventional Commits

[acassis]

Summary

Following this convention will let NuttX commits to be easily parsed by standard tools: https://www.conventionalcommits.org/en/v1.0.0/

Impact

Avoids using "[BREAKING]" in the commit title table sends a bad message for anyone reading the git history, also avoid cluttering the title, specially when more tags are used, since the title should be restricted to 50 chars.

Testing

N/A

[cederom]

• Thanks @acassis, since requirement for marking breaking changes is part of Contributing Guide for 3 months but everyone seems to ignore marking, maybe other way will work.
• There is nothing wrong in [BREAKING] mark, no bad message, quite opposite, someone who finds their code broken after update will easily find possible cause that way.
• I do insist to mark breaking changes in git commit topic and PR message title as this is first place to search (git log search and changelog).
• Putting "BREAKING CHANGE" in the git commit message may be okay, but we also need this "!" mark before ":" as stated in https://www.conventionalcommits.org/en/v1.0.0/.

[acassis]

We also need "!" mark before ":" in the git commit topic for breaking changes as stated by [1]. Otherwise we are hiding breaking changes and this is not the goal, these should be clearly visible.

[1] https://www.conventionalcommits.org/en/v1.0.0/

• Thanks @acassis, since requirement for marking breaking changes is part of Contributing Guide for 3 months but everyone seems to ignore marking, maybe other way will work.

  • There is nothing wrong in [BREAKING] mark, no bad message, quite opposite, someone who finds their code broken after update will easily find possible cause that way.

  • I do insist to mark breaking changes in git commit topic and PR message title as this is first place to search (git log search and changelog).

@cederom the issues are those I submitted to the mailing list:

• It passes a wrong message, as something very negative (not all breaking are bad, or shouldn't be)
• Someone reading our git history could get a wrong impression of the project
• It will cluttering the title, by convention the title should have only 50 chars
• It doesn't follow the conventional commits specification: https://www.conventionalcommits.org/en/v1.0.0/

* Putting "BREAKING CHANGE" in the git commit message may be okay, but we also need this "!" mark before ":" as stated in https://www.conventionalcommits.org/en/v1.0.0/.

It is not mandatory: please see items 12 and 13 of specification. If "!" is included in the commit title the "BREAKING CHANGE: " in the foot are not required. Just it! :-)

[cederom]

@acassis: It is not mandatory: please see items 12 and 13 of specification. If "!" is included in the commit title the "BREAKING CHANGE: " in the foot are not required. Just it! :-)

I am for placing both "!:" in the git commit title and "BREAKING CHANGE" mark in the git commit body. "!:" is not self-explanatory, but if someone sees two git commits in the logs with both "!:" and "BREAKING CHANGE" they will quickly associate.

We cannot hide breaking changes, these must be clearly exposed and easy to find. People with broken code will search for 1. git log, 2. changelog based on PR titles, so both git commits and PR titles should follow.

This "BREAKING CHANGE: blah blah" part in the footer with description is a bit awkward because upper part of commit message already contains description what was changed and why. Thus only BREAKING CHANGE mark seems necessary if someone git --grep based on "!:" or "BREAKING" string and if someone does not yet know what the "!" in topic means.

[acassis]

@acassis: It is not mandatory: please see items 12 and 13 of specification. If "!" is included in the commit title the "BREAKING CHANGE: " in the foot are not required. Just it! :-)

I am for placing both "!:" in the git commit title and "BREAKING CHANGE" mark in the git commit body. "!:" is not self-explanatory, but if someone sees two git commits in the logs with both "!:" and "BREAKING CHANGE" they will quickly associate.

We cannot hide breaking changes, these must be clearly exposed and easy to find. People with broken code will search for 1. git log, 2. changelog based on PR titles, so both git commits and PR titles should follow.

This "BREAKING CHANGE: blah blah" part in the footer with description is a bit awkward because upper part of commit message already contains description what was changed and why. Thus only BREAKING CHANGE mark seems necessary if someone git --grep based on "!:" or "BREAKING" string and if someone does not yet know what the "!" in topic means.

@cederom the commit message normally says what was done, the message after "BREAKING CHANGE: " will give a hint to user how a feature was used in the past and how it is used now.

The breaking features is not supposed to be hidden, but they we don't need to be in the title :)

[linguini1]

Could you modify the git commit message template file in this repository to also remind contributors at commit time to mark the breaking change?

.gitmessage

[cederom]

Update from the mailing list: @jerpelea proposes to use ! (exclamation mark) as the first character of git commit topic and PR title for breaking changes.

I like the idea best as it is most versatile no matter what comes after, costs only single char, and its easy to parse.

But it does not fully align with Conventional Commits spec.. and so we may propose that idea over there? :-)

I also agree that "BREAKING CHANGE" should be put somewhere in the git commit message and PR description, just in case someone does not know what the "!" means, or parses logs by "BREAKING" string. If we want to put this in the footer as Conventional Commits declares, then a short sentence on how to fix should follow (i.e. BREAKING CHANGE: lvgl api changed from 8 to 9, update your code.).

[cederom]

@acassis how Release Notes are generated? These are generated from Pull Requests right?

Thus we need this mark both in git commit and pull request. I am asking for this like 20 times already :D

Thus my proposition for the point 9:

9. Breaking Changes must be clearly marked both in Git Commits and Pull Requests: 1. Put ! (exclamation mark) as first character of the title; 2. Put BREAKING CHANGE: in the body with a brief change description and the quick-fix instructions. This helps users identify areas that need an update on their side, grep git logs, and auto-generate Release Notes.

[acassis]

@cederom @linguini1 please re-review and remove the change request

[acassis]

@acassis how Release Notes are generated?

It doesn't change the way we already generate it (BTW I never generate it, so your question should be redirected to @jerpelea :-D ). Later on we can modify the existing script to automatically detect the "!" and warn that the commit is a breaking change.

[cederom]

@acassis how Release Notes are generated?

It doesn't change the way we already generate it (BTW I never generate it, so your question should be redirected to @jerpelea :-D ). Later on we can modify the existing script to automatically detect the "!" and warn that the commit is a breaking change.

@jerpelea can you please explain, looks like I cannot :-)

[simbit18]

hi @cederom If I'm not mistaken, @jerpelea uses this tool in Python.

https://github.com/apache/nuttx/blob/master/tools/doreleasenotes.py

[cederom]

If you want to remove breaking mark ("!") from PR titles and leave only in git commits then I will follow the community voice. Should we vote that change on the mailing list?

[acassis]

If you want to remove breaking mark ("!") from PR titles and leave only in git commits then I will follow the community voice. Should we vote that change on the mailing list?

Nobody is proposing it Tomek! The exclamation mark will be needed by the Release Notes script...

Could you please look at the code Luke! https://github.com/apache/nuttx/pull/16823/files

[cederom]

If you want to remove breaking mark ("!") from PR titles and leave only in git commits then I will follow the community voice. Should we vote that change on the mailing list?

Nobody is proposing it Tomek! The exclamation mark will be needed by the Release Notes script...

Could you please look at the code Luke! https://github.com/apache/nuttx/pull/16823/files

Release Notes are generated from Pull Requests not git commits:

• See https://nuttx.apache.org/releases/12.10.0/.
• Note the list contains links to Pull Request URL on GitHub. You can click a number before change title and you are redirected to a Pull Request.
• See the RN generation script https://github.com/apache/nuttx/blob/master/tools/doreleasenotes.py, the commit is only used to obtain date not the title.

1. Original requirement text - there is git commit and PR title:

9. Breaking Change must be clearly marked with a [BREAKING] tag in the git commit topic and PR title that will propagate to Release Notes.

2. New requirement proposition - no PR title here just git commit:

Breaking change must be clearly identified with inclusion of an exclamation mark ! as the first character of the commit title and a BREAKING CHANGE: in the git commit log message, followed by a short comment about how the user should adapt their code. This change description will be used to create the Release Notes.

3. My proposition - both git commit and PR title as before change:

Breaking Changes must be clearly marked both in Git Commits and Pull Requests: 1. Put ! (exclamation mark) as first character of the title; 2. Put BREAKING CHANGE: in the body with a brief change description and the quick-fix instructions. This helps users identify areas that need an update on their side, grep git logs, and auto-generate Release Notes.

This "!" (+"BREAKING CHANGE:") mark should be both part of Git Commit and the Pull Request too as this tells more attention needed here when checking, and it aligns with existing release process as explained above. This is my last message about this.

Let @jerpelea decide he is the Release Manager :-)

[acassis]

@cederom a PR could contains more than one commit, and more than one commit can break different things. Currently we don't track Break Changes, so the script will need to be updated anyway. Then we modify it to detect breaks on commit instead of PR

BTW, looking at https://github.com/apache/nuttx/blob/master/tools/doreleasenotes.py seems like it go by commits, not PR

[acassis]

@linguini1 please review, I already fixed the suggest.

@cederom please review and remove Change Request since the modification as implemented as suggested and adding it to PR too as you later suggested is redundant.

[cederom]

@jerpelea ?

[acassis]

Ok @jerpelea, let us to discuss there, but I think there is no way to prevent people to create a PR with more than one commit. And some cases it is necessary to have more than one commit to avoid compilation errors (when you modify and driver and need to modify the board files).

You cannot include the board modifications in the driver commit and you cannot separate it in a new PR otherwise the driver PR will break the board building.

[jerpelea]

@acassis case by case we should accept more than 1 commit if the breaking change is in a sequence. I was referring to the case when 1 commit is breaking and the rest are fixing bugs or adding features

[acassis]

I think we should merge this PR and then use @raiden00pl script to automatically indicate the [breaking changes] in the PR

[jerpelea]

@acassis please modify the PR to include

• ! in both the commit and PR title
• explanation for the reason to break compatibility

We need to send a clear message to our developers and users

[jerpelea]

@cederom any concerns?

[cederom]

Thanks @jerpelea my all remarks are above already, text proposition too :-) My main concern was about not only changing breaking change mark but also process (i.e. no mark in the PR which I know would impact release process, etc). I am happy we are back on the track already, thank you guys :-)

[jerpelea]

@acassis please update the commit to

• include the breaking information in both PR and commit
• the PR MUST contain only breaking changes (1 or more commits)

[acassis]

@jerpelea I will add the breaking information in the pull request, but asking people to include only breaking commits in the PR doesn't work: commits are separated by logical features, however adding a breaking will requires modifications in other files (and commits) to pass the CI.

Forcing a PR to have only breaking changes could means we will have to merge PR that doesn't pass in the CI, because it could requires board modifications that not necessarily are breaking commits.

@xiaoxiang781216 @hartmannathan @simbit18 @linguini1 @cederom what do you think? Does it make sense to force devs to include only 1 commit or only commits with breaking changes?

[cederom]

It sounds like when breaking change is provided, PR should only contain breaking change and nothing more, it should pass build, but should not contain any additional features or updates, any additional features or updates should be sent in another PR linked by "depends-on" or "provides" kind of reference, right @jerpelea ?

I think this is required by release process handling and backporting changes from master to a release branch, so PR with breaking change can be backported, verified, and then following related PRs can be backported?

Long story short the changes should be as small as possible but not smaller, that is they should pass build, but different changes should be provided with separate PRs, that is out current process already :-) But it may be good to underline that for breaking changes too :-)

[linguini1]

@xiaoxiang781216 @hartmannathan @simbit18 @linguini1 @cederom what do you think? Does it make sense to force devs to include only 1 commit or only commits with breaking changes?

I'm not sure as I'm not too familiar with the release process. It might be hard to separate in some cases. The example I can think of is a breaking change commit for some code + another commit to update the documentation to explain that breaking change. Logically these two would be separate commits since one is a code change and the other is a doc change, but I personally am of the opinion that code changes + corresponding doc change commits should be in the same PR so they are merged at once.

I'm not sure how much of a headache that is for @jerpelea in the release process though, maybe he could explain how it causes issues so we can brainstorm other solutions?

[acassis]

I think since the current release process only uses the PR title to be generated, it is ideal that each PR has only one commit, but life is no so simple. We need to fix the release process, not force people to create PRs with a single commit, that is the point! :-)

[cederom]

Don't fix stuff that is not broken :-P

[linguini1]

Well how difficult would it be to use the commit subject in releases instead of the PR title?

Maybe we can just follow @jerpelea 's request for now (breaking changes are rare so we probably won't encounter this multi commit issue right away) and then try to work towards using commit subjects instead of PR titles for the releases?