Skip to content

Cleaned up the CI a little. #38

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Cleaned up the CI a little. #38

wants to merge 4 commits into from

Conversation

wermos
Copy link
Contributor

@wermos wermos commented Jun 15, 2025

When the PR gets a +1 from the reviewers, I will need to rebase this PR to remove the first commit (29018de) so that it can be merged to main. I needed to add the branch to the CI to ensure that my changes weren't breaking anything.

In this PR, I

  • removed the unnecessary "Install Ninja" step. If we look at the specs for the GitHub-hosted Ubuntu 24.04 runner, we can see that Ninja comes preinstalled. (For further information, this change seems to have been made near the beginning of the year.)
  • Removed the spurious rm -rf .build step. The runners only come with the default directories. If we didn't make the .build directory in a previous step, then it cannot exist.
  • Removed the multiple calls to set -x. Running that once is enough. The shell persists across the different steps.
  • Removed the call to cat /etc/lsb_release. It is fine to use that during the development of the CI, but there is no reason to run it every CI run. It just adds to the logs for no reason. We specifically asked for the Ubuntu 24.04 runner, so all the information we might need to know can be found in the specs for the GitHub-hosted Ubuntu 24.04 runner.

Apart from these, I want to also remove the "Install LLVM + Clang" and "Install GCC" steps. Not only would it make the CI script simpler (and therefore reduce the maintenance burden a bit), but also because the current method of doing things makes no sense to me. The Ubuntu 24.04 runner comes with Clang 16-18 and GCC 12-14 preinstalled. So, I don't quite see the value of removing it and then readding it.

For Clang, we can remove the current "Install LLVM+Clang" step and use the Install LLVM and Clang Action, which would greatly simplify the script, reduce maintenance burden, and remove the need to reinvent the wheel.

For GCC, as far as I can tell, there is no reason for the Install GCC step to exist. All the 3 GCC versions we test against are preinstalled in the runner.

I'd be happy to make these changes, but I wanted to get some feedback from the maintainers/community first.

@wermos wermos requested review from a team, inbal2l and wusatosi June 15, 2025 07:32
@neatudarius neatudarius self-assigned this Jun 15, 2025
@wermos
Copy link
Contributor Author

wermos commented Jun 15, 2025

I was rereading my comment and realized that there was a link missing, so I added it. I had forgotten to link to the Install LLVM and Clang Action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants