Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parallelize clang-tidy runs #4731

Merged
merged 2 commits into from
Mar 17, 2020
Merged

Parallelize clang-tidy runs #4731

merged 2 commits into from
Mar 17, 2020

Conversation

abadams
Copy link
Member

@abadams abadams commented Mar 16, 2020

Using a script provided by llvm. This makes it way faster to clang-tidy a branch before opening a PR.

Using a script provided by llvm
@steven-johnson
Copy link
Contributor

On my Mac, make clang-tidy-fix fails with /Users/srj/llvm-trunk-install/bin/../share/clang/run-clang-tidy.py: No such file or directory

@abadams
Copy link
Member Author

abadams commented Mar 17, 2020

Does /Users/srj/llvm-trunk-install/bin/clang-tidy exist for you? On top of the instructions in the readme I think you need to do a make install and enable the clang-tools-extra project.

@steven-johnson
Copy link
Contributor

Does /Users/srj/llvm-trunk-install/bin/clang-tidy exist for you? On top of the instructions in the readme I think you need to do a make install and enable the clang-tools-extra project.

Nope. We should add that to the README.

@abadams
Copy link
Member Author

abadams commented Mar 17, 2020

Done.

@steven-johnson
Copy link
Contributor

After doing that, I now can run it on OSX, but it is unable to find ~any of the system , e.g. /Users/srj/GitHub/Halide/src/IR.h:8:10: error: 'string' file not found [clang-diagnostic-error] #include <string>

@steven-johnson
Copy link
Contributor

I also get a number of warnings of the form

/Users/srj/GitHub/Halide/src/IR.h:666:68: error: the parameter 'param' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
    static Expr make(Type type, const std::string &name, Parameter param) {

@abadams
Copy link
Member Author

abadams commented Mar 17, 2020

Can you try adding libcxx;libcxxabi to the list of enabled projects? For this to work you need to be able to actually compile c++ with the llvm that you built.

Those warnings sound like things that should be fixed. Not sure why I don't get them with my clang-tidy.

@steven-johnson
Copy link
Contributor

libcxx;libcxxabi

Trying now, but FWIW, that adds a lot to the build (> 1000 files).

Since clang-tidy might be available in prebuilt form, it might be worth smartening our script to do something like:
(1) if $CLANG_TIDY is defined, use that
(2) else, if which clang-tidy reports it is available, use that
(3) else, use the logic based on $LLVM_CONFIG that we do now

(AFAICT, Homebrew doesn't offer a clang-tidy formula on Mac, but the prebuilts you can download include it)

@abadams
Copy link
Member Author

abadams commented Mar 17, 2020

I think the Makefile is already smart in that way. For any complete llvm install, prebuilt or not, setting LLVM_CONFIG correctly will find the rest of it. It sounds like to run clang-tidy you need a more complete llvm install than you currently have? Or maybe the homebrew prebuilt is only a partial build of llvm?

The default value of RUN_CLANG_TIDY points to llvm's standard install location for run-clang-tidy.py using the location of CLANG, which comes from querying llvm-config where to look.

@steven-johnson
Copy link
Contributor

I think the Makefile is already smart in that way. For any complete llvm install, prebuilt or not, setting LLVM_CONFIG correctly will find the rest of it. It sounds like to run clang-tidy you need a more complete llvm install than you currently have? Or maybe the homebrew prebuilt is only a partial build of llvm?

The default value of RUN_CLANG_TIDY points to llvm's standard install location for run-clang-tidy.py using the location of CLANG, which comes from querying llvm-config where to look.

I'm building from scratch, with LLVM_ENABLE_PROJECTS="clang;lld;clang-tools-extra" (still rebuilding the libcxx stuff)

@steven-johnson
Copy link
Contributor

(FWIW: I never bothered trying make clang-tidy locally before now anyway, so it was likely broken already, and I will probably rely on the presubmit checks anyway most of the time. That said, might as well figure out the failure modes while we are here.)

@abadams
Copy link
Member Author

abadams commented Mar 17, 2020

Yeah, I'm assuming this PR doesn't actually change the requirements for you. If run-clang-tidy.py doesn't work then probably clang-tidy didn't either.

@steven-johnson
Copy link
Contributor

FYI: after rebuilding llvm with the libcxx;libcxxabi projects enabled, make clang-tidy is unchanged for me on this branch (still can't find system headers).

FYI #2: on the master branch, make clang-tidy just fails outright for me like so:

echo '[' >> bin/build/compile_commands.json
bash: bin/build/compile_commands.json: No such file or directory
make: *** [bin/build/compile_commands.json] Error 1

so, I guess this is a net improvement? Something is wonky but fine with me to land if you like.

@abadams
Copy link
Member Author

abadams commented Mar 17, 2020

master is missing a key mkdir. It would be interesting to know if clang-tidy works if you manually create bin/build first.

@steven-johnson
Copy link
Contributor

(OK to land this without the buildbot testing IMHO)

@abadams abadams merged commit 9b8ad0f into master Mar 17, 2020
@abadams abadams deleted the parallel_clang_tidy branch March 17, 2020 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants