Enzyme icon indicating copy to clipboard operation
Enzyme copied to clipboard

Add enzyme/tools to clang-format check

Open PragmaTwice opened this issue 1 year ago • 15 comments

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

PragmaTwice avatar May 23 '23 10:05 PragmaTwice

@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.

ZuseZ4 avatar May 23 '23 21:05 ZuseZ4

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.

PragmaTwice avatar May 24 '23 03:05 PragmaTwice

@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 avatar May 24 '23 03:05 ZuseZ4

@ZuseZ4 yeah if its already run on every commit push, also running for PR (which would require a commit) is redundant.

wsmoses avatar May 24 '23 14:05 wsmoses

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.

wsmoses avatar May 24 '23 14:05 wsmoses

Sounds good @PragmaTwice, would you be fine with implementing these changes (removing runs on PR and adding the two level CI)?

ZuseZ4 avatar May 24 '23 14:05 ZuseZ4

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.

PragmaTwice avatar May 24 '23 16:05 PragmaTwice

That sounds good, feel free to implement it this way :)

ZuseZ4 avatar May 25 '23 19:05 ZuseZ4

LGTM. There are just a few minor formatting issues left.

tgymnich avatar May 26 '23 12:05 tgymnich

Conflict has been solved now.

PragmaTwice avatar May 27 '23 03:05 PragmaTwice

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.

PragmaTwice avatar May 27 '23 03:05 PragmaTwice

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...

PragmaTwice avatar May 27 '23 03:05 PragmaTwice

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.

ZuseZ4 avatar May 27 '23 04:05 ZuseZ4

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.

PragmaTwice avatar May 27 '23 04:05 PragmaTwice

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.

PragmaTwice avatar May 28 '23 03:05 PragmaTwice