Move dotnet-format script to pre-commit

[raulsntos]

• Follow-up to Replace hand-written pre-commit hooks with pre-commit Python tool #88866.
• Can't be cherry-picked to 3.x, dotnet-format is a .NET 6+ tool.

[paulloz]

All good 👍

I think that at some point in the future, we might want to do something clever to avoid running dotnet format on all the projects all the time.

[akien-mga]

It seems like running dotnet format via pre-commit should be supported officially via https://github.com/dotnet/format (see https://github.com/dotnet/format/blob/main/.pre-commit-hooks.yaml, and it's referenced on https://pre-commit.com/hooks.html).

This might be better than a local hook, as it might include a seamless process for installing a local copy of dotnet to run dotnet format, even for contributors who don't have a .NET setup usually (e.g. if people fix a typo in a .cs file, the current hook would just fail if they don't have the command in PATH).

Edit: Tested the current PR, if you don't have dotnet in PATH, it succeeds but does nothing:

dotnet-format............................................................Passed

With the current python script approach, we'd need to return a non-zero error code when the call fails.

[akien-mga]

I've tried using the https://github.com/dotnet/format hook but despite cloning the whole repo in ~/.cache/pre-commit/repo6r0si42t/, it doesn't actually include a usable dotnet binary so it would still fail the same way.

For the reference, here's how I included it:

  - repo: https://github.com/dotnet/format
    # https://github.com/dotnet/dotnet/blob/v8.0.2/Components.md
    rev: 28925c0

Another issue I had was that I couldn't find how to make pre-commit trigger the hook on c# files, but run the hook on csproj files, without using the globbing you added in your script. That's maybe a feature we could get added to pre-commit in the future, so the pattern for which files trigger a hook and which files are passed to the hook entry could differ, without having to make a custom script for it.

---

So I guess we can stick to the approach in this PR. We could either make it return non-zero when dotnet is missing, or keep it as is so it "succeeds" without bothering contributors who don't have dotnet (and CI will fail if they happened to change C# files, in which case our docs should clarify that installing dotnet is optional but recommended for contributors).

[raulsntos]

It seems like running dotnet format via pre-commit should be supported officially

Yep, I saw that and originally I tried to set it up like the example in their repo. But I run into multiple issues:

• We'd want to always use the latest version of dotnet format but the hook requires to specify a rev.
• We have to run dotnet format on the csproj files. Running only on the modified C# files didn't work properly for me.
  • @paulloz suggested to be smarter about the csproj files we run the hook on. Ideally we'd only run it on the csproj that contains the C# files that were modified, but that seems complicated to implement and I don't think it would speed things up that much.
• The C# projects fail to build without the generated props files, so running some pre-pre-commit phase to generate dummy files is needed.
  • We can exclude these generated files from the csproj with an environment variable (as suggested in Move dotnet-format script to pre-commit #88933 (comment)).

We could either make it return non-zero when dotnet is missing, or keep it as is so it "succeeds" without bothering contributors who don't have dotnet (and CI will fail if they happened to change C# files, in which case our docs should clarify that installing dotnet is optional but recommended for contributors).

Even though the hook only runs when you modify C# files, sometimes non-C# contributors modify C# files just for parity with changes they are making in other parts of the engine. I wouldn't want to make it a requirement to have dotnet installed for those cases.

To me, running dotnet format in CI is the most important part, so I think it's fine if users don't run the hook locally. That said, if you are a C# contributor, I'd expect you already have the .NET SDK installed so the hook should work.

[paulloz]

@paulloz suggested to be smarter about the csproj files we run the hook on. Ideally we'd only run it on the csproj that contains the C# files that were modified, but that seems complicated to implement and I don't think it would speed things up that much.

It would speed things up drastically if people are aiming to actually use the hook as a pre-commit. For CI, I agree that it doesn't matter. As for being complicated, that kind of thing should work?

# Match all the input files to their respective C# project.
input_files = [ os.path.normpath(x) for x in sys.argv ]
projects = { path: [
    f for f in sys.argv if os.path.commonpath([f, path]) == path
] for path in [ os.path.dirname(f) for f in glob.glob("**/*.csproj", recursive=True) ] }
# Run dotnet format on all projects with more than 0 modified files.
for (path, files) in projects.items():
    if len(files) > 0:
        command = f"dotnet format {path} --include {' '.join(files)}"
        os.system(command)

[raulsntos]

Thanks, I gave it a try and it's actually faster than I expected.

All csproj	Only modified csproj
1:50.98	10.771

So I added your changes to the PR.

[akien-mga]

Thanks!