This is an archive of the discontinued Mercurial Phabricator instance.

tests: allow Google's internal builds of clang-format to be used
ClosedPublic

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

Download Raw Diff

Details

Reviewers

durin42
Alphare

Group Reviewers

hg-reviewers

Commits

rHG1ff06ceb070f: tests: allow Google's internal builds of clang-format to be used

Summary

These builds do not actually include any Google-specific formatting changes; the
only reason they don't include the LLVM version number is due to a decision to
elide the version number from *all* LLVM/clang projects.

For most builds of clang-format, even "unofficial" ones, the LLVM version will
be displayed; example:

clang-format version 14.0.0 (https://github.com/llvm/llvm-project.git 1830ec94ac022ae0b6d6876fc2251e6b91e5931e)

The Google-internal build looks like this:

clang-format version google3-trunk (1830ec94ac022ae0b6d6876fc2251e6b91e5931e)

Diff Detail

Repository

rHG Mercurial

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptApr 30 2021, 7:20 PM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

spectral added a child revision: D10539: black: make codebase compatible with black v21.4b2 and v20.8b1.Apr 30 2021, 7:20 PM

spectral mentioned this in D10470: chg: pass --no-profile to disable profiling when starting hg serve.Apr 30 2021, 7:22 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.

In D10538#160300, @joerg.sonnenberger wrote:

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.

@joerg.sonnenberger, are you happy with @spectral's answer?

In D10538#160330, @spectral wrote:

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/)

In D10538#164587, @durin42 wrote:

In D10538#160330, @spectral wrote:

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.

In D10538#164634, @marmoute wrote:

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.

In D10538#164634, @marmoute wrote:

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

In D10538#178945, @marmoute wrote:

In D10538#164634, @marmoute wrote:

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 :)

In D10538#179035, @spectral wrote:

In D10538#178945, @marmoute wrote:

In D10538#164634, @marmoute wrote:

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.

I feel like the proposal is going in the wrong direction because it make a more diverse set of version passe the version checks while I feel like we need to more toward a narrower check. Different formater version tends to produce different results and it seems saner to pin the project to specific versions (that we update from time to time).

For the record, right now we have :

black: check says ">= 20.8b1", CI uses "20.8b1"
clang: check says ">= 11", CI uses debian's clang-format-11 package (in practice "Debian clang-format version 11.1.0-++20210622113218+1fdec59bffc1-1~exp1~20210622213839.163")
rust: check says "nightly-2020-10-04", CI uses "nightly-2020-10-04"

Is my feedback clearer and does it make sense to you? Or do you feel like I missed something?

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.

My own experience is more about running blindly formatting on my whole stack (hg fix --rev .#stack) and as this usually happens after I checked the details of the commits, unrelated formatting change might goes unnoticed. So laxer formatter version check might bite me here.

[likely] I'll break the CI with bad formatting less often :)

May I convince you todo CI runs on your changes before submitting them for review ? That would achieve the same purpose, and apply to more than just bad formatting.

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.

We could have a way to globally set config as we already have not for various options (through environment variable). I just want to avoid having casual contributor unable to do simple local test run because some tool are missing.

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 :)

I still manually type it. (except I make myself phenil alias that point to run-tests.py and pass all option blindly.

Note:

you don't need to cd tests to run the tests
you probably do not need to specify the -j 36, run-tests will pick you number of core as the default
with good cc-cache, the --local only save you a handful of second so I tend to skip it nowaday.

In D10538#179050, @marmoute wrote:

In D10538#179035, @spectral wrote:

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.

I feel like the proposal is going in the wrong direction because it make a more diverse set of version passe the version checks while I feel like we need to more toward a narrower check. Different formater version tends to produce different results and it seems saner to pin the project to specific versions (that we update from time to time).
For the record, right now we have :

black: check says ">= 20.8b1", CI uses "20.8b1"

clang: check says ">= 11", CI uses debian's clang-format-11 package (in practice "Debian clang-format version 11.1.0-++20210622113218+1fdec59bffc1-1~exp1~20210622213839.163")

rust: check says "nightly-2020-10-04", CI uses "nightly-2020-10-04"

Is my feedback clearer and does it make sense to you? Or do you feel like I missed something?

Do we know of specific formatting differences between clang-format 11/12/13/HEAD? It could be that clang-format has stabilized enough for C that we're fussing over nothing. If we're not fussing over nothing, maybe we should have a "swat a fly with a buick" solution and check in a script that abuses docker to always run an exact clang-format that contributors can use when they touch C code. I think that hg fix can be taught to only reformat edited lines too, so I would expect that in the common case there aren't any formatting differences in the vast majority of our patches, so we're strictly better off with contributors being bugged by clang-format test failures than not? Maybe we can add a --check mode to hg fix that could check only edited lines, thus dramatically reducing the rate of false positives?

Assuming there is substantial formatting skew: could we add an environment variable here a la "HGHAVE_ACCEPT_ANY_CLANG_FORMAT" so that people can opt-in to seeing HEAD-formatter results? Or maybe there's no substantive skew between clang 12 (or clang 13) and HEAD, and we can update the specific version we require.

(Basically, I'm wondering if we're arguing about a hypothetical problem, or an actual one. And if it's an actual one, if we can find some way to only format-check draft-phase patches that probably solves the issue anyway...)

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.

My own experience is more about running blindly formatting on my whole stack (hg fix --rev .#stack) and as this usually happens after I checked the details of the commits, unrelated formatting change might goes unnoticed. So laxer formatter version check might bite me here.

hg fix would already not check the version of clang-format involved, so I'm not sure this is strictly related.

[likely] I'll break the CI with bad formatting less often :)

May I convince you todo CI runs on your changes before submitting them for review ? That would achieve the same purpose, and apply to more than just bad formatting.

That's an extra step, and one that requires registering an account on a new service, etc. I don't like adding the expectation that people will use a (proprietary?) service as part of the contribution flow, even if that service is owned and operated by friends of the project.

I suppose, worst case, we could have a bot that can recognize formatting issues and just _does_ hg fix with the canonical version of formatters, and then it's not a big deal.

In D10538#179050, @marmoute wrote:

I feel like the proposal is going in the wrong direction because it make a more diverse set of version passe the version checks while I feel like we need to more toward a narrower check. Different formater version tends to produce different results and it seems saner to pin the project to specific versions (that we update from time to time).

Pinning the project to a specific supported version is fine, no one is arguing that.

Having the tests near-silently skip running in this situation is somewhat orthogonal. Please describe what harm you can imagine happening if this were accepted, meaning that developers that have a "trunk" version of clang-format on their path start seeing formatting errors (when replying, please consider both of these cases: (a) the user's trunk clang-format doesn't produce any differences from v11 [see below: a version from 3 days ago produces the same formatting as is currently checked in], (b) the user's trunk clang-format *does* produce differences from v11; also, for both scenarios, please describe how not running test-check-clang-format at all is better behavior than what I'm proposing we do).

[likely] I'll break the CI with bad formatting less often :)

May I convince you todo CI runs on your changes before submitting them for review ? That would achieve the same purpose, and apply to more than just bad formatting.

If you are suggesting to make the CI a standard step for sending changes for all developers, I could probably get behind that (not that you need my input, considering how few changes I send). However, as of right now, this is (afaik) not the way that most people (maybe I should say "most people outside of Octobus"?) operate. Until it is, I still think this change has a moderate positive value and essentially zero downsides.

In D10538#179057, @durin42 wrote:

Do we know of specific formatting differences between clang-format 11/12/13/HEAD? It could be that clang-format has stabilized enough for C that we're fussing over nothing.

Not that I'm aware of; I likely wouldn't have sent the change if there'd been anything. I'm running a version of clang-format built from commit 1830ec94ac, which was from Friday (3 days ago), and if I force test-check-clang-format.t to run, it doesn't produce any warnings or diffs.

spectral planned changes to this revision.Oct 19 2021, 9:30 PM

spectral retitled this revision from tests: allow trunk versions of clang-format to be used to tests: allow Google's internal builds of clang-format to be used.Oct 20 2021, 5:48 PM

spectral edited the summary of this revision. (Show Details)

spectral updated this revision to Diff 30961.

In D10538#179057, @durin42 wrote:

In D10538#179050, @marmoute wrote:

In D10538#179035, @spectral wrote:

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.

I feel like the proposal is going in the wrong direction because it make a more diverse set of version passe the version checks while I feel like we need to more toward a narrower check. Different formater version tends to produce different results and it seems saner to pin the project to specific versions (that we update from time to time).
For the record, right now we have :

black: check says ">= 20.8b1", CI uses "20.8b1"

clang: check says ">= 11", CI uses debian's clang-format-11 package (in practice "Debian clang-format version 11.1.0-++20210622113218+1fdec59bffc1-1~exp1~20210622213839.163")

rust: check says "nightly-2020-10-04", CI uses "nightly-2020-10-04"

Is my feedback clearer and does it make sense to you? Or do you feel like I missed something?

Do we know of specific formatting differences between clang-format 11/12/13/HEAD? It could be that clang-format has stabilized enough for C that we're fussing over nothing.

We have had clang version related issue no later than 6 month ago. And all the people I know to use clang do some serious version pining.

Our C codebase is probably simple enough that they are rare, and according to @spectral , things seems "fine" right now, but who know for how long.

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.

My own experience is more about running blindly formatting on my whole stack (hg fix --rev .#stack) and as this usually happens after I checked the details of the commits, unrelated formatting change might goes unnoticed. So laxer formatter version check might bite me here.

hg fix would already not check the version of clang-format involved, so I'm not sure this is strictly related.

@spectral did a list of way people run the formatter, might checks the result afterward and how likely it is to happens. My usage pattern and associated likeliness was not listed so I added a comment.

In D10538#179281, @spectral wrote:

In D10538#179050, @marmoute wrote:

I feel like the proposal is going in the wrong direction because it make a more diverse set of version passe the version checks while I feel like we need to more toward a narrower check. Different formater version tends to produce different results and it seems saner to pin the project to specific versions (that we update from time to time).

Pinning the project to a specific supported version is fine, no one is arguing that.

I am happy that we agree on that.

Having the tests near-silently skip running in this situation is somewhat orthogonal.

The test near silently skipping is another issue we should tackle. What do you think of the way forward I suggested (flag + env-variable) ?

Please describe what harm you can imagine happening if this were accepted, meaning that developers that have a "trunk" version of clang-format on their path start seeing formatting errors (when replying, please consider both of these cases: (a) the user's trunk clang-format doesn't produce any differences from v11 [see below: a version from 3 days ago produces the same formatting as is currently checked in], (b) the user's trunk clang-format *does* produce differences from v11; also, for both scenarios, please describe how not running test-check-clang-format at all is better behavior than what I'm proposing we do).

If the local test-run and the CI test-run differs, the user probably need a couple of round trip to figure out what is happening and will have to live with a perpetual red test locally. Both are harmful situation.

[likely] I'll break the CI with bad formatting less often :)

May I convince you todo CI runs on your changes before submitting them for review ? That would achieve the same purpose, and apply to more than just bad formatting.

If you are suggesting to make the CI a standard step for sending changes for all developers, I could probably get behind that (not that you need my input, considering how few changes I send).

I am not suggesting that, but if regular contributor could get into the habit of doing so it would be great. (In addition to people queuing patch so that we avoid "CI broken on default" situations.

However, as of right now, this is (afaik) not the way that most people (maybe I should say "most people outside of Octobus"?) operate.

Actually, multiple other people do, both regular contributors and new-comers. The CI is actually a simpler way to run the full test-suite for normal people.

I see that you updated your diff to directly target the google specific naming scheme (I understand the motivation for your change better now). I guess this is fine.

Alphare accepted this revision.Oct 28 2021, 5:00 AM

This revision is now accepted and ready to land.Oct 28 2021, 5:00 AM

spectral added a commit: rHG1ff06ceb070f: tests: allow Google's internal builds of clang-format to be used.Oct 28 2021, 5:16 AM

Closed by commit rHG1ff06ceb070f: tests: allow Google's internal builds of clang-format to be used (authored by spectral). · Explain Why

This revision was automatically updated to reflect the committed changes.

In D10538#179907, @marmoute wrote:

The test near silently skipping is another issue we should tackle. What do you think of the way forward I suggested (flag + env-variable) ?

As long as I don't have to remember to put it in my commandline each time, I'm fine with whatever.

Revision Contents
Changeset List

			Path	Packages
M			tests/hghave.py (9 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	27264		Apr 30 2021, 7:20 PM	★	★
Diff 2	30961		Oct 20 2021, 5:48 PM	★	★
Diff 3	30998	rHG1ff06ceb070fd17ee7423bc6c68edb90c451587d	Apr 30 2021, 7:00 PM	★	★

Status	Author	Revision
Closed	spectral	D10539 black: make codebase compatible with black v21.4b2 and v20.8b1
Closed	spectral	D10538 tests: allow Google's internal builds of clang-format to be used
Abandoned	spectral	D10537 tests: fix chg tests missed in D10470

Diff 30998

tests/hghave.py

	def has_pylint():			def has_pylint():
	return matchoutput("pylint --help", br"Usage:[ ]+pylint", True)			return matchoutput("pylint --help", br"Usage:[ ]+pylint", True)


	@check("clang-format", "clang-format C code formatter (>= 11)")			@check("clang-format", "clang-format C code formatter (>= 11)")
	def has_clang_format():			def has_clang_format():
	m = matchoutput('clang-format --version', br'clang-format version (\d+)')			m = matchoutput('clang-format --version', br'clang-format version (\d+)')
	# style changed somewhere between 10.x and 11.x			# style changed somewhere between 10.x and 11.x
	return m and int(m.group(1)) >= 11			if m:
				return int(m.group(1)) >= 11
				# Assist Googler contributors, they have a centrally-maintained version of
				# clang-format that is generally very fresh, but unlike most builds (both
				# official and unofficial), it does not include a version number.
				return matchoutput(
				'clang-format --version', br'clang-format .*google3-trunk \([0-9a-f]+\)'
				)


	@check("jshint", "JSHint static code analysis tool")			@check("jshint", "JSHint static code analysis tool")
	def has_jshint():			def has_jshint():
	return matchoutput("jshint --version 2>&1", br"jshint v")			return matchoutput("jshint --version 2>&1", br"jshint v")


	@check("pygments", "Pygments source highlighting library")			@check("pygments", "Pygments source highlighting library")