[ScottTodd]

We're getting close to 0 patches across the ROCm projects but we've still had several disruptive patches land recently.

I want to keep a closer eye on changes to the patches subdirectory and put some guidelines in place for how the team should be thinking about them.

Some highlights from the doc:

• Documenting how fetch_sources.py applies existing patches
• Documenting how save_patches.sh saves new patches
• Documenting how rocm-libraries and rocm-systems consume patches, resolve/remove patches, and can have their development blocked by patches not applying
• A "Rules for creating patches" section that emphasizes that problematic commits should be reverted directly upstream and not patched - open source CI/CD takes priority over closed source CI/CD in open source code

[geomin12]

great guide! some nits

[geomin12]

is that m/ supposed to be there?

[ScottTodd]

Copied from the example in the script itself:

TheRock/build_tools/save_patches.sh

Lines 4 to 7 in 0887429

	# Usage: save_patches.sh base_tag project_name patch_subdir
	#
	# Example:
	# save_patches.sh m/amd-mainline hipBLASLt amd-mainline

We've had a long standing TODO there to pull the base commit from git submodule status or equivalent so that argument isn't needed. I often just manually look up what commit I want to stack patches on top of.

[jayhawk-commits]

I will take a look tomorrow as it is already 8pm here.

[amd-shiraz]

shouldn't we be adding more eyes to this than just yourself. at least start with shadowing reviews and eventually ramp up?

[ScottTodd]

Volunteers are welcome :)

[ScottTodd]

To add some color here:

• I want to be careful rolling out CODEOWNERS so we don't diffuse responsibility too much. The current teams set by https://github.com/orgs/ROCm/teams are not fit for use in CODEOWNERS as a review requirement, but that is how https://github.com/ROCm/rocm-libraries/blob/develop/.github/CODEOWNERS is using them. For example:

  # CI ownership
  /.github/                                           @ROCm/rocm-libraries-reviewers

  the "rocm-libraries reviewers" team currently has 28 subteams (e.g. "rocblas reviewers") for a total of ~104 team members.

• Anyone is encouraged to watch PR discussions and contribute reviews. That won't change with the addition of CODEOWNERS.

• I like the CODEOWNERS and subscription settings in LLVM:

  • https://github.com/llvm/llvm-project/blob/main/.github/CODEOWNERS has individuals
  • https://github.com/orgs/llvm/teams uses teams to subscribe to issues via @ tagging and a bot:

[amd-shiraz]

@ScottTodd overall lgtm intention wise, i would love to understand later few things from the current workflow of system/libraries patches, who owns fixing conflicts if they don't own the patch etc. i will sync with you and others later on that. otherwise just some nit.

[marbre]

LGTM, just minor nits + volunteering me as an additional codeowner.