[CI] clang-format step in lit run broken

[jsji]

Describe the bug

Looks like the lint CI is broken? The log in clang-format step shows fatal error, and the uploaded patch is always 0 bytes.

git config --global --add safe.directory /__w/llvm/llvm
git clang-format ${GITHUB_SHA}^1
git diff > ./clang-format.patch
shell: bash {0}
fatal: ambiguous argument '372cc94890e4e5334b7d64e29a55eee3480a9922^1': unknown revision or path not in the working tree.

Uploaded '0' bytes from '/home/runner/work/llvm/llvm/clang-format.patch' to server

All CI jobs are failing with same log.

eg: https://github.com/intel/llvm/runs/8042514327?check_suite_focus=true
Click the lint, then click the run clang-format section, you will be able to see logs similar to above.

[aelovikov-intel]

@jsji , can you please add a link to the failing PR/task?

[jsji]

@jsji , can you please add a link to the failing PR/task?
Sure, all CI are failing with same log, so any of them should be OK.

eg: https://github.com/intel/llvm/runs/8042514327?check_suite_focus=true
Click the lint, then click the run clang-format section, you will be able to see logs similar to above.

[aelovikov-intel]

I thought the Lint task is reported as failing while in fact it's green but checks nothing.

[AlexeySachkov]

I think I might have an idea of what went wrong.

Presumably, the guilty commit is #6471. It changed the trigger for pre-commit workflow from pull_request to pull_request_target

They look similar, but they set different values to GITHUB_SHA environment variable which is used to ,compute a diff for clang-format input.

For pull_request:

Last merge commit on the GITHUB_REF branch

For pull_request_target:

Last commit on the PR base branch

The author of the change is @apstasen and I see no comments or explanations why it was done. If you look for "target", then you will find at least 3 commits which change the workflow trigger from pull_request to pull_request_target.

If you go through sycl_precommit action history (at the moment around page 11), then you can find correct and incorrect launches of clang-format there. For example: correct one and the very next which is already incorrect.

The first one launched on Aug 12 2:50 AM GMT +2, the second one launched on Aug 12 4:43 AM GMT +2. #6471 was merged on Aug 12 12:07 AM GMT +2, which doesn't really proves anything, but I suppose that some PRs could have used the older actions script because they weren't up-to-date with sycl branch at the moment.

Could someone do a quick experiment to see if reverting the workflow trigger back helps to fix clang-format? @apstasen, do you have any insights about why exactly it is needed to change the workflow trigger?

[apstasen]

Workflow trigger was changed from pull_request to pull_request_target because pull_request does not pass repository secrets we will be using for AWS testing in future.

[jsji]

Thanks @apstasen for fixing this in #6652 . I see the new CI are fixed now, eg: https://github.com/intel/llvm/runs/8111821254?check_suite_focus=true.

[alexbatashev]

@apstasen why did you need to change it to pull_request_target in the first place? The GitHub-recommended solution uses a very different approach: https://docs.github.com/en/actions/hosting-your-own-runners/autoscaling-with-self-hosted-runners

[apstasen]

Approaches described in https://docs.github.com/en/actions/hosting-your-own-runners/autoscaling-with-self-hosted-runners will also require pull_request_target or separate process outside of current CI to access AWS Lambda or k8s endpoint services. Having extra endpoint is also an extra security risk and can cause AWS charges in case of DDOS.

Also described solutions are much more complex to deploy, harder to debug (since there will be no logs on github side about instance spawning), and in general using terraform or k8s is overkill for the purposes of starting of couple of spot/demand instances.

[apstasen]

It is possible to make AWS lambda service without externally visible endpoint (and that will not require any secrets in the github repo so we can use pull_request trigger like before) that will check for need of runners in specific Github repo every couple of seconds and we actually use similair approach for other internal projects but is also have drawbacks like I already mentioned above:

1. lack of logs for runner creation on Github repo side;
2. code and majority of configuration will be likely placed outside of current Github repo (basically we will only have github runner name name and labels and how this will translate into AWS instance properties will be not so transparent);
3. need to rely on interval check logic to start/stop AWS EC2 instances which is not optimal and can cause extra AWS charges (for example from https://github.com/philips-labs/terraform-aws-github-runner#overview: "Scaling down the runners is at the moment brute-forced, every configurable amount of minutes a lambda will check every runner (instance) if it is busy").