-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Depot runners arm #120786
base: main
Are you sure you want to change the base?
Depot runners arm #120786
Conversation
This depends on #117111 |
- name: Install Ninja | ||
# This doesn't seem to work on Ubuntu any more, so just use apt-get | ||
if: runner.os != 'Linux' | ||
uses: llvm/actions/install-ninja@22e9f909d35b50bd1181709564bfe816eaeaae81 # main | ||
|
||
- name: Install Ninja (Linux) | ||
if: runner.os == 'Linux' | ||
shell: bash | ||
run: | | ||
sudo apt-get update | ||
sudo apt-get -y install ninja-build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this should work still, no?
- name: Install Ninja | |
# This doesn't seem to work on Ubuntu any more, so just use apt-get | |
if: runner.os != 'Linux' | |
uses: llvm/actions/install-ninja@22e9f909d35b50bd1181709564bfe816eaeaae81 # main | |
- name: Install Ninja (Linux) | |
if: runner.os == 'Linux' | |
shell: bash | |
run: | | |
sudo apt-get update | |
sudo apt-get -y install ninja-build | |
- name: Update apt | |
# Needed for the install-ninja action to work on Ubuntu for some reason. | |
if: runner.os == 'Linux' | |
run: sudo apt-get update | |
- name: Install Ninja | |
uses: llvm/actions/install-ninja@22e9f909d35b50bd1181709564bfe816eaeaae81 # main |
echo ccache=sccache >> $GITHUB_OUTPUT | ||
fi | ||
if [ "${{ runner.os }}" = "Linux" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if [ "${{ runner.os }}" = "Linux" ]; then | |
if [ "$RUNNER_OS" = "Linux" ]; then |
We don't need workflow interpolation here, and it's better to avoid it if we don't need it: https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-an-intermediate-environment-variable
In general I'd prefer to remove all workflow interpolation (even in instances where it is safe) to make auditing easier, but that would require plenty of changes across all workflows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a patch to clean up the file: #120860
I'll update this patch with your suggestions too.
if: github.repository == 'llvm/llvm-project' | ||
runs-on: ${{ inputs.runs-on }} | ||
if: >- | ||
github.repository == 'llvm/llvm-project' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github.repository == 'llvm/llvm-project' | |
github.repository_owner == 'llvm' |
Might make it easier to reuse workflow snippets in other org repos, but this is a very mild preference.
No description provided.