Hi Varun,

> I think you just increased the scope of the problem. We should add that as a new requirement – the commit message header should be pretty.

I don't think the scope has increased, but perhaps the requirement that we are able to generate the changelog was lacking clarity; it's not necessarily that the commit message headers should be pretty, but that the changelog should remain so to the extent that it can - it is still user-facing documentation, after all. By extension, we gain nothing from using the commit log to generate the changelog if they just mirror one another.

> Honestly, we should also check if in an effort to make the changelog “pretty”, are we losing the traditional git log formatting. Honestly, the git log gets used more than the changelog, so your proposal of changing the commit header has a greater impact. I would like to make this low impact to the developers that create patches on a daily basis. 

The point I'm trying to emphasise is that there is no traditional Git log formatting - as it stands today, our commit guidelines make no mention of tags. As a result, the tags we do see vary drastically, from none at all to generic "TF-A:" tags, to platforms, drivers and sometimes to specific files. Everybody has their own status quo which they obviously want to maintain, but at some point we have to try to bring everybody onto the same page - commit style rules are not particularly rare for the same reasons code style rules aren't. I don't think the CC rules deviate all that much from the styles we most often see today.

> Introducing a completely new way of creating the commit message header or introducing more scripts to create that format is a no-no.

There are no mandatory scripts involved - you can continue to write your commits as you do today. The only tangible difference is that we are standardising the tag syntax.

> Personally, I feel that you are getting the required information from the git log by just adding tags, which to me, seems like a very low impact approach.

Incidentally, it was this very disagreement that brought on this investigation - you can see exactly what the v2.4 changelog looked like after basic categorisation, which is where it was decided that a straight dump of the commit log did not suffice for user documentation.

> Isnt that an easy fix? We just don’t add tags to such commits. I don’t see how “Conventional Commits” is better.

You can avoid tagging the revert commit, but you still need to detect whether the probably-tagged commit it reverts was merged before or after last release, and remove it from the log if the latter. I would suggest Conventional Commits is "better" because we don't even have to consider edge cases like these - we've done the configuration, we know it sorts this out for us, and there's nothing more we need to do to make it just work.

> As a maintainer, I feel that forcing developers to unlearn the standard way used by almost all other OSS projects, is disruptive. I am all for automating as long as the process does not get in my way every day.

But there is no "standard way" - some projects use "component: xyz" (e.g. Linux), others "[component] xyz" (e.g. LLVM), others yet don't use a tag at all (e.g. Mbed TLS), and I would argue most are like us: lax enough that it's largely down to individual contributors to determine their own. This just happens to be one style among many (and, as far as I know, the only one with an entire tooling ecosystem). I appreciate that you have a favoured variant, but I don't think it's any more useful to debate the most popular commit styles than it is to debate the most popular code styles.

As we can see today though, TF-A's existing commit guidelines go largely ignored, and it's our intention to rectify that in a way that allows us to do something useful with information that was previously inaccessible. I won't try to argue that enforcing something that wasn't previously enforced takes some initial getting used to, but I think the emphasis on the "extra work for committers" is severely overrepresented here - realistically, it's a minimal change to how we format the tags that we already write, and it's something some of us have already had to get used to (and have, honestly with very little effort).

> I think any proposal should be scalable and forward looking. I’m sure we will hit a scenario where someone needs custom tags and this proposal does not allow us that flexibility.

It does afford us that flexibility - we can extend the list of supported types, I'm just unsure of why we might. We would not have settled on this solution if we did not believe it to be scalable and, considering it does already see widespread usage, I would argue it's a relatively safe bet that it can handle most, if not all, of what we need now and in the future.

Chris
From: Varun Wadekar <vwadekar@nvidia.com>
Sent: 03 March 2021 01:26
To: Chris Kay <Chris.Kay@arm.com>; Gyorgy Szing <Gyorgy.Szing@arm.com>; tf-a@lists.trustedfirmware.org <tf-a@lists.trustedfirmware.org>
Cc: nd <nd@arm.com>
Subject: RE: [TF-A] Adoption of Conventional Commits
 

Hi Chris,

 

I think you just increased the scope of the problem. We should add that as a new requirement – the commit message header should be pretty.

 

Honestly, we should also check if in an effort to make the changelog “pretty”, are we losing the traditional git log formatting. Honestly, the git log gets used more than the changelog, so your proposal of changing the commit header has a greater impact. I would like to make this low impact to the developers that create patches on a daily basis. Introducing a completely new way of creating the commit message header or introducing more scripts to create that format is a no-no.

 

Personally, I feel that you are getting the required information from the git log by just adding tags, which to me, seems like a very low impact approach.

 

On the two examples, I don’t see a big difference in the supposedly human readable log you posted. But the proposal to get that is disruptive.

 

>> You can see here that it emphasises the scope for each change for human readability, and also omits both the revert commit and the commit it reverts because neither of them have been part of a release

 

Isnt that an easy fix? We just don’t add tags to such commits. I don’t see how “Conventional Commits” is better.

 

>> I think burdening reviewers with additional work is likely to prove unreliable, and certainly counter-productive if we can both largely automate the problem away and provide rapid feedback to developers before ever even having to push for review

 

As a maintainer, I feel that forcing developers to unlearn the standard way used by almost all other OSS projects, is disruptive. I am all for automating as long as the process does not get in my way every day.

 

>> The tooling I proposed does already offer some flexibility for defining our own types and scopes, though the default set is already pretty extensive

 

I think any proposal should be scalable and forward looking. I’m sure we will hit a scenario where someone needs custom tags and this proposal does not allow us that flexibility.

 

-Varun

 

From: Chris Kay <Chris.Kay@arm.com>
Sent: Tuesday, March 2, 2021 4:21 PM
To: Varun Wadekar <vwadekar@nvidia.com>; Gyorgy Szing <Gyorgy.Szing@arm.com>; tf-a@lists.trustedfirmware.org
Cc: nd <nd@arm.com>
Subject: Re: [TF-A] Adoption of Conventional Commits

 

External email: Use caution opening links or attachments

 

Hi guys,

 

Note: a lot of this email relies on HTML formatting to force monospace fonts and emphasis – it probably won’t show up correctly on the mailing lists archives.

 

One major point of contention with this model is that it’s not immediately clear what goes into the changelog. The obvious first answer is “the commit subject”, but let’s investigate that.

 

Here are the last 17 commits from upstream as of right now:

 

    plat/marvell/armada: cleanup MSS SRAM if used for copy

    plat/marvell: cn913x: allow CP1/CP2 mapping at BLE stage

    plat/marvell/armada/common/mss: use MSS SRAM in secure mode

    libc: memset: Fix MISRA issues

    plat:xilinx:zynqmp: Remove the custom crash implementation

    lib: cpus: aarch32: sanity check pointers before use

    Revert "spmd: ensure SIMD context is saved/restored on SPMC entry/exit"

    plat/arm/css: rename rd_n1e1_edge_scmi_plat_info array

    docs: stm32mp1: correct formatting issues

    marvell: uart: a3720: Increase TX FIFO EMPTY timeout from 2ms to 3ms

    marvell: uart: a3720: Update delay code to be compatible with 1200 MHz CPU

    marvell: uart: a3720: Fix comments in console_a3700_core_init() function

    nxp: added the makefile helper macros

    spmd: ensure SIMD context is saved/restored on SPMC entry/exit

    nand: stm32_fmc_nand: remove dead code

    plat/arm: juno: Refactor juno_getentropy()

    bl32: Enable TRNG service build

 

Here it is if we applied Conventional Commits to it:

 

    feat(marvell armada): cleanup MSS SRAM if used for copy

    feat(marvell cn913x): allow CP1/CP2 mapping at BLE stage

    feat(marvell armada): use MSS SRAM in secure mode

    fix(libc): fix MISRA issues

    refactor(xilinx zynqmp): remove the custom crash implementation

    refactor(aarch32): add sanity check pointers before use

    revert: fix(spmd): ensure SIMD context is saved/restored on SPMC entry/exit

    refactor(arm css): rename rd_n1e1_edge_scmi_plat_info array

    docs(stm32mp1): correct formatting issues

    refactor(marvell a3720): increase TX FIFO EMPTY timeout from 2ms to 3ms

    refactor(marvell a3720): update delay code to be compatible with 1200 MHz CPU

    fix(marvell a3720): fix comments in console_a3700_core_init() function

    build(nxp): add Makefile helper macros

    fix(spmd): ensure SIMD context is saved/restored on SPMC entry/exit

    refactor(stm32 fmc_nand): remove dead code

    refactor(arm juno): Refactor juno_getentropy()

    feat(bl32): Enable TRNG service build

 

Side note: the “screen real estate” concern some raised does not actually seem to manifest itself in any meaningful way – the longest line only increases by 3 characters, and the shortest line is actually reduced by 3 characters.

 

To me, immediately, the single subject style is much less mentally taxing to parse. Without it, there are at least four different schemes at play here that we need to interpret for every commit (and we’re only 17 commits deep!):

 

    foo/bar/baz: xyz
    foo: bar: baz: xyz

    foo/bar: baz: xyz

    Revert “xyz”

 

So, if we just forget about trying to read the history manually for a moment, without a standardised subject format we end up with a changelog that looks like this:

 

    Features:

 

        - plat/marvell/armada: cleanup MSS SRAM if used for copy

        - plat/marvell: cn913x: allow CP1/CP2 mapping at BLE stage

        - plat/marvell/armada/common/mss: use MSS SRAM in secure mode

        - bl32: Enable TRNG service build

 

    Bug Fixes:

 

        - libc: memset: Fix MISRA issues

        - marvell: uart: a3720: Fix comments in console_a3700_core_init() function

        - spmd: ensure SIMD context is saved/restored on SPMC entry/exit

 

    Build System:

 

        - nxp: added the makefile helper macros

 

    Code Refactoring:

 

        - plat:xilinx:zynqmp: Remove the custom crash implementation

        - lib: cpus: aarch32: sanity check pointers before use

        - plat/arm/css: rename rd_n1e1_edge_scmi_plat_info array

        - marvell: uart: a3720: Increase TX FIFO EMPTY timeout from 2ms to 3ms

        - marvell: uart: a3720: Update delay code to be compatible with 1200 MHz CPU

        - nand: stm32_fmc_nand: remove dead code

        - plat/arm: juno: Refactor juno_getentropy()

 

    Documentation:

 

        - docs: stm32mp1: correct formatting issues

 

   Reverts:

 

        - Revert "spmd: ensure SIMD context is saved/restored on SPMC entry/exit"

 

This, in my opinion, still suffers from the same problem: as a human, it’s difficult to interpret.

 

Compare that to how we expect that to look with Conventional Commits:

 

    Features:

 

        - bl32: enable TRNG service build

        - marvell armada: cleanup MSS SRAM if used for copy

        - marvell armada: use MSS SRAM in secure mode

        - marvell cn913x: allow CP1/CP2 mapping at BLE stage

 

    Bug Fixes:

 

        - libc: fix MISRA issues

        - marvell a3720: fix comments in console_a3700_core_init() function

 

    Build System:

 

        - nxp: add Makefile helper macros

 

    Code Refactoring:

 

        - aarch32: add sanity check pointers before use

        - arm css: rename rd_n1e1_edge_scmi_plat_info array

        - arm juno: refactor juno_getentropy()

        - marvell a3720: increase TX FIFO EMPTY timeout from 2ms to 3ms

        - marvell a3720: update delay code to be compatible with 1200 MHz CPU

        - stm32 fmc_nand: remove dead code

        - xilinx zynqmp: remove the custom crash implementation

 

    Documentation:

 

        - stm32mp1: correct formatting issues

 

… and I feel like the value prop of using robust tooling becomes more obvious – this tooling is intended not just to categorise commits, but to understand them. You can see here that it emphasises the scope for each change for human readability, and also omits both the revert commit and the commit it reverts because neither of them have been part of a release.

 

Additionally, without a way to enforce this, we’re not necessarily solving one of the current fundamental problems: our changelogs do not accurately and reliably reflect the changes to the project. I think burdening reviewers with additional work is likely to prove unreliable, and certainly counter-productive if we can both largely automate the problem away and provide rapid feedback to developers before ever even having to push for review.

 

Just a quick note on one of your points:

 

3. Flexibility to define project specific tags

 

The tooling I proposed does already offer some flexibility for defining our own types and scopes, though the default set is already pretty extensive:

 

  • build (Build System)
  • ci (Continuous Integration)
  • docs (Documentation)
  • feat (Features)
  • fix (Bug Fixes)
  • perf (Performance Improvements)
  • refactor (Code Refactoring)
  • revert (Reverts)
  • style (Styles)
  • test (Tests)

 

Chris

 

From: Varun Wadekar <vwadekar@nvidia.com>
Date: Tuesday, 2 March 2021 at 22:31
To: Gyorgy Szing <Gyorgy.Szing@arm.com>, Chris Kay <Chris.Kay@arm.com>, tf-a@lists.trustedfirmware.org <tf-a@lists.trustedfirmware.org>
Cc: nd <nd@arm.com>, nd <nd@arm.com>
Subject: RE: [TF-A] Adoption of Conventional Commits

Hi George,

 

Few clarifications.

 

>> it would need significant investments on tooling

 

[VW] I am not sure why you say that. The only expectation form tooling perspective is to run the ‘git log’ command before the release.

 

>> There is no tool which could help developers crafting the commit message in the right format

 

[VW] I don’t think we need a tool to generate commit messages with the right tags. I expect developers to add the tag manually as the footer. Maintainers will have to check that tags exist as part of the reviews.

 

>> Possibly the easiest would be to modify the javascript machinery available for Conventional Commits

 

[VW] I don’t think we need any tools from the “Conventional Commits” toolbox for this to work.

 

My proposal was from the requirements I heard from Chris in the meeting. If there are any implicit or obvious requirements that I missed, I propose we freeze them first. A solution can only work if the requirements are frozen.

 

-Varun

 

From: Gyorgy Szing <Gyorgy.Szing@arm.com>
Sent: Sunday, February 28, 2021 11:14 PM
To: Varun Wadekar <vwadekar@nvidia.com>; Chris Kay <Chris.Kay@arm.com>; tf-a@lists.trustedfirmware.org
Cc: nd <nd@arm.com>; nd <nd@arm.com>
Subject: RE: [TF-A] Adoption of Conventional Commits

 

External email: Use caution opening links or attachments

 

Hi Varun,

 

I really like your proposal, but it would need significant investments on tooling. There is no tool which could help developers crafting the commit message in the right format, there is no tool, which can validate the format (and be used i.e. as a git-hook), and there is no tool, which can generate the change history document from git history.

Can you please extend the proposal and turn it to be an end-to-end solution? Can you contribute tooling for commit message editing and validation, and for change log document generation? Possibly the easiest would be to modify the javascript machinery available for Conventional Commits. Can you contribute the needed changes?

 

/George

 

From: TF-A <tf-a-bounces@lists.trustedfirmware.org> On Behalf Of Varun Wadekar via TF-A
Sent: 26 February 2021 00:39
To: Chris Kay <Chris.Kay@arm.com>
Cc: tf-a@lists.trustedfirmware.org
Subject: Re: [TF-A] Adoption of Conventional Commits

 

Hi,

 

I really like the idea of using tags in the commit message, but the rigidity of the spec puts me off. Frankly, I believe we just need a way to identify commits and their intent. So, I would like to propose an approach that builds on the “Conventional Commits” spec.

 

The approach would be

  1. Add an identifier (e.g. Tags: fix) to the commit message footer.
  2. At the start of the release window run “git log”* to print a list of features, bug fixes, performance improvements, deprecations etc.
  3. Either update the main changelog manually or use a script to append individual sections.

 

*git log v2.4...HEAD --no-merges --pretty='- %s (%C(auto)%h)' --grep "Tags: fix">

 

‘git log’ can be easily modified to look for other metadata as long as we agree to add it to the commit message.

 

Advantages

  1. Light(er)
  2. No impact to the subject header
  3. Flexibility to define project specific tags
  4. Training needs at par with “Conventional Commits” proposal

 

Thoughts?

 

-Varun

 

From: TF-A <tf-a-bounces@lists.trustedfirmware.org> On Behalf Of Chris Kay via TF-A
Sent: Thursday, February 25, 2021 9:31 AM
To: tf-a@lists.trustedfirmware.org
Subject: Re: [TF-A] Adoption of Conventional Commits

 

External email: Use caution opening links or attachments

 

Thanks to all those who attended the Tech Forum today!

 

It’s become apparent that the initial 2 week deadline for alternative proposals or implementations is too short, so – as agreed – we’ll push the deadline for the investigation period to the end of March. This period is dedicated to evaluating the changelog automation proposal made, or to identifying alternative solutions. If you have an alternative proposal, any proof-of-concept tooling would be highly appreciated so we can get a clear idea of what sort of work and maintenance is going to be involved.

 

If you do find a solution you wish to propose, please give it just a short name (e.g. “Update it manually”) and make it obvious you want to propose it formally – I’ll collect up the proposals made on the mailing list thread at the end of March and set up a Wiki poll so we can get a clear picture of where the community wants to take this.

 

Chris

 

From: TF-A <tf-a-bounces@lists.trustedfirmware.org> on behalf of Chris Kay via TF-A <tf-a@lists.trustedfirmware.org>
Reply to: Chris Kay <Chris.Kay@arm.com>
Date: Thursday, 11 February 2021 at 13:59
To: "tf-a@lists.trustedfirmware.org" <tf-a@lists.trustedfirmware.org>
Subject: [TF-A] Adoption of Conventional Commits

 

Hi all,

 

Recently we had an internal discussion on the merits of introducing semantics to commit messages pushed to the main TF-A repository, the conclusion being that we would look to adopting the Conventional Commits specification in the near future. There was one major reason for this, which was to help us in automating the changelog in future releases, but it might also help us to dramatically reduce the overall amount of work needed to make a formal release in the future.

 

This requires some buy-in (or buy-out, in this case) from maintainers because - even though it’s to only a relatively minor extent - it does involve an adjustment to everybody’s workflow. Notably, commit messages will be expected to adopt the structure defined by the specification, which will be enforced by the CI. Most commits that go upstream today adhere to “something that looks like Conventional Commits”, so the change is not exactly sweeping, but any change has the potential be an inconvenience.

 

With that in mind, I propose the following:

  • We collectively adopt the specification, enforced only for @arm.com contributors until such a time that the majority of maintainers are familiar with the new demands
  • We suggest - in the prerequisites documentation - the installation of two helper tools:

 

Installation of these tools will be optional, but I believe they can help with the transition. In the patches currently in review, they are installed as Git hooks automatically upon execution of npm install, so it requires no manual installation or configuration (other than a relatively up-to-date Node.js installation).

 

You’ll find the patches here, and specifically the changes to the prerequisites documentation here. Feel free to review these changes if you have comments specifically on their implementation.

 

Let me know if you have any questions or concerns. If everybody’s on board, we can look to have this upstreamed shortly.

 

Chris