Run LLVM install script on Linux runners

[mirenradia]

This will add the relevant LLVM PPA to the runner's package manager which provides versions of clang-format and clang-tidy that may not yet be available in the official Ubuntu repositories (e.g. LLVM 16 on Ubuntu 22.04 at the time this commit was authored).

This resolves #145.

[2bndy5]

I fed your changes into the test repo, and it looks like the added PPA doesn't support v9-v12 on Ubuntu 22.04 (jammy). See this workflow run. So, we might be only able to use this approach for select versions of LLVM (v13-v16 as of this writing). In the future, we might have to keep updating the action.yml when the github runner's ubuntu image is updated.

[shenxianpeng]

Maybe we just need to use llvm_install.sh for installing version 16 enough.

if [[ "${{ inputs.version }}" == "16" ]]; then
  # This LLVM script will add the relevant LLVM PPA: https://apt.llvm.org/
  wget https://apt.llvm.org/llvm.sh -O $GITHUB_ACTION_PATH/llvm_install.sh
  chmod +x $GITHUB_ACTION_PATH/llvm_install.sh
  sudo $GITHUB_ACTION_PATH/llvm_install.sh ${{ inputs.version }} || true
fi

[2bndy5]

That could also work.

[mirenradia]

I fed your changes into the test repo, and it looks like the added PPA doesn't support v9-v12 on Ubuntu 22.04 (jammy). See this workflow run. So, we might be only able to use this approach for select versions of LLVM (v13-v16 as of this writing). In the future, we might have to keep updating the action.yml when the github runner's ubuntu image is updated.

Ah, I had hoped the || true would save me from this problem but it doesn't seem to have done so.

Maybe we just need to use llvm_install.sh for installing version 16 enough.

It would be good if there was a way to avoid hardcoding the number 16 so that this change would be robust to upgrades LLVM versions (and runner Ubuntu version). Maybe it should try installing the package from the Ubuntu package repos first and only try this LLVM install script if the former fails?

[2bndy5]

Maybe it should try installing the package from the Ubuntu package repos first and only try this LLVM install script if the former fails?

I like this idea better because it seems more future-proof. 👍🏼

[mirenradia]

OK, I've tried to implement what I suggested above. There's probably a syntax error somewhere as I haven't got a good way to test GitHub actions locally so please let me know if there are problems when the relevant workflows have run.

[2bndy5]

This works!

Now, I hate to ask for more changes because your time is very appreciated. I think we can reduce the steps back into 1:

if [[ "${{runner.os}}"  == "Linux" ]]; then
    sudo apt-get update
    if ! sudo apt-get install clang-format-${{ inputs.version }} clang-tidy-${{ inputs.version }}
    then
        # This LLVM script will add the relevant LLVM PPA: https://apt.llvm.org/
        wget https://apt.llvm.org/llvm.sh -O $GITHUB_ACTION_PATH/llvm_install.sh
        chmod +x $GITHUB_ACTION_PATH/llvm_install.sh
        sudo $GITHUB_ACTION_PATH/llvm_install.sh ${{ inputs.version }} all
    fi
# ... if not on linux runner

where the added all arg is subject to my review observation.

[mirenradia]

Now, I hate to ask for more changes because your time is very appreciated. I think we can reduce the steps back into 1:

I'm happy to change it back to 1 step if that's what you want. One argument you might want to consider keeping them as separate steps is that it makes the log output easier to parse.

Let me know what you prefer.

[2bndy5]

One argument you might want to consider keeping them as separate steps is that it makes the log output easier to parse.

Since we switched to a composite action (no more docker), the logs have been a little weird... Each composite step gets its own collapsible "group" of log statements. But our cpp-linter package creates a subset of grouped logs which will never render as intended until

• github supports nested log groups
• github provides an option to control the log groups for a composite action

Either way, I see the human readability of the deps-setup step as an issue with github actions (not entirely within our control).

@shenxianpeng are you ok with the dependency-install steps separated? I don't really have preference either way. I only suggested combining the install steps because that's how it was before.

[shenxianpeng]

I prefer to combine the install dependencies steps into one step as before if possible. it seems more simple and readable. thank you for your time on this.

outdated

[2bndy5]

Thanks for doing this! I'll leave this open to give @shenxianpeng a chance to review it as well, but this LGTM.

The test-run #6 is passing.

[shenxianpeng]

LGTM. thank you!