Add arm-toolchain CONTRIBUTING.md including downstream patch policy #48

smithp35 · 2025-01-22T16:44:44Z

This is based on the "LLVM Embedded Toolchain for Arm" contributing.md with adaptations for the arm-toolchain.

The largest change is the introduction of the downstream patch policy. Arm wants the arm-toolchain to be as close to the upstream open-source projects as possible, we impose some additional requirements on downstream patches so that we can summarize all the patches and know why they were made.

Replace llvm-project's CONTRIBUTING.md rather than using another location like .github as we expect many developers to read the file from the terminal.

This is based on the "LLVM Embedded Toolchain for Arm" contributing.md with adaptations for the arm-toolchain. The largest change is the introduction of the downstream patch policy. Arm wants the arm-toolchain to be as close to the upstream open-source projects as possible, we impose some additional requirements on downstream patches so that we can summarize all the patches and know why they were made. Replace llvm-project's CONTRIBUTING.md rather than using another location like .github as we expect many developers to read the file from the terminal.

vhscampos · 2025-01-22T17:22:18Z

CONTRIBUTING.md

+The Arm Toolchain repository is synchronized with the upstream
+*llvm-project* including the main and release branches. Arm Toolchain
+for Embedded aims to stay close to the head of the *picolibc* main
+branch. The newlib overlay package aims to track the most recent


I suggest we stick to a consistent formatting of project names. In some places they are emphasized with stars, in others they aren't. For instance in this line, newlib does not have emphasis.

ACK. I thought I'd caught those. Will fix.

vhscampos · 2025-01-22T17:22:47Z

CONTRIBUTING.md

+before reporting. If the issue exists in *llvm-project* or *picolibc*
+then please report the issue in the upstream project.
+
+Please create a Github issue in the *arm-toolchain* project


Github -> GitHub

vhscampos · 2025-01-22T17:23:18Z

CONTRIBUTING.md

+
+Please create a Github issue in the *arm-toolchain* project
+[Issues](https://github.com/arm/arm-toolchain/issues)
+list and label is as a `bug`.


We ought to check that external contributors have permissions to add labels. That is not the case for example in the ABI and the ACLE repositories.

Looks like our base role is Read and Triage is needed to add labels. I've taken out the requirement to add labels, I'm not sure that was happening in practice anyway.

I've suggested putting [RFC] in the title. I expect one of us with sufficient privilege will need to add the labels.

vhscampos · 2025-01-22T17:23:44Z

CONTRIBUTING.md

+For a bigger change, please create an issue in the
+*arm-toolchain* project
+[Issues](https://github.com/arm/arm-toolchain/issues)
+list and label is as an `rfc` (Request for Comments) to initiate the discussion


Same remark about permissions to add labels

ACK. I've suggested [RFC] in the subject instead.

vhscampos · 2025-01-22T17:24:29Z

CONTRIBUTING.md

+#### Toolchain integration
+
+Patches that are specific to integrating components of the Arm
+toolchain or the the projects that it depends on, may be implemented


vhscampos · 2025-01-22T17:27:35Z

CONTRIBUTING.md

+* New files added to an external project like *picolibc* or *newlib*
+  must follow the licensing and copyright of the external project.
+
+* Any patch that causes the arm toolchain to deviate from upstream


Capitalise arm toolchain

vhscampos · 2025-01-22T17:27:49Z

CONTRIBUTING.md

+
+* Any patch that causes the arm toolchain to deviate from upstream
+  behavior, such as a new command-line option, must be documented in
+  the arm-software/doc/features.md file. The documentation for a


Put the filepath between ticks

* Same emphasis used for open-source projects. * Removed requirement to add labels on issues. * Fix typos and capitalisation.

smithp35

Thanks for the review. I'll upload a fixup patch.

smithp35 · 2025-01-22T17:52:28Z

CONTRIBUTING.md

+The Arm Toolchain repository is synchronized with the upstream
+*llvm-project* including the main and release branches. Arm Toolchain
+for Embedded aims to stay close to the head of the *picolibc* main
+branch. The newlib overlay package aims to track the most recent


ACK. I thought I'd caught those. Will fix.

smithp35 · 2025-01-22T17:52:36Z

CONTRIBUTING.md

+before reporting. If the issue exists in *llvm-project* or *picolibc*
+then please report the issue in the upstream project.
+
+Please create a Github issue in the *arm-toolchain* project


smithp35 · 2025-01-22T17:54:04Z

CONTRIBUTING.md

+
+Please create a Github issue in the *arm-toolchain* project
+[Issues](https://github.com/arm/arm-toolchain/issues)
+list and label is as a `bug`.


Looks like our base role is Read and Triage is needed to add labels. I've taken out the requirement to add labels, I'm not sure that was happening in practice anyway.

I've suggested putting [RFC] in the title. I expect one of us with sufficient privilege will need to add the labels.

smithp35 · 2025-01-22T17:54:31Z

CONTRIBUTING.md

+For a bigger change, please create an issue in the
+*arm-toolchain* project
+[Issues](https://github.com/arm/arm-toolchain/issues)
+list and label is as an `rfc` (Request for Comments) to initiate the discussion


ACK. I've suggested [RFC] in the subject instead.

smithp35 · 2025-01-22T17:54:40Z

CONTRIBUTING.md

+#### Toolchain integration
+
+Patches that are specific to integrating components of the Arm
+toolchain or the the projects that it depends on, may be implemented


smithp35 · 2025-01-22T17:54:48Z

CONTRIBUTING.md

+* New files added to an external project like *picolibc* or *newlib*
+  must follow the licensing and copyright of the external project.
+
+* Any patch that causes the arm toolchain to deviate from upstream


smithp35 · 2025-01-22T17:54:54Z

CONTRIBUTING.md

+
+* Any patch that causes the arm toolchain to deviate from upstream
+  behavior, such as a new command-line option, must be documented in
+  the arm-software/doc/features.md file. The documentation for a


voltur01 · 2025-01-23T10:12:38Z

CONTRIBUTING.md

-To get started with contributing, please take a look at the
-[Contributing to LLVM](https://llvm.org/docs/Contributing.html) guide. It
-describes how to get involved, raise issues and submit patches.
+Arm Toolchain for Embedded integrates *llvm-project* with the


Do we need to mention llvm-libc separately as well? It is part of llvm-project so implicitly is there, however we seem to go into much more details in regard to the other two libraries.

Taking into account that we want to use llvm-libc more, we may want to mention is explicitly.

The distinction I was trying to make was in-tree llvm-project, which includes llvm-libc, and out-of-tree picolibc and newlib. LLVM libc

I've updated the wording to hopefully make it clearer.

voltur01 · 2025-01-23T10:16:30Z

CONTRIBUTING.md

+list and put [RFC] in the subject line (Request for Comments) to initiate the discussion
+first, before submitting a PR for the change.
+
+There is no formal template for an `rfc`, however it would be good to


rfc was probably a label previously, so may be clearer as RFC now with the new recommendation.

voltur01 · 2025-01-23T10:18:58Z

CONTRIBUTING.md

+
+A patch is considered a downstream patch if:
+
+* It touches any part of the *llvm-project* repository outside the


Should this be arm-toolchain repository? Or maybe more complicated statement like llvm-project files inside arm-toolchin repo?

I've used the arm-toolchain repository and tweaked the language a bit.

voltur01 · 2025-01-23T10:22:27Z

CONTRIBUTING.md

+  as when the upstream equivalent lands, must include Removes
+  downstream issue:#<issue number>.
+
+* New files in *llvm-project* must include the LLVM license, with the


Same as above: llvm-prooject files inside arm-toolchain repo?

voltur01 · 2025-01-23T10:24:50Z

CONTRIBUTING.md

+
+* Any patch that causes the Arm Toolchain to deviate from upstream
+  behavior, such as a new command-line option, must be documented in
+  the `arm-software/doc/features.md file`. The documentation for a


Should we add the placeholder file now as well?
Maybe it should have sections for common and embedded/linux specific features?

Yes, I was in two minds over whether to do that here or in a follow up patch.

There's an interesting design decision here. The current directory structure is:

arm-software |-- ci |-- embedded |-----docs |-- linux

With the docs directory for embedded being part of the installed package.

I think we would want the documentation of downstream features to be available in the installed package for LLVM embedded at least. Not sure about what is supposed to happen in linux.

One way of doing this is to put the downstream feature documentation in embedded/docs/features.md and linux/docs/features.md with duplication for common changes. In theory all changes are common to both, but I suspect that the users for each toolchain will care about different downstream features. For example ATfA won't care about M-profile code-generation changes.

An alternative is to add a new doc directory to arm-software for common docs for both embedded and linux. The tool installation could then copy the features.md file into the installation.

My instinct is that ATfE will have more of the downstream features, and that these won't be that useful to ATfA, so I think each toolchain can have its own features.md file.

I'll start with a patch for embedded/docs/features.md

Yes, this is a tricky one. On one hand, most of the LLVM changes will technically impact both, so would be easier to document in one place, but then you are right it is not obvious how to nicely include into the distribution.

I think I prefer to have a shared folder to document the changes in one place, then an extra CMake rule to copy contents into the distribution.

We may have 3 feature files: common to copy to both, embedded and linux for very clearly specific things to be copied to respective distributions.

* change rfc to RFC. * prefer arm-toolchain repository to llvm-project when talking about scope of changes. * Add docs subdirectory for documentation common to both ATfE and ATfA. * Created skeleton docs for downstream feature user documentation.

smithp35 · 2025-01-23T17:36:28Z

I've uploaded a new patch with the common docs folder and some skeleton feature pages.

I've also removed the embedded/contributing.md as this replaces it.

vhscampos reviewed Jan 22, 2025

View reviewed changes

Address Victor's review comments

009c7d9

* Same emphasis used for open-source projects. * Removed requirement to add labels on issues. * Fix typos and capitalisation.

smithp35 commented Jan 22, 2025

View reviewed changes

vhscampos approved these changes Jan 23, 2025

View reviewed changes

voltur01 reviewed Jan 23, 2025

View reviewed changes

Review comments from Volodymyr

6223bf9

* change rfc to RFC. * prefer arm-toolchain repository to llvm-project when talking about scope of changes. * Add docs subdirectory for documentation common to both ATfE and ATfA. * Created skeleton docs for downstream feature user documentation.


		A patch is considered a downstream patch if:

		* It touches any part of the llvm-project repository outside the

Add arm-toolchain CONTRIBUTING.md including downstream patch policy #48

Are you sure you want to change the base?

Add arm-toolchain CONTRIBUTING.md including downstream patch policy #48

Conversation

smithp35 commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smithp35 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smithp35 commented Jan 23, 2025