From 99c3bc2247050eb1d8ef3ed6ab665d455cfd4c1a Mon Sep 17 00:00:00 2001 From: Gustavo Romero Date: Fri, 12 Aug 2022 23:20:59 +0000 Subject: [PATCH 1/7] [RFC] Add Commit Message Guideline This RFC proposes adding a Commmit Message Guideline to Apache TVM documentation to help guide contributors on how to write good commit messages when submitting code / Pull Requests. Co-authored-by: Leandro Nunes --- rfcs/0088-commit-message-guideline.md | 181 ++++++++++++++++++++++++++ 1 file changed, 181 insertions(+) create mode 100644 rfcs/0088-commit-message-guideline.md diff --git a/rfcs/0088-commit-message-guideline.md b/rfcs/0088-commit-message-guideline.md new file mode 100644 index 00000000..5a81f6d1 --- /dev/null +++ b/rfcs/0088-commit-message-guideline.md @@ -0,0 +1,181 @@ +- Feature Name: Commit Message Guideline +- Start Date: 2022-08-12 +- RFC PR: [apache/tvm-rfcs#0000](https://github.com/apache/tvm-rfcs/pull/88) +- GitHub Issue: [apache/tvm#0000](https://github.com/apache/tvm/issues/0000) + +# Summary +[summary]: #summary + +This RFC proposes adding a Commmit Message Guideline to TVM documentation to +help guide contributors on how to write good commit messages when submitting +code / PRs (Pull Requests) to Apache TVM. + +# Motivation +[motivation]: #motivation + +Currently TVM commit logs are less than ideal because many commit messages lack +valuable information and don't follow any format standard. + +Valuable information is usually left behind in Github PR conversations or +discussion threads in the Discuss forum, making it hard to retrieve them when +inspecting the commit messages -- using `git log`, for instance. + +Because commit messages are an indirect but important aspect of code quality, +and also important for code maintenance, it is essential for a long term open +source project to ensure that they meet high standards. + +The importance of commit messages conveying enough context and information about +the code being changed will grow as the project grows and bad (poorly written) +commit messages can affect negatively the code quality of future changes that +would otherwise benefit from past good commit messages if they existed. + +Beyond code itself, poorly written commit messages can also affect the community +in other ways. For example, by not providing to new contributors a consistent +and complete history or context for the code changes, it can work as a barrier +for new contributions because much more time will be necessary trying to +understand what motivated a past critical but unclear change. + +Hence this Commit Message Guideline can help contributors to write good commit +messages and so improve the current situation regarding the TVM commit logs. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +Commit Message Guideline + +Apache TVM uses the Github (GH) platform for patch submission and code review +via Pull Requests (PRs). The final commit (title and body) that is merged into +the Apache TVM main tree is composed of the PR's title and body and must be kept +updated and reflecting the new changes in the code as per the reviews and +discussions. + +Although these guidelines apply essentially to the PRs’ title and body messages, +because GH auto-generates the PR’s title and body from the commits on a given +branch, it’s recommended to follow these guidelines right from the beginning, +when preparing commits in general to be submitted to the Apache TVM project. +This will ease the creation of a new PR, avoiding rework, and also will help the +review. + +The rules below will help to achieve uniformity that has several benefits, both +for review and for the code base maintenance as a whole, helping you to write +commit messages with a good quality suitable for the Apache TVM project, +allowing fast log searches, bisecting, and so on. + +_PR/commit title_: + +* Guarantee a title exists (enforced); +* Don’t use Github usernames in the title, like @username (enforced); +* Check if a tag should be present as a hint about what component(s) of the code + the commits “touch”. For example [BugFix], [CI], [microTVM], and [TVMC]. Tags + go between square brackets and appear first in the title. Tags help reviewers + to identify the PRs they can/want to review and also help the release folks + when generating the release notes (enforced); +* Use an imperative mood. Avoid titles like “Added operator X” and “Updated + image Y in the CI”, instead use the forms “Add feature X” and “Update image Y + in the CI” instead; +* Observe proper use of caps at the beginning (uppercase for the first letter) + and for acronyms, like, for instance, TVM, FVP, OpenCL. Hence instead of + “fix tvm use of opencl library”, write it as “Fix TVM use of OpenCL library”; +* No period at the end of the title is necessary. + +_PR/commit body_: + +* Guarantee a body exists (enforced); +* Don’t use Github usernames in body text, like @username (enforced); +* Avoid “bullet” commit message bodies: “bullet” commit message bodies are not + bad per se, but “bullet” commit messages without any description or + explanation is likely as bad as commits without any description, rationale, + or explanation in the body. + +For minor deviations from these guidelines, the community will normally favor +reminding the contributor of this policy over reverting or blocking a commmit / +PR. + +Commits and PRs without a title and/or a body are not considered minor +deviations from these guidelines and hence must be avoided. + +Most importantly, the contents of the commit message, especially the body, +should be written to convey the intention of the change, so it should avoid +being vague. For example, commits with a title like “Fix”, “Cleanup”, and +“Fix flaky test” and without any body text should be avoided. Also, for the +review, it will leave the reviewer wondering about what exactly was fixed or +changed and why the change is necessary, slowing the review. + +Below is an example that can be used as a model: + +> [microTVM] Zephyr: Remove zephyr_board option from build, flash, and open_transport methods +> +> Currently it’s necessary to pass the board type via ‘zephyr_board’ option to +> the Project API build, flash, and open_transport methods. +> +> However, since the board type is already configured when the project is +> created (i.e. when the generate_project method is called), it’s possible to +> avoid this redundancy by obtaining the board type from the project +> configuration files. +> +> This commit adds code to obtain the board type from the project CMake files, +> removing this option from build, flash, and open_transport methods, so it’s +> only necessary to specify the ‘zephyr_board’ option when calling +> generate_project. +> +> This commit also moves the ‘verbose’ and ‘west_cmd’ options from ‘build’ +> method to ‘generate_project’, reducing further the number of required options +> when building a project, since the ‘build’ method is usually called more often +> than the ‘generate_project’. + +If a PR is created from more than one commit (a patchset), then ideally the +changes should be split into commits in a reasonable way. For instance, if it’s +a fix and so it also adds a regression test, it’s a good practice to split the +changes having one commit for the fix and another one for the test, applying the +rules and recommendations in this guideline to each commit. + +After a new PR is created and the review starts it’s common that reviewers will +request changes. Usually the author will address the reviewers’ comments and +push additional commits on top of the initial ones. For these additional commits +there is no recommendation regarding the commit messages. The only +recommendation is that if the additional commits render the PR title and/or body +outdated then the PR title and body must reflect the new changes in the code and +be updated accordingly (remember that the PR title and body will be used to +compose the final commit message that will land in the main tree). + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +TVM Community must reach a certain concensus about the rules in this guideline, +hence this RFC will be voted. + +Once it's voted and approved the Commit Message Guideline text will be added to +`./docs/contribute/pull_request.rst` doc, under section 'Submit a Pull Request', +below subsection 'Guidelines', as a subsection named “Commit Message Guideline”. +The text in the second-last item in subsection 'Guidelines' that mentions PR +tags will also be extended (a hyperlink will be added) to refer to this +guideline, since it also contains guidelines about use of tags. + +New contributors can consult the Commit Message Guidelilne before submitting +PRs. Also, committers and reviewers can use this guideline when reviewing PRs in +case some clarification or help is necessary about how an author or contributor +should write or improve the PR's title and body. + +# Drawbacks +[drawbacks]: #drawbacks + +None. + +# Rationale and alternatives +[rationale-and-alternatives]: #rationale-and-alternatives + +# Prior art +[prior-art]: #prior-art + +This guideline is similar to other ones already being used succesfully in other +open source projects, like gcc, Zephyr, LLVM, and Linux. + +# Unresolved questions +[unresolved-questions]: #unresolved-questions + +# Future possibilities +[future-possibilities]: #future-possibilities + +A linter can also be used with tvm-bot to enforce some rules or aspects of these +guidelines, when pertinent. If that is implemented the rules enforced by the bot +would be those marked with "enforce" in the guideline. From b7ee4c7432a1f24f6304751af4acd3a4e2139512 Mon Sep 17 00:00:00 2001 From: Gustavo Romero Date: Tue, 16 Aug 2022 19:31:59 +0000 Subject: [PATCH 2/7] Remove out-of-scope paragraph As per the reviews, remove this paragraph since multiple commits / changes in a PR is out-of-scope, best fits in another guideline, and would require addtional discussions, needing a separate vote. --- rfcs/0088-commit-message-guideline.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/rfcs/0088-commit-message-guideline.md b/rfcs/0088-commit-message-guideline.md index 5a81f6d1..38a9cf79 100644 --- a/rfcs/0088-commit-message-guideline.md +++ b/rfcs/0088-commit-message-guideline.md @@ -123,12 +123,6 @@ Below is an example that can be used as a model: > when building a project, since the ‘build’ method is usually called more often > than the ‘generate_project’. -If a PR is created from more than one commit (a patchset), then ideally the -changes should be split into commits in a reasonable way. For instance, if it’s -a fix and so it also adds a regression test, it’s a good practice to split the -changes having one commit for the fix and another one for the test, applying the -rules and recommendations in this guideline to each commit. - After a new PR is created and the review starts it’s common that reviewers will request changes. Usually the author will address the reviewers’ comments and push additional commits on top of the initial ones. For these additional commits From afcbb981155b5a7eaf99c8306ee1cd2fb33aa463 Mon Sep 17 00:00:00 2001 From: Gustavo Romero Date: Wed, 17 Aug 2022 00:57:21 +0000 Subject: [PATCH 3/7] Be more explicit about committer and author responsibilities Be more explicit about committer and author responsibilities when the PR title and/or body need to get updated to be in sync with the code changes after a review cycle. --- rfcs/0088-commit-message-guideline.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/rfcs/0088-commit-message-guideline.md b/rfcs/0088-commit-message-guideline.md index 38a9cf79..fc579639 100644 --- a/rfcs/0088-commit-message-guideline.md +++ b/rfcs/0088-commit-message-guideline.md @@ -126,11 +126,18 @@ Below is an example that can be used as a model: After a new PR is created and the review starts it’s common that reviewers will request changes. Usually the author will address the reviewers’ comments and push additional commits on top of the initial ones. For these additional commits -there is no recommendation regarding the commit messages. The only -recommendation is that if the additional commits render the PR title and/or body -outdated then the PR title and body must reflect the new changes in the code and -be updated accordingly (remember that the PR title and body will be used to -compose the final commit message that will land in the main tree). +there is no recommendation regarding the commit messages. However if the +additional commits render the PR title and/or body outdated then it's the +author's responsibility to keep the PR title and body in sync with new changes +in the code and updated the PR title and body accordingly (remember that the PR +title and body will be used to compose the final commit message that will land +in the main tree). + +Committers will seek to fix any issues with the commit message prior to +committing but they retain the right to inform the author of the rules and +encourage them to follow them in future. Also, they retain the right to ask to +the author to update the PR title and/or body when they are not correctly +updated or fixed. # Reference-level explanation [reference-level-explanation]: #reference-level-explanation From 8486cc76cd7b76b244af02b561c889bfce95ea37 Mon Sep 17 00:00:00 2001 From: Gustavo Romero Date: Wed, 17 Aug 2022 22:06:33 +0000 Subject: [PATCH 4/7] Clarify about capitalization in tags Clarify about the preferred capitalization in tags. Also clarify about multiple tags and acronyms when used as a tag. --- rfcs/0088-commit-message-guideline.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/rfcs/0088-commit-message-guideline.md b/rfcs/0088-commit-message-guideline.md index fc579639..0497f914 100644 --- a/rfcs/0088-commit-message-guideline.md +++ b/rfcs/0088-commit-message-guideline.md @@ -67,9 +67,14 @@ _PR/commit title_: * Don’t use Github usernames in the title, like @username (enforced); * Check if a tag should be present as a hint about what component(s) of the code the commits “touch”. For example [BugFix], [CI], [microTVM], and [TVMC]. Tags - go between square brackets and appear first in the title. Tags help reviewers - to identify the PRs they can/want to review and also help the release folks - when generating the release notes (enforced); + go between square brackets and appear first in the title. If more than one tag + exist, multiple brackets should be used, like [BugFix][CI]. The case + recommended for tags, in geral, is the upper camel case. For example, prefer + the forms [Fix], [BugFix], and [Docker] instead of [fix], [bug_fix], and + [docker]. Acronyms should be kept as such so, for example, use [CI] and [TVMC] + instead of [ci] and [tvmc]. Tags help reviewers to identify the PRs they + can/want to review and also help the release folks when generating the release + notes (enforced); * Use an imperative mood. Avoid titles like “Added operator X” and “Updated image Y in the CI”, instead use the forms “Add feature X” and “Update image Y in the CI” instead; From bb5936e231b2f0f4f156c73f5ac2fc15ba4794fe Mon Sep 17 00:00:00 2001 From: Gustavo Romero Date: Thu, 18 Aug 2022 23:47:15 +0000 Subject: [PATCH 5/7] Be more emphatic about no need of a period at the end of the title Be more emphatic about no need of a period at the end of the title but don't enforce it. --- rfcs/0088-commit-message-guideline.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0088-commit-message-guideline.md b/rfcs/0088-commit-message-guideline.md index 0497f914..1c7fc656 100644 --- a/rfcs/0088-commit-message-guideline.md +++ b/rfcs/0088-commit-message-guideline.md @@ -81,7 +81,7 @@ _PR/commit title_: * Observe proper use of caps at the beginning (uppercase for the first letter) and for acronyms, like, for instance, TVM, FVP, OpenCL. Hence instead of “fix tvm use of opencl library”, write it as “Fix TVM use of OpenCL library”; -* No period at the end of the title is necessary. +* Do not put a period at the end of the title. _PR/commit body_: From 32527cf74bbdb8999b5a6ccc417292c56f574dda Mon Sep 17 00:00:00 2001 From: Gustavo Romero Date: Mon, 22 Aug 2022 15:35:28 +0000 Subject: [PATCH 6/7] Make it clear that tag is enforced but capitalization is not Original proposal for this RFC had tag presence as an enforced rule. Later, text on recommendation about tag capitalization was added to the rule and it became a tad unclear if the enforcement applies to the tag presence only or also to the tag capitalization text. This commit moves the "enforced" mark before the tag capitalization text to make it clearer that it applies to the tag presence only. It also tweaks a bit the text to avoid the idea that tag presence is optional -- since it's enforced, just like it was in the original proposal. --- rfcs/0088-commit-message-guideline.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/rfcs/0088-commit-message-guideline.md b/rfcs/0088-commit-message-guideline.md index 1c7fc656..8ea7a265 100644 --- a/rfcs/0088-commit-message-guideline.md +++ b/rfcs/0088-commit-message-guideline.md @@ -65,16 +65,16 @@ _PR/commit title_: * Guarantee a title exists (enforced); * Don’t use Github usernames in the title, like @username (enforced); -* Check if a tag should be present as a hint about what component(s) of the code - the commits “touch”. For example [BugFix], [CI], [microTVM], and [TVMC]. Tags - go between square brackets and appear first in the title. If more than one tag - exist, multiple brackets should be used, like [BugFix][CI]. The case - recommended for tags, in geral, is the upper camel case. For example, prefer - the forms [Fix], [BugFix], and [Docker] instead of [fix], [bug_fix], and - [docker]. Acronyms should be kept as such so, for example, use [CI] and [TVMC] - instead of [ci] and [tvmc]. Tags help reviewers to identify the PRs they - can/want to review and also help the release folks when generating the release - notes (enforced); +* A tag must be present as a hint about what component(s) of the code + the PRs / commits “touch” (enforced). For example [BugFix], [CI], [microTVM], + and [TVMC]. Tags go between square brackets and appear first in the title. If + more than one tag exist, multiple brackets should be used, like [BugFix][CI]. + The case recommended for tags, in geral, is the upper camel case. For example, + prefer the forms [Fix], [BugFix], and [Docker] instead of [fix], [bug_fix], + and [docker]. Acronyms should be kept as such so, for example, use [CI] and + [TVMC] instead of [ci] and [tvmc]. Tags help reviewers to identify the PRs + they can/want to review and also help the release folks when generating the + release notes; * Use an imperative mood. Avoid titles like “Added operator X” and “Updated image Y in the CI”, instead use the forms “Add feature X” and “Update image Y in the CI” instead; From 7649f8daee1b5e1bc99eb94a8810cff2cb7a25af Mon Sep 17 00:00:00 2001 From: Leandro Nunes Date: Tue, 23 Aug 2022 16:03:40 +0100 Subject: [PATCH 7/7] Update rfcs/0088-commit-message-guideline.md Co-authored-by: Siyuan Feng --- rfcs/0088-commit-message-guideline.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rfcs/0088-commit-message-guideline.md b/rfcs/0088-commit-message-guideline.md index 8ea7a265..dd32fa66 100644 --- a/rfcs/0088-commit-message-guideline.md +++ b/rfcs/0088-commit-message-guideline.md @@ -1,7 +1,6 @@ - Feature Name: Commit Message Guideline - Start Date: 2022-08-12 -- RFC PR: [apache/tvm-rfcs#0000](https://github.com/apache/tvm-rfcs/pull/88) -- GitHub Issue: [apache/tvm#0000](https://github.com/apache/tvm/issues/0000) +- RFC PR: [apache/tvm-rfcs#0088](https://github.com/apache/tvm-rfcs/pull/88) # Summary [summary]: #summary