Enzyme
Enzyme copied to clipboard
Add enzyme/tools to clang-format check
Some misc work are done in this PR:
- move
.clang-format
to the top directory, as a common way - add
pull_request
event for the clang-format check CI - add enzyme/tools dir for clang-format check
- format enzyme/tools dir via clang-format
- remove some useless content (e.g. CMakeLists.txt in the exclude list, which is nonsense)
Close #1211. cc @wsmoses @ZuseZ4
@tgymnich @wsmoses iirc we had a reason to exclude PRs? Also, how about running the rest of CI only once clang-fmt passes, if it's not too much work? Formating needs to get fixed before merging anyway and might save us CI time or the work to manually cancel old runs which already failed due to clang-format.
Also, how about running the rest of CI only once clang-fmt passes, if it's not too much work? Formating needs to get fixed before merging anyway and might save us CI time or the work to manually cancel old runs which already failed due to clang-format.
Refer to https://stackoverflow.com/questions/58457140/dependencies-between-workflows-on-github-actions , it seems we can add workflow_run
clause to other workflows.
@wsmoses any objections to it? It's nothing big, but it is easy to fix formating when it breaks CI and it is annoying to go trough all of our CI categories to cancel runs in which the formating has failed. Having clang-format as first check would prevent that and clang format also doesn't take long to run (15s).
@ZuseZ4 yeah if its already run on every commit push, also running for PR (which would require a commit) is redundant.
I'm fine with the don't run the rest of CI, but let's enable it for everything except the LLVM CI (that way it can still check that it builds on all versions), which takes the least time.
Sounds good @PragmaTwice, would you be fine with implementing these changes (removing runs on PR and adding the two level CI)?
yeah if its already run on every commit push, also running for PR (which would require a commit) is redundant.
Thanks, I can get your point. But I think it is not friendly to outside contributors who fork this repo and open pull requests from their own fork, in which case these CI status does not appear in the PR page.
It seems an alternative solution is something like:
on:
push:
branches:
- main
pull_request:
So that if you push to a branch X
(not main
) and open a pull request upon X
, the format CI will only be triggered once per commit.
That sounds good, feel free to implement it this way :)
LGTM. There are just a few minor formatting issues left.
Conflict has been solved now.
It seems this method cannot work well, i.e. these workflows by workflow_run
event cannot be triggered as expected.
Maybe need some investigation or workaround, or just keep the original behavior and try it in another PR.
https://stackoverflow.com/questions/72551643/workflow-run-not-triggered-as-expected-after-parent-workflow-completes
Maybe the root cause is from the quote. Some trying...
It does look like Integration CI is now blocked on all Enzyme CI tests passing. Especially the apple runners take a while, so by now it would increase our CI times by a good amount. Can you update it please to make sure that we are only waiting for the clang-format check to pass before starting Integration CI?
Edit: looks like we noticed it simultaneously.
Edit: looks like we noticed it simultaneously.
:rofl:
https://stackoverflow.com/questions/72551643/workflow-run-not-triggered-as-expected-after-parent-workflow-completes Maybe the root cause is from the quote. Some trying...
Some try, still not triggered. I will revert all these workflow_run
changes firstly.
Hi @ZuseZ4 @wsmoses , now it is reverted from workflow_run
clauses to build dependencies between workflows.
I think we can firstly merge current changes to check tblgen code format and prevent further conflicts, and try to make workflow dependencies later.