search

google / llvm-premerge-checks

CI system for premerge-testing in LLVM project
Apache License 2.0
41 stars 37 forks source link

Phabricator lint is not using the latest version of clang-format #303

Open psteinfeld opened 3 years ago

psteinfeld commented 3 years ago

I built clang-format from the latest clang sources. I then used it to format all of the .h and .cpp files in the flang project and submitted the Phabricator review https://reviews.llvm.org/D102424.

In the review Phabricator complains that several of the files need to be reformatted. It's as if the version of clang-format that Phabricator is using doesn't match the one produced by building clang-format from the latest sources. One example file is flang/lib/Evaluate/characteristics.cpp

joker-eph commented 3 years ago

The premerge testing is using in general clang-format from the latest LLVM release.

psteinfeld commented 3 years ago

The premerge testing is using in general clang-format from the latest LLVM release.

I'm not sure about the phrase "in general". The files in https://reviews.llvm.org/D102424 were formatted with a version of clang-format built from the latest sources, but Phabricator is complaining about their formatting.

joker-eph commented 3 years ago

I meant by "in general" that it is how it is supposed to be, but there can also be some delay between the exact moment a new release come out and when the infrastructure actually gets updated.

metaflow commented 3 years ago

Thank you for the report! I will check what version is used, might be it's not 12 yet 🐉

psteinfeld commented 3 years ago

Thank you for the report! I will check what version is used, might be it's not 12 yet 🐉

Thanks, Mikhail. You're the best.

metaflow commented 3 years ago

@psteinfeld I have updated toolchain to 12 (latest stable) and already saw a reduction of clang-tidy warnings for some builds. Feel free to reopen this if something is off.

psteinfeld commented 3 years ago

Thanks, @metaflow . My development process is to build and use the latest versions of the LLVM tools while developing on the latest version of LLVM. There seems to be general agreement, at least within the flang community, that this is the best process. When I build the latest version of clang-format, it says that its version is 13.0.0.

Can you please upgrade to version 13.0.0?

I couldn't figure out how to re-open this issue. I suspect that I don't have permission.

Thanks for working on this.

metaflow commented 3 years ago

@psteinfeld I understand. AFAIK version 13 is not released yet and it's just "trunk". Ideally, we would like to use the development version of the tools but that will require us to either periodically build and distribute them to build-agents; or build clang-tidy / clang-format for every commit. Maybe the second option is not that expensive. I will check if adding clang-tidy and clang-format affects build time.

psteinfeld commented 3 years ago

Thanks, @metaflow . You're the best.

joker-eph commented 3 years ago

I never ever built clang-format locally, and that is likely the situation most people in the community are in. The released binaries for clang-format are published on llvm.org: this is much simpler to just grab a new version every 6 months than rebuilding continuously locally a version of clang-format to keep up.

ChristianKuehnel commented 3 years ago

The LLVM policy does not really say anything around the version of clang-format to be used.

Should we add something like?

It's highly recommend to used the latest released version of clang-format (see https://releases.llvm.org/).