CI workflow to check clang-format usage on pull requests

[pmatos]

Adapted but heavily copied from LLVM version of pr-code-format.yml.

[pmatos]

Do not merge yet!

[pmatos]

I am using nektos/act to test this action locally.

[pmatos]

@Sonicadvance1 can you please create a read-only github token GITHUB_TOKEN ? Feel free to give it another name and let me know so I can update the workflow.

[Sonicadvance1]

@Sonicadvance1 can you please create a read-only github token GITHUB_TOKEN ? Feel free to give it another name and let me know so I can update the workflow.

What github token do you need? We don't use Github's CI runners and instead run everything on self-hosted runners. So this workflow will need some changes to facilitate that.

[pmatos]

@Sonicadvance1 can you please create a read-only github token GITHUB_TOKEN ? Feel free to give it another name and let me know so I can update the workflow.

What github token do you need? We don't use Github's CI runners and instead run everything on self-hosted runners. So this workflow will need some changes to facilitate that.

This one could run using Github CI runners, and it would avoid putting pressure on your local machines. What do you think?

[Sonicadvance1]

@Sonicadvance1 can you please create a read-only github token GITHUB_TOKEN ? Feel free to give it another name and let me know so I can update the workflow.

What github token do you need? We don't use Github's CI runners and instead run everything on self-hosted runners. So this workflow will need some changes to facilitate that.

This one could run using Github CI runners, and it would avoid putting pressure on your local machines. What do you think?

It's just setting up a python environment and running a script isn't it? The pressure on the self-hosted CI system should be negligible.

[pmatos]

@Sonicadvance1 can you please create a read-only github token GITHUB_TOKEN ? Feel free to give it another name and let me know so I can update the workflow.

What github token do you need? We don't use Github's CI runners and instead run everything on self-hosted runners. So this workflow will need some changes to facilitate that.

This one could run using Github CI runners, and it would avoid putting pressure on your local machines. What do you think?

It's just setting up a python environment and running a script isn't it? The pressure on the self-hosted CI system should be negligible.

That's fine by me. Let me refactor this considering it's going to run self-hosted.

[pmatos]

@Sonicadvance1 can you please create a read-only github token GITHUB_TOKEN ? Feel free to give it another name and let me know so I can update the workflow.

What github token do you need? We don't use Github's CI runners and instead run everything on self-hosted runners. So this workflow will need some changes to facilitate that.

This one could run using Github CI runners, and it would avoid putting pressure on your local machines. What do you think?

It's just setting up a python environment and running a script isn't it? The pressure on the self-hosted CI system should be negligible.

I have updated the PR not to use a secret and to run self-hosted.

But by not passing a secret, it means it won't be able to update the PR with the reason for failure like in llvm/llvm-project#88244 (comment)

[Sonicadvance1]

With some modifications to the workflow file you should be able to test this with our self-hosted runners while it is still in PR form

[pmatos]

With some modifications to the workflow file you should be able to test this with our self-hosted runners while it is still in PR form

Yes. Thanks. I will look at it first thing on Monday.

[pmatos]

With some modifications to the workflow file you should be able to test this with our self-hosted runners while it is still in PR form

I am getting this error:

rmacklin/fetch-through-merge-base@v0, tj-actions/changed-files@v39,
and aminya/setup-cpp@v1 are not allowed to be used in FEX-Emu/FEX.
Actions in this workflow must be: within a repository owned by FEX-Emu,
created by GitHub, or matching the following: sonicadvance1/, Sonicadvance1/, pmatos/*.

Can you update the permissions please?

[Sonicadvance1]

Updated permissions

[pmatos]

Updated permissions

• Nobody is picking up the job it seems.
• I understand these permissions open a possibility of getting code running on your machines, so if you think that granting these permissions is too much, we can consider using github hosted runners btw.

[pmatos]

The code-format-helper has a bug and makes --token a required argumnet. I have added a PR to llvm to fix this. Hoping it doesn't take much to get it merged.

llvm/llvm-project#88699

[Sonicadvance1]

Does the full llvm-repo need to be pulled in or can we just have those in an Externals/ folder that we control? Seems like overkill to pull all of llvm for two files.

[pmatos]

Does the full llvm-repo need to be pulled in or can we just have those in an Externals/ folder that we control? Seems like overkill to pull all of llvm for two files.

As far as I understand the sparse checkout just pulls those two files. We could have it in externals and sync when needed but it was to avoid pulling yet more externals.

[Sonicadvance1]

Didn't realize it was sparse, so that's good. Having it in Externals for one we control still sounds reasonable to me.

[pmatos]

Didn't realize it was sparse, so that's good. Having it in Externals for one we control still sounds reasonable to me.

yeah - was just thinking about it. If we pull it from llvm, it means that at a push of a button, llvm can break our CI which is not good.

I will pull it to externals then and ensure I check the need for sync on a regular basis.

[pmatos]

@Sonicadvance1 git-clang-format doesn't seem to be installed on the runner. Maybe installing clang-format-16 only installed git-clang-format-16?

[Sonicadvance1]

@Sonicadvance1 git-clang-format doesn't seem to be installed on the runner. Maybe installing clang-format-16 only installed git-clang-format-16?

It only installed clang-format-16 which is what the global clang-format is symlinked to. git-clang-format doesn't make any sense. Looks like their script supports the environment variable CLANG_FORMAT_PATH to set the path to the binary.
git-clang-format is probably only for their tree testing for git HEAD testing?

[pmatos]

@Sonicadvance1 git-clang-format doesn't seem to be installed on the runner. Maybe installing clang-format-16 only installed git-clang-format-16?

It only installed clang-format-16 which is what the global clang-format is symlinked to. git-clang-format doesn't make any sense. Looks like their script supports the environment variable CLANG_FORMAT_PATH to set the path to the binary. git-clang-format is probably only for their tree testing for git HEAD testing?

But it also created git-clang-format-16 right?
The reason we need git-clang-format is so we can use the --diff option to just apply clang-format to the changed code, rather than the whole file. The nuclear alternative is to just apply it to the whole file but the script assumes the --diff option exists which is not an option in clang-format.

[pmatos]

Nice - it works. A bad formatting of Frontend.cpp is caught by the CI. Logs show:

Running: git-clang-format-16 --diff 7e97db8d9879db18dd9f1467cee2845348f7cb35 342ca208240f489d3a557edeb3fc58ff2a475aba -- FEXCore/Source/Interface/Core/Frontend.cpp
error: clang-format exited with code 1
diff --git a/FEXCore/Source/Interface/Core/Frontend.cpp b/FEXCore/Source/Interface/Core/Frontend.cpp
index dc488a18ba..1d34b249c4 100644
--- a/FEXCore/Source/Interface/Core/Frontend.cpp
+++ b/FEXCore/Source/Interface/Core/Frontend.cpp
@@ -49,11 +49,9 @@ static uint32_t MapModRMToReg(uint8_t REX, uint8_t bits, bool HighBits, bool Has
     return FEXCore::X86State::REG_XMM_0 + Offset;
   } else if (HasMM) {
     return FEXCore::X86State::REG_MM_0 + Offset;
-  }
-  else if (!(HighBits && !HasREX))
+  } else if (!(HighBits && !HasREX))
     return FEXCore::X86State::REG_RAX + Offset;
 
-
   return GPR8BitHighIndexes[Offset];
 }
 
@@ -79,7 +77,7 @@ uint8_t Decoder::ReadByte() {
   LOGMAN_THROW_AA_FMT(InstructionSize < MAX_INST_SIZE, "Max instruction size exceeded!");
   Instruction[InstructionSize] = Byte;
   InstructionSize++;
-      return Byte;
+  return Byte;
 }
 
 uint8_t Decoder::PeekByte(uint8_t Offset) const {
Warning: C/C++ code formatter, clang-format detected some issues with your code formatting...

[Sonicadvance1]

@Sonicadvance1 git-clang-format doesn't seem to be installed on the runner. Maybe installing clang-format-16 only installed git-clang-format-16?

It only installed clang-format-16 which is what the global clang-format is symlinked to. git-clang-format doesn't make any sense. Looks like their script supports the environment variable CLANG_FORMAT_PATH to set the path to the binary. git-clang-format is probably only for their tree testing for git HEAD testing?

But it also created git-clang-format-16 right? The reason we need git-clang-format is so we can use the --diff option to just apply clang-format to the changed code, rather than the whole file. The nuclear alternative is to just apply it to the whole file but the script assumes the --diff option exists which is not an option in clang-format.

Oh, you're right. I didn't realize that was a different tool. It did install git-clang-format-16 I have now symlinked that to git-clang-format on the X64 runner.

[pmatos]

OK - testing complete. It seems to be working. I have made sure it only runs for FEX repo and pull requests to the main branch.

[pmatos]

@Sonicadvance1 ready for review.