1670949 - Plug "mach clang-format" to "mach lint -l clang-format" or deprecate it

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, task)

Description

[Sylvestre Ledru [:Sylvestre]]

Before we had mozlint, we introduced a mach command call "mach clang-format" see bug 952379 https://firefox-source-docs.mozilla.org/code-quality/coding-style/format_cpp_code_with_clang-format.html
In bug 1466070, we introduced support of clang-format into mozlint. This is pretty much the same thing, just leveraging mozlint support. See https://firefox-source-docs.mozilla.org/code-quality/lint/linters/clang-format.html

It is now time to deprecate "mach clang-format".

At least two options:

• Deprecate this mach command and recommend the other
• Plug "mach clang-format" to be just a shortcut to mach lint -l clang-format.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Andrew, Simon, do you have an opinion on this?
thanks!

Comment 2

[Andi [:andi]]

I have some comments regarding this approach. I agree that we should move ./mach clang-format to ./mach lint. Even though linting is not a well define behavior for a tool in C/C++ ecosystem, since you can argue that linting performs only static-analysis but on the other hand in the js world it also performs formatting, what's the case here, we should have a consistent behavior and infrastructure for all our tools that interact with source file as a programming language manipulator.
Also I have a big objection regarding the way how clang-format is currently integrated into ./mach lint, the process of bootstrapping is non existent right now, so we should take this into account when migrating from the old style to ./mach lint. At the moment if you have ran C++ static-analysis you will get just a python error saying that clang-format binary cannot be located and it doesn't perform actual steps too bootstrap you in a workable environment.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

the process of bootstrapping is non existent right now

Good point, could you please open a separate bug to get this? Should be easy as we already have the code for "mach clang-format/static-analysis"

Comment 4

[Simon Giesecke [:sg] [he/him]]

I don't know much about the implementation differences, so I will just rely on andi's opinion on that :)

For the UI: I would keep mach clang-format as a short-cut. Apart from being shorter, the lint term is very unusual in the C++ world. (Personally I wouldn't expect lint to make any changes to the source files for any programming language, at least not by default, in the same way I wouldn't expect the -fix option being a default for clang-tidy.)

Comment 5

[Andrew Halberstadt [:ahal]]

It would be pretty trivial to have a mach clang-format alias that dispatches to mach lint -l clang-format --fix (the same way mach eslint is an alias to mach lint -l eslint). Here's the implementation for mach eslint:
https://searchfox.org/mozilla-central/source/tools/lint/mach_commands.py#126

Long term bug 1511122 still feels like the best way to go to me.

Comment 7

[Mark Banner (:standard8)]

Note: bug 1389073 also has a discussion about this.

Comment 8

[Simon Friedberger [:simonf]]

Can we get rid of ./mach clang-format now? Afaict we have ./mach lint --linter clang-format since bug 1466070 and I'm not sure there is anything missing. And we have a few bugs related to ./mach clang-format like bug 1514279, bug 1545276, bug 1523073

Comment 9

[Andrew Halberstadt [:ahal]]

I think so? I'm not actually sure. Historically folks on Marco's team have maintained this. Marco any objections? Or can you redirect to whoever can make the call?

Edit: but just to clarify.. no objections from me.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

yeah, let's do it
(i will do it, should be easy)

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1670949 - remove ./mach clang-format in the ci r?#linter-reviewers

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1670949 - Remove deprecated `mach clang-format` code and update docs to use `mach lint -l clang-format` r?#linter-reviewers

• add a warning message

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5289bca11aa7
https://hg.mozilla.org/integration/autoland/rev/1b7c76df07a8
remove ./mach clang-format in the ci r=linter-reviewers,sergesanspaille
https://github.com/mozilla-firefox/firefox/commit/d8c329f668dc
https://hg.mozilla.org/integration/autoland/rev/dfb9e7cd0291
Remove deprecated `mach clang-format` code and update docs to use `mach lint -l clang-format` r=linter-reviewers,geckoview-reviewers,ai4dev-reviewers,suhaib,m_kato,sergesanspaille,ahal

Comment 14

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/1b7c76df07a8
https://hg.mozilla.org/mozilla-central/rev/dfb9e7cd0291