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.