GitHub: Investigate GitHub Actions for clang-format and validate

[StephanTLavavej]

We currently use Azure Pipelines to check whether PRs follow our clang-format and other conventions:

STL/azure-devops/run_build.yml

Lines 52 to 67 in fffbd8f

	- task: BatchScript@1
	displayName: 'Enforce clang-format'
	timeoutInMinutes: 60
	condition: eq('${{ parameters.targetPlatform }}', 'x64')
	inputs:
	filename: 'azure-devops/enforce-clang-format.cmd'
	failOnStandardError: true
	arguments: '$(Build.ArtifactStagingDirectory)/tools/parallelize/parallelize.exe'
	- task: BatchScript@1
	displayName: 'Validate Files'
	timeoutInMinutes: 2
	condition: eq('${{ parameters.targetPlatform }}', 'x64')
	inputs:
	filename: 'azure-devops/validate-files.cmd'
	failOnStandardError: true
	arguments: '$(Build.ArtifactStagingDirectory)/tools/validate/validate.exe'

STL/azure-devops/enforce-clang-format.cmd

Lines 3 to 12 in fffbd8f

	"%1" "clang-format.exe -style=file -i" ^
	stl/inc ^
	stl/src ^
	tests ^
	tools
	@echo If your build fails here, you need to format the following files with:
	@clang-format.exe --version
	@git status --porcelain stl tests tools 1>&2
	@echo clang-format will produce the following diff:
	@git diff stl tests tools 1>&2

STL/tools/validate/validate.cpp

Lines 140 to 146 in fffbd8f

	if (has_utf8_bom) {
	fwprintf(stderr, L"Validation failed: %s contains UTF-8 BOM characters.\n", filepath.c_str());
	}
	if (tab_policy == TabPolicy::Forbidden && tab_characters != 0) {
	fwprintf(stderr, L"Validation failed: %s contains %zu tab characters.\n", filepath.c_str(), tab_characters);
	}

This is extremely useful, but suboptimal. The user experience is somewhat non-intuitive for new contributors, as we perform these checks within the x64 build. (Edit: #682 improved this, running Code Format Validation separately.) While their error messages are informative, they're hidden behind a Details link. Additionally, our clang-format enforcement complains about problems, but doesn't fix them - the user has to install and run clang-format themselves, or manually edit their files until clang-format is happy.

It appears that GitHub Actions could provide a better experience. They can post "check notices" on specific lines of PRs, and they can push changes to branches. They can also be triggered by comments. So, we could automatically push clang-format changes, and post check notices for validate.cpp failures (as those are less amenable to automatic fixing).

Originally suggested by Charlie Barto (@barcharcraz): #509 (comment)

[StephanTLavavej]

Curtis J Bezault (@cbezault) notes that our Azure Pipelines infrastructure could be changed to simply push a commit when clang-format fails. One contributor has requested this; I believe we should run a poll/survey to judge whether this would be generally desirable for contributors.

[StephanTLavavej]

After thinking about this, I'm opposed to automatically pushing formatting changes. That could have ugly results, which should be noticed before creating a PR instead of during review. Additionally, I believe it would encourage behavior that we should not encourage - one's editor should be properly configured to format-on-save. (clang-format/validate failures are generally an indication that one's development process is incomplete.)

After #2671 and #2697, we now have a convenient way to format all files (I use this when updating clang-format versions, but it could also be used before pushing changes, if one is using an editor that can't format-on-save). There are potential improvements to how we display clang-format and validation failures (clang-format failures are displayed in the Azure Pipelines results with a downloadable diff, although the discoverability of these messages could probably be improved; #2378 is exploring how to report more detailed failures from validation). Automatically creating comments for failures would probably be too noisy, even if we could do so.

We might be able to close this tracking issue now.

[StephanTLavavej]

We talked about this at the weekly maintainer meeting and concluded:

• Automatically pushing formatting changes would be undesirable.
• Having GitHub Actions run clang-format (and validate) and post a comment highlighting any issues found, similar to what vcpkg does (nicole mazzuca (@strega-nil-ms) provided an example: [ctk] Add new port vcpkg#25490 (review) ), would make formatting failures more discoverable, but would avoid the downsides of automatically pushing changes
• We would need to ensure that if GitHub Actions runs clang-format, that it uses exactly the same version that's shipping in VS, to avoid formatting divergence. (We wouldn't want "this version or later" logic.)
  • There may be potential platform differences, which would be concerning; hopefully there are no significant differences between Linux and Windows clang-format.
• validate should be portable code and we should be able to make it run on Ubuntu with minimal effort

[StephanTLavavej]

I proposed closing this as "won't do". We've got Azure Pipelines via 1ES Hosted Pools working nicely, so maintaining parallel infra would be more work, and with increasing scrutiny of infrastructure security, adding GitHub Actions to the mix is moving in the wrong direction. Auto-commenting on formatting issues doesn't seem valuable; people should be encouraged to configure format-on-save, whereas auto-commenting seems likely to increase reliance on PR checks - also it would involve granting more repo permissions to scripts and we've been encouraged to lock that down as much as possible.

We talked about this again at the weekly maintainer meeting and agreed to wontfix this.