Page MenuHomePhabricator

tests: allow trunk versions of clang-format to be used
Needs ReviewPublic

Authored by spectral on Apr 30 2021, 7:20 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

spectral created this revision.Apr 30 2021, 7:20 PM

I'm not sure that this is an improvement. We discussed whether we want to hard-wire the version even stricter ("exactly version 11"), especially because it is hard to predict when the output is going to change again.

I'm not sure that this is an improvement. We discussed whether we want to hard-wire the version even stricter ("exactly version 11"), especially because it is hard to predict when the output is going to change again.

I think that there can be two different policies, depending on the machine involved. Have CI and maintainers use the "officially supported" versions (11.x), while others are able to use a looser set (>=11.0, trunk [with the assumption it's kept relatively up to date]).

I think that the worst that can happen with this change is:
a. clang-format makes a backwards-incompatible change
b. someone picks up the newer version (via trunk instead of an officially versioned one), or someone has an old version of trunk that's not "forwards compatible"
c. tries to help and sends a change that reformats stuff (that they didn't modify) to conform to the new version, breaking it for those that use the old version

If that happens, either the minimum version should be incremented then (if there's an officially versioned number released), or the change should be rejected. I expect this trio of events to be rather uncommon.

On the flip side, by requiring an official version number, you make it so that every change I make can't be formatted, and it's up to the people accepting the change to either kick it back to me to fix, fix it in flight, or have the tests break and have me fix it afterward. I can't easily install a different clang-format - the version on my machine is managed by another team; maybe there's a way I could customize what clang-format to use just for Mercurial's tests, but that probably means that running the tests just becomes a bit harder (for me and everyone in my situation) and while it's rather small, it's still a non-zero extra hurdle to remember to go through. Even if I run the tests before sending (and I'll be the first to admit that I'm not good at remembering to do so), if I don't remember to run them in the special way that makes this one test work, I'll just have it skipped (and it'll be in the middle of a giant block of other tests that get skipped).

I wonder if there should be a configure-once mechanism for "this set of tests are tests that if they get skipped, I want that to be treated as a failure, not a skip". My test-check-format.t broke due to other changes (I believe because my distro upgraded to py3.9, breaking my pip installation of black), and I didn't notice. :( I still think this proposed change is an improvement.

durin42 accepted this revision as: durin42.May 27 2021, 5:17 PM
durin42 added a subscriber: durin42.

I think that the worst that can happen with this change is:
a. clang-format makes a backwards-incompatible change

This has happened (in the past, anyway) pretty regularly. When I asked djasper about it, his answer was "you should check in the clang-format binary you want to test against" which we obviously can't do. :/

That said, I'm +0 on this and I think we should try it, pending any lingering objection from @joerg.sonnenberger

Consider it more a -0, not a very strong objection.

The CI running on Heptapod is available to any developer who pushs draft to it, and it uses the "correct" black version, pinned in the docker image what we uses for the CI.

Something we have been entertaining for a while is to a manually trigger a CI "job" that would run the black formatter one current stack and push back the result to heptapod. This would give a simple option to format with the right version when the CI complains about it. It looks like it could fit the @spectral usecase here.

(The docker image can be easily updated to newer version when appropriate by anyone creating a Merge request here: https://foss.heptapod.net/octobus/ci-dockerfiles/)

I think that the worst that can happen with this change is:
a. clang-format makes a backwards-incompatible change

This has happened (in the past, anyway) pretty regularly. When I asked djasper about it, his answer was "you should check in the clang-format binary you want to test against" which we obviously can't do. :/

One thing I noticed: currently the check is for >= major version 11. So presumably we *already* have these version skew issues, we're just not detecting 'trunk' as being likely-recent.

The CI running on Heptapod is available to any developer who pushs draft to it, and it uses the "correct" black version, pinned in the docker image what we uses for the CI.
Something we have been entertaining for a while is to a manually trigger a CI "job" that would run the black formatter one current stack and push back the result to heptapod. This would give a simple option to format with the right version when the CI complains about it. It looks like it could fit the @spectral usecase here.
(The docker image can be easily updated to newer version when appropriate by anyone creating a Merge request here: https://foss.heptapod.net/octobus/ci-dockerfiles/)

While this is/would be very useful, it requires manual action, I think? My goal is to prevent that. I want/need there to be a single command I can run that ensures a commit is ready to land: correctly formatted, tests pass, etc.; there's a reason these check-format tests exist as tests. :/

Ideally I'd be able to list features (like black,clang-format) or tests (test-check-format.t,test-check-clang-format.t) that if they're skipped due to missing features they're treated as errors. My copy of black seems to have broken itself somehow, and I only noticed because I explicitly went looking for these format checking tests being skipped, knowing that it's been a problem in the past.

The CI running on Heptapod is available to any developer who pushs draft to it, and it uses the "correct" black version, pinned in the docker image what we uses for the CI.
Something we have been entertaining for a while is to a manually trigger a CI "job" that would run the black formatter one current stack and push back the result to heptapod. This would give a simple option to format with the right version when the CI complains about it. It looks like it could fit the @spectral usecase here.
(The docker image can be easily updated to newer version when appropriate by anyone creating a Merge request here: https://foss.heptapod.net/octobus/ci-dockerfiles/)

While this is/would be very useful, it requires manual action, I think?

Yes, it would be an optional way for people who don't want (or can't easily) install the required tools at the required version.

My goal is to prevent that. I want/need there to be a single command I can run that ensures a commit is ready to land: correctly formatted, tests pass, etc.; there's a reason these check-format tests exist as tests. :/

I am not challenging that, but whatever we do with these tests need to be viable at the project level and it don't seems like using too many, incompatible, version of the formatter would fly well.

I think the baseline for the project should be "the CI shoul stay green" which implies "new changesets should keep the CI green". Running tests locally is a good way to quickly catch errors, but eventually, the only way to validate this is to run the CI on submitted changeset. Something that is now easy to do for a couple of years.
However this implies to make sure the version of formatter used by developers is aligned with the one used by the CI. Which should not be to complicated.

Is the above clear ?

Ideally I'd be able to list features (like black,clang-format) or tests (test-check-format.t,test-check-clang-format.t) that if they're skipped due to missing features they're treated as errors. My copy of black seems to have broken itself somehow, and I only noticed because I explicitly went looking for these format checking tests being skipped, knowing that it's been a problem in the past.

Yeah, this would be useful for the CI too. What about getting a --required LIST/PATTERN/FILE flag for run-tests.py

The CI running on Heptapod is available to any developer who pushs draft to it, and it uses the "correct" black version, pinned in the docker image what we uses for the CI.
Something we have been entertaining for a while is to a manually trigger a CI "job" that would run the black formatter one current stack and push back the result to heptapod. This would give a simple option to format with the right version when the CI complains about it. It looks like it could fit the @spectral usecase here.
(The docker image can be easily updated to newer version when appropriate by anyone creating a Merge request here: https://foss.heptapod.net/octobus/ci-dockerfiles/)

While this is/would be very useful, it requires manual action, I think?

Yes, it would be an optional way for people who don't want (or can't easily) install the required tools at the required version.

My goal is to prevent that. I want/need there to be a single command I can run that ensures a commit is ready to land: correctly formatted, tests pass, etc.; there's a reason these check-format tests exist as tests. :/

I am not challenging that, but whatever we do with these tests need to be viable at the project level and it don't seems like using too many, incompatible, version of the formatter would fly well.
I think the baseline for the project should be "the CI shoul stay green" which implies "new changesets should keep the CI green". Running tests locally is a good way to quickly catch errors, but eventually, the only way to validate this is to run the CI on submitted changeset. Something that is now easy to do for a couple of years.
However this implies to make sure the version of formatter used by developers is aligned with the one used by the CI. Which should not be to complicated.

I'm still struggling to identify where my proposed fix makes any of this worse.

Current state: people with a version of clang-format <11, or people with a 'trunk' version of clang-format, don't get their code run through clang-format at all, and send changes that will break CI when they land (unless the issue is caught during review).

Proposed state: that no longer happens for people running a 'trunk' version of clang-format, just those using clang-format <11.

Potential differences between those two states (all of these assume that the user authoring the changes are using a 'trunk' version that produces formatting incompatible with the "released version that is >=11" check we currently have, either because their 'trunk' version is too old OR too new; I'm assuming that having a 'trunk' version installed is already rather rare for non-Googlers):

  • [extremely unlikely] the user runs check-code-format, it produces a warning for *only* their changes, they format it with that style of formatting, and send the change. This is *indistinguishable* for reviewers and automation from the user just formatting it like this themselves, not having clang-format installed at all. This is extremely unlikely because any sort of formatting breakages are almost certainly going to affect more than just the changes the user is making.
  • [very unlikely] the user runs check-code-format, it produces a warning for their changes and a bunch of other unrelated files, they *don't* ask "what's up with this" and instead send a change with *just* a series of fixes to make the entire codebase compatible with their version of clang-format (and incompatible with the version(s) we officially support. That was really nice of them to do that, but it's easy to reject the change if this happens; it's a shame they spent time on it just to have it rejected, but I'm assuming the likelihood of this is very low, as I stated -- either they'll ask "why is this test broken in files I didn't modify", and we reply "your clang-format is an unsupported version", or they'll just ignore the output of that test, assuming it's "someone else's problem". If they then send a change that is formatted using the incompatible clang-format, we're back where we started: this is indistinguishable from the user not having clang-format installed at all.
  • [likely] I'll break the CI with bad formatting less often :)

I'm explicitly *not* suggesting that we consider formatting produced by any arbitrary 'trunk' version of clang-format to be "supported". We should still reject changes that would break the CI using whatever version it has, and we wouldn't install a 'trunk' version of clang-format on the CI.

Is the above clear ?

Ideally I'd be able to list features (like black,clang-format) or tests (test-check-format.t,test-check-clang-format.t) that if they're skipped due to missing features they're treated as errors. My copy of black seems to have broken itself somehow, and I only noticed because I explicitly went looking for these format checking tests being skipped, knowing that it's been a problem in the past.

Yeah, this would be useful for the CI too. What about getting a --required LIST/PATTERN/FILE flag for run-tests.py

I think it'd be most useful if it was something that didn't need to be specified each time, because if you remember to specify it, you'd probably also remember to check the output for the "skipped" message. But having a flag would work, we'd just be encouraging people to make wrapper scripts/functions/whatever; maybe most people already have something like that and I'm the odd one out for manually typing cd tests && ./run-tests.py -lj36 :)