Page MenuHomePhabricator

[clang-tidy] Install run-clang-tidy.py in bin/ as run-clang-tidy
ClosedPublic

Authored by Flow on Nov 6 2020, 2:06 PM.

Details

Summary

The run-clang-tidy.py helper script is supposed to be used by the
user, hence it should be placed in the user's PATH. Some
distributions, like Gentoo [1], won't have it in PATH unless it is
installed in bin/.

Furthermore, installed scripts in PATH usually do not carry a filename
extension, since there is no need to know that this is a Python
script. For example Debian and Ubuntu already install this script as
'run-clang-tidy' [2] and hence build systems like Meson also look for
this name first [3]. Hence we install run-clang-tidy.py as
run-clang-tidy, as suggested by Sylvestre Ledru [4].

1: https://bugs.gentoo.org/753380
2: https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/60aefb14171ab5c3867a0081844b507fc9f6e015/debian/clang-tidy-X.Y.links.in#L2
3: https://github.com/mesonbuild/meson/blob/b6dc4d5e5c6e838de0b52e62d982ba2547eb366d/mesonbuild/scripts/clangtidy.py#L44
4: https://reviews.llvm.org/D90972#2380640

Diff Detail

Event Timeline

Flow created this revision.Nov 6 2020, 2:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2020, 2:06 PM
Flow requested review of this revision.Nov 6 2020, 2:06 PM

Adding involved persons from https://reviews.llvm.org/D12700 as reviewers, which was the change introducing share/clang/ as install location.

FWIW, I am doing this change already in Debian & Ubuntu ( https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/10/debian/clang-tidy-X.Y.links.in#L2 )

By the way, while you are working on it, why not removing the python extension when installing? We don't need to know that it is Python code.

Flow added a comment.Nov 7 2020, 4:25 AM

By the way, while you are working on it, why not removing the python extension when installing? We don't need to know that it is Python code.

Thanks for the feedback. Please see https://reviews.llvm.org/D91001

sylvestre.ledru requested changes to this revision.Nov 7 2020, 4:41 AM

Thanks.
Could you please also update the release notes for 12?

And if you are motivated, this script should be documented (or at least mentioned in the doc a bit more)
I only found two references in:
clang-tools-extra/docs/clang-tidy/Contributing.rst

This revision now requires changes to proceed.Nov 7 2020, 4:41 AM
Flow updated this revision to Diff 303638.Nov 7 2020, 6:21 AM
Flow retitled this revision from [clang-tidy] Install run-clang-tidy.py in bin/ not in share/clang/ to [clang-tidy] Install run-clang-tidy.py in bin/ as run-clang-tidy.
Flow edited the summary of this revision. (Show Details)
sylvestre.ledru accepted this revision.Nov 7 2020, 6:35 AM

Sounds great. You might want to ask feedback to other reviews too!

This revision is now accepted and ready to land.Nov 7 2020, 6:35 AM
Eugene.Zelenko added inline comments.Nov 7 2020, 7:06 AM
clang-tools-extra/docs/ReleaseNotes.rst
76

Please enclose run-clang-tidy.py, bin/, run-clang-tidy and share/clang/ in single back-ticks.

Flow updated this revision to Diff 303642.Nov 7 2020, 7:57 AM

Use backticks in ReleaseNotes

Flow marked an inline comment as done.Nov 7 2020, 7:57 AM
JonasToth accepted this revision.Nov 13 2020, 10:06 AM
JonasToth added a subscriber: JonasToth.

LGTM! thanks for fixing.

Flow requested review of this revision.Dec 10 2020, 10:09 AM

LGTM! thanks for fixing.

I am unable to commit this myself. The latest version of this change can be found in the install-run-clang-tidy-into-bin on my LLVM fork.

LGTM! thanks for fixing.

I am unable to commit this myself. The latest version of this change can be found in the install-run-clang-tidy-into-bin on my LLVM fork.

Noticed this is a little stale, Do you still want this landing?

Flow added a comment.Feb 22 2021, 10:09 PM

LGTM! thanks for fixing.

I am unable to commit this myself. The latest version of this change can be found in the install-run-clang-tidy-into-bin on my LLVM fork.

Noticed this is a little stale, Do you still want this landing?

This probably was not addressed to me, but yes I want this to land but need someone to sponsor the commit. 😀

This revision was not accepted when it landed; it landed in state Needs Review.Feb 23 2021, 4:38 AM
This revision was automatically updated to reflect the committed changes.