Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add format target to cmake #2671

Merged
merged 35 commits into from
May 1, 2022

Conversation

strega-nil-ms
Copy link
Contributor

@strega-nil-ms strega-nil-ms commented Apr 26, 2022

This replaces the parallelize.cpp hack, as well as allows users to run format with CMake.

Additionally, this publishes the diff, so that people without the specific version of clang-format on their machine can download the diff and apply it themselves.

@strega-nil-ms strega-nil-ms requested a review from a team as a code owner April 26, 2022 17:02
@StephanTLavavej StephanTLavavej added the infrastructure Related to repository automation label Apr 26, 2022
azure-pipelines.yml Outdated Show resolved Hide resolved
azure-pipelines.yml Show resolved Hide resolved
azure-devops/Create-PRDiff.ps1 Outdated Show resolved Hide resolved
azure-devops/Create-PRDiff.ps1 Outdated Show resolved Hide resolved
azure-devops/Create-PRDiff.ps1 Outdated Show resolved Hide resolved
format/CMakeLists.txt Outdated Show resolved Hide resolved
azure-devops/Create-PRDiff.ps1 Outdated Show resolved Hide resolved
azure-pipelines.yml Outdated Show resolved Hide resolved
azure-pipelines.yml Outdated Show resolved Hide resolved
azure-pipelines.yml Outdated Show resolved Hide resolved
azure-pipelines.yml Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter self-requested a review April 27, 2022 00:57
format.diff Outdated Show resolved Hide resolved
also make sure that we don't add diff files in the future

(this is a thing Nicole has done more times than she'd like to admit)
azure-devops/create-prdiff.ps1 Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
@ghost

This comment was marked as outdated.

@StephanTLavavej
Copy link
Member

Looks great! I've verified the failure experiences in #2681 and #2682, and locally verified that CMake's printed messages are expected. The clang-format experience is a little different from the current one (have to click through to see the diff) but I'm ok with that behavioral change.

I went ahead and pushed a message clarification to create-prdiff.ps1 to help contributors seeing this output for the first time, and some changes to validate.cpp - minor style issues plus a rework of how any_errors are recorded to reduce verbosity and ensure that Code Format Validation will always fail if we print a failure message (this restores detection of long paths, paths with spaces, and the new bad extensions). I believe that this preserves your design intent but please meow if you disagree!

@azure-pipelines

This comment was marked as resolved.

@CaseyCarter
Copy link
Contributor

The clang-format experience is a little different from the current one (have to click through to see the diff) but I'm ok with that behavioral change.

I'm mildly concerned that this will mean we spend a lot more time explaining to new users how to find the formatting diff, but we can wait to address it if and when it actually becomes a problem.

@StephanTLavavej
Copy link
Member

Closing and reopening to fix the CLA check.

@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit d849a95 into microsoft:main May 1, 2022
@StephanTLavavej
Copy link
Member

Thanks for improving this important part of our infrastructure (parallelize was a source of headaches in the past, e.g. #1619), and congratulations on your first microsoft/STL commit!

🎉 😻 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Related to repository automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants