Open Bug 1670949 Opened 4 years ago Updated 5 months ago

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

Categories

(Developer Infrastructure :: Lint and Formatting, task)

Tracking

(Not tracked)

People

(Reporter: Sylvestre, Unassigned)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

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.
Depends on: 1670950
Depends on: 1670951

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

Flags: needinfo?(sgiesecke)
Flags: needinfo?(ahal)

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.

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"

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

Flags: needinfo?(sgiesecke)
Depends on: 1671120

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.

Flags: needinfo?(ahal)
Product: Firefox Build System → Developer Infrastructure
Duplicate of this bug: 1389073

Note: bug 1389073 also has a discussion about this.

Blocks: 1514279
You need to log in before you can comment on or make changes to this bug.