1771483 - `mach clang-format` prints misleading error messages due to requiring an exact version match when its config is out-of-date relative to our clang version

Status: REOPENED

(Developer Infrastructure :: Lint and Formatting, defect, P3)

Description

[Tim Chevalier]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/101.0.4951.54 Safari/537.36

Steps to reproduce:

mach clang-format -s --outgoing

Actual results:

Output:

0:02.95 Setting up artifact clang-tidy.tar.zst
 0:02.95 Using artifact from local cache: /home/tjc/.mozbuild/toolchains/28739e61a849e888-clang-tidy.tar.zst
 0:03.10 untarring "/home/tjc/.mozbuild/clang-tools/clang-tidy.tar.zst"
 0:03.91 ERROR: You're using an old or incorrect version (clang-format version 14.0.4 (taskcluster-P6yBRSPoSOCJ3-sgYnaiUA)) of clang-format binary. Please update to a more recent one (at least > 14.0.3) by running: './mach bootstrap' 

When I checked the version of my clang-format binary, I got:

$ ~/.mozbuild/clang-tools/clang-tidy/bin/clang-format --version
clang-format version 14.0.4 (taskcluster-P6yBRSPoSOCJ3-sgYnaiUA)

Expected results:

Since 14.04 > 14.0.3, I would expect the command to succeed, or at least use "== 14.0.3" rather than "> 14.0.4" in the error message.

It seems that the bug is the _is_version_eligible() function in code_analysis/mach_commands.py ( https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/code_analysis/mach_commands.py#515 ):

if version in current_version:
        return True

Since the string '14.0.4' doesn't exactly match '14.0.3', the command fails even though the clang-format version I have installed is newer than the required one.

Comment 1

[Tim Chevalier]

Attachment: WIP: Bug 1771483: Allow newer clang-format versions in `mach clang-format`

Comment 2

[Tim Chevalier]

I have a fix in D147523 -- requested review from firefox-build-system-reviewers. Hopefully that's the right procedure!

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1771483 - Bump clang-tidy version

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ff4b2d42a77
Bump clang-tidy version

Comment 6

[Daniel Holbert [:dholbert]]

--> clarifying summary to more precisely define the issue, in a way that makes sense in light of the fix (version-bump) that we landed.

(The original summary "mach clang-format requires exact version match" is describing a situation that still exists, though of course it's only troublesome when we have a version mismatch -- which we did, but no longer do, as of glandium's patch.)

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/8ff4b2d42a77

Comment 8

[Tim Chevalier]

I don't think this is fixed. What happens the next time someone forgets to bump the version? I spent half an hour trying to figure out why mach was telling me that 3 is greater than 4, and I would like it if the next person didn't have to. At the very least, can the error message be changed to use the '==' sign rather than '>'?

Comment 9

[Tim Chevalier]

Attachment: WIP: Bug 1771483: Improve error message for version mismatch in `mach clang-format`

• Remove misleading error message that suggested mach will accept a newer
  version of clang-format than the expected one
• Print the full path to config.yaml in some error messages to make it
  easier to diagnose errors
• Log errors in code_analysis.utils methods rather than calling a non-
  existent logging method
• Handle errors properly in code_analysis.utils() methods rather than
  crashing when _clang_tidy_config is None.

Comment 10

[Tim Chevalier]

I submitted a different fix in D147649 that only improves error messages and doesn't change version parsing.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1771242

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:ahal, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.