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

Move clang-format download into dotnet/runtime and add docs for setting up auto-formatting in the repository #59374

Merged
merged 13 commits into from
Sep 23, 2021

Conversation

jkoritzinsky
Copy link
Member

In the dotnet/runtime repository, we regularly have many "nit" comments on PRs from new contributors. Over the last few years, we've made some improvements in this area, but they've generally provided a poorer experience than possible.

For the RyuJIT code base, we have an extra CI leg that checks formatting for the CoreCLR JIT. This script generates a .diff file that can be git applyd and outputs instructions on how to run the formatter locally. However, to get the diff file, users need to manually trawl through AzDO build artifacts. Running the script also takes a significant amount of time since it clones and builds the https://github.com/dotnet/jitutils repo to get the tool that executes the formatters.

For the majority of our managed code, we use Roslyn-based analyzers and .editorconfig files to enforce our formatting. However, we do not provide any mechanism to fix any formatting issues. The build will just emit build errors for formatting problems.

This PR provides mechanisms within the dotnet/runtime repo to pull down clang-format (for C and C++ formatting) and dotnet-format (for .NET code) as well as documentation for how to enable auto-formatting when committing code or "format-on-save" in editors like Visual Studio Code.

This PR does not yet include formatting rules for the rest of the repository (in particular the CoreCLR native code outside of the JIT or corehost). I wanted to get some feedback on the docs before I start putting together clang-format configuration files based on the existing code styles.

To ensure we do not change the existing style used by RyuJIT developers, we are using the same version of clang-format as the current formatting scripts (3.8). Newer versions have slightly different formatting settings that I can't quite seem to figure out how to map to the equivalent of the 3.8 formatting with the limited time-boxed time I spent on it.

@ghost
Copy link

ghost commented Sep 20, 2021

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

In the dotnet/runtime repository, we regularly have many "nit" comments on PRs from new contributors. Over the last few years, we've made some improvements in this area, but they've generally provided a poorer experience than possible.

For the RyuJIT code base, we have an extra CI leg that checks formatting for the CoreCLR JIT. This script generates a .diff file that can be git applyd and outputs instructions on how to run the formatter locally. However, to get the diff file, users need to manually trawl through AzDO build artifacts. Running the script also takes a significant amount of time since it clones and builds the https://github.com/dotnet/jitutils repo to get the tool that executes the formatters.

For the majority of our managed code, we use Roslyn-based analyzers and .editorconfig files to enforce our formatting. However, we do not provide any mechanism to fix any formatting issues. The build will just emit build errors for formatting problems.

This PR provides mechanisms within the dotnet/runtime repo to pull down clang-format (for C and C++ formatting) and dotnet-format (for .NET code) as well as documentation for how to enable auto-formatting when committing code or "format-on-save" in editors like Visual Studio Code.

This PR does not yet include formatting rules for the rest of the repository (in particular the CoreCLR native code outside of the JIT or corehost). I wanted to get some feedback on the docs before I start putting together clang-format configuration files based on the existing code styles.

To ensure we do not change the existing style used by RyuJIT developers, we are using the same version of clang-format as the current formatting scripts (3.8). Newer versions have slightly different formatting settings that I can't quite seem to figure out how to map to the equivalent of the 3.8 formatting with the limited time-boxed time I spent on it.

Author: jkoritzinsky
Assignees: -
Labels:

area-Infrastructure

Milestone: -

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting up this general framework seems fine to me.

The JIT team should definitely consider updating to a brand new clang-format/clang-tidy, and updating the 5+ year old cached copies we have. Perhaps that would solve some of the problems you were seeing integrating with VS Code?

Note that the JIT team uses the jit-format tool to invoke clang-format/clang-tidy. This is super important since it parallelizes the run, making it much, much faster.

Do you expect to change jitutils bootstrap.cmd/sh to not download the clang tools, and instead use the ones you've provided scripts here for downloading?

If the non-JIT C/C++ code teams decide to use clang-*, that is a big and contentious decision. Just sayin...

@jkoritzinsky
Copy link
Member Author

Setting up this general framework seems fine to me.

The JIT team should definitely consider updating to a brand new clang-format/clang-tidy, and updating the 5+ year old cached copies we have. Perhaps that would solve some of the problems you were seeing integrating with VS Code?

Updating clang-format/clang-tidy would solve some of the issues, and would make it easier to apply formatting rules to various parts of the repo that have semi-consistent styling already.

Note that the JIT team uses the jit-format tool to invoke clang-format/clang-tidy. This is super important since it parallelizes the run, making it much, much faster.

Is jit-format actually faster? I've been testing out using the "format-on-save" and commit hooks here. At least for clang-format, the performance is better than whenever I've tried to use jit-format and much better than when I've used the format.py script in this repo. I have plans to follow this up with docs for using clang-tidy (including plugins for all of the different IDEs) to help move that into a repo-wide framework as well and effectively obsolete jit-format.

Do you expect to change jitutils bootstrap.cmd/sh to not download the clang tools, and instead use the ones you've provided scripts here for downloading?

I guess since I've already updated format.py, I could update those scripts as well after this is merged in.

If the non-JIT C/C++ code teams decide to use clang-*, that is a big and contentious decision. Just sayin...

I'm gearing up for that conversation as we speak. That's why I added the "disable formatting" file at the root of the repo, to make this more opt-in on a team by team basis if I can convince people to describe their existing conventions in clang-format (or let me do it for them).

@BruceForstall
Copy link
Member

Is jit-format actually faster?

Maybe format-on-save is only touching changed files, whereas jit-format is looking at all JIT files, touched or not?

I'm not sure we can depend on editor format-on-save, given how people use so many different editing workflows.

@jkoritzinsky
Copy link
Member Author

Yes, format-on-save and the pre-commit hook both only look at changed files.

A script that touches all files (like jit-format) for CI validation of formatting makes sense. All other cases should be covered by the pre-commit hook or the format-on-save functionality.

@jkoritzinsky
Copy link
Member Author

Failures are #59542 and #59541

@jkoritzinsky
Copy link
Member Author

Now that we have the default "disable formatting" file and the filled out docs, I'm going to merge this in.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants