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

Add clang-tidy to Github Actions #61

Open
GleasonK opened this issue Aug 30, 2022 · 3 comments
Open

Add clang-tidy to Github Actions #61

GleasonK opened this issue Aug 30, 2022 · 3 comments
Labels

Comments

@GleasonK
Copy link
Member

Request description

From feedback in #49 - Add a clang-tidy task to the lint action.

Can be bootstrapped from MHLO:
https://github.com/tensorflow/mlir-hlo/blob/master/.clang-tidy

Additional context

No response

@GleasonK
Copy link
Member Author

GleasonK commented Sep 9, 2022

Started looking into this, and a few notes:

There are quite a few clang-tidy errors in the files we bootstrapped from mlir-hlo. Because of this I was running clang-tidy-diff to filter by the changes in the git changelist, aka clang-tidy issues that the PR owner is likely responsible for:

git diff -U0 HEAD^ | clang-tidy-diff.py -p1 -path /path/to/stablehlo-build/

Second it seems like clang-tidy takes quite a bit of time to complete (Only being run on StablehloOps.cpp locally on my fairly powerful machine takes 5m37.121s).

Third, maybe someone more familiar with clang-tidy knows better, but it requires a compilation database. Does this mean that it must be run after build and test has completed? We may want to split these action jobs out so that lint doesn't have to occur after build, unless we feel that is right.

This raises the question - how often would we want to run this test? Every PR? If running serially is required, then each update to a PR (even if ccached) may trigger 20+min of actions. I can run these tests in my fork for a bit and get a feel for the overhead.

@burmako burmako added Build and removed Process labels Sep 15, 2022
@GleasonK
Copy link
Member Author

Did some testing last week in actions-tidy branch. The CI environment does not seem to work well with clang-tidy (ref). This trace makes me think it is a configuration issue more than anything. I'll try a few more things like building clang-tidy from llvm source in CI, or installing a different version via package management and running that.

@GleasonK
Copy link
Member Author

Unassigning for now. I hope to get to this in the near future, but this has lower priority than compatibility work, as well as some other early-stage proposals in the works.

@GleasonK GleasonK removed their assignment Oct 10, 2022
ghpvnist added a commit that referenced this issue Feb 9, 2023
Manually fixing as part of clang-tidy check which is currently not
supported in StableHLO #61
atondwal pushed a commit to atondwal/stablehlo that referenced this issue Mar 3, 2023
Manually fixing as part of clang-tidy check which is currently not
supported in StableHLO openxla#61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants