[#382] Add build action for PRs #370
Conversation
We probably want main to clean build?
This is taken from the GH docs. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows#force-deleting-cache-entries Note: This seems to just speed up cache eviction.
Caches are cleaned automatically.
|
After thinking about this a little more, making sure the project compiles correctly is more important than the amount of time taken to build it. Let's remove/disable ccache for now. |
|
Do we also want this to run on main, or just PRs? |
|
On PRs is good. See irods/irods for reference. |
|
I'll take out cache and put in a separate draft so we won't forget about it |
In that case, only thing to do is remove cache. Already only acts on PRs. |
| steps: | ||
| - name: Checkout Repository | ||
| uses: actions/checkout@v4 | ||
| - name: Install Misc |
There was a problem hiding this comment.
Let's use the same wording as in irods/irods for this.
Install Prerequisites
There was a problem hiding this comment.
Avoided that name initially since I wasn't sure what exactly are the prerequisites.
| @@ -0,0 +1,54 @@ | |||
| name: iRODS HTTP API Builder | |||
There was a problem hiding this comment.
Looking at irods/irods, how do you feel about changing this to one of the following?
Build-Ubuntubuild-irods-http-api-ubuntu
Then we could expand this to cover other versions of Ubuntu, and later other platforms such as EL8 and EL9.
There was a problem hiding this comment.
I think it would be better without the hyphens. This name is supposed to be a user-facing name. You can see this in the passed checks at the bottom.
Perhaps we can name it so it ends up as Ubuntu iRODS HTTP API Builder / Build iRODS HTTP API - <Specific Ubuntu Image Here>?
There was a problem hiding this comment.
Sure. I don't think hyphens make it less readable.
Try expanding the builder workflow to support ub22/24, EL8/9 and see how the github actions report it. We want things to be sorted, grouped, and organized.
Aim for consistency across repos.
Take a look at the workflows for irods/irods.
There was a problem hiding this comment.
Try expanding the builder workflow to support ub22/24, EL8/9 and see how the github actions report it. We want things to be sorted, grouped, and organized.
Expanded Ubuntu side.
There was a problem hiding this comment.
Good.
Please add one for EL8 and EL9.
Perhaps we can name it so it ends up as
Ubuntu iRODS HTTP API Builder / Build iRODS HTTP API - <Specific Ubuntu Image Here>?
Remove the name of the project (i.e. iRODS HTTP API) and from the workflow file. That's implied due to the repo.
The entries in the workflow results are not sorted for some reason, weird. Anyway, change it so that the entries produce something like the following:
Build Package / Ubuntu 20.04
Build Package / Ubuntu 22.04
Build Package / Ubuntu 24.04
Build Package / Enterprise Linux 8
Build Package / Enterprise Linux 9
That aligns with the style of the clang-format/tidy entries.
I like the inclusion of iRODS 4.3.x in the entries, but it implies each release of iRODS will have an entry unless we come up with a policy for when a dev package should be added/removed. I don't think that's necessary so let's stick with building against the minimum version of the iRODS dev package (i.e. 4.3.2). That aligns with the dev package used to produce the Docker images and it covers backwards compatibility.
Thoughts?
There was a problem hiding this comment.
I like the inclusion of iRODS 4.3.x in the entries, but it implies each release of iRODS will have an entry
Yes, each release would have an entry since we state a minimum only.
unless we come up with a policy for when a dev package should be added/removed.
I'm not sure if I follow, are you asking what if we have a change of requirements or a breaking change?
If our requirements change, then our tests would match that too, whenever the build container changes, right?
In terms of iRODS version, isn't this just when our README or build container changes?
I don't think that's necessary so let's stick with building against the minimum version of the iRODS dev package (i.e. 4.3.2). That aligns with the dev package used to produce the Docker images and it covers backwards compatibility.
We should test the minimum, but I think it would be nice to make sure there are no problems compiling with a more recent version (and eventually test against this too). There's also the concern of accidental forward "departures from interface" or breakages.
There was a problem hiding this comment.
I'm not sure if I follow, are you asking what if we have a change of requirements or a breaking change?
If our requirements change, then our tests would match that too, whenever the build container changes, right?
In terms of iRODS version, isn't this just when our README or build container changes?
Consider the case where iRODS 4.3.7 is released and 4.3.2 is still the minimum version required for compiling the software.
While it would be nice to build against all versions, it's probably unrealistic to do that in GitHub due to potential restrictions/constraints. We'd need to define rules (a policy) for picking which versions to build against, else people may wonder why we build against a subset of the versions instead of all of them.
We should test the minimum, but I think it would be nice to make sure there are no problems compiling with a more recent version (and eventually test against this too). There's also the concern of accidental forward "departures from interface" or breakages.
I agree. That's something that can be done by the k8s testing environment.
Keep in mind we'll continue to iterate on this. This is a good first step.
There was a problem hiding this comment.
In Discuss:
We will implement the other non Ubuntu images in a separate PR.
|
Also, needs an issue number. Seems good overall |
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [ ubuntu-20.04, ubuntu-22.04, ubuntu-24.04 ] |
There was a problem hiding this comment.
These entries are not the same as what's used in irods/irods.
Are these container names? irods/irods uses ubuntu:20.04.
Is there a difference between ubuntu-20.04 and ubuntu:20.04?
Where is the container property? Is that deprecated now?
There was a problem hiding this comment.
Are these container names?
The names come from: https://github.com/actions/runner-images
Where is the
containerproperty? Is that deprecated now?
container is not deprecated.
Using irods/irods as an example:
https://github.com/irods/irods/blob/a6d646c37423f4e8fc25b21d4e15c2025eaed984/.github/workflows/build-irods-ubuntu.yml#L6-L10
Here, I am changing runs-on, the github action runner, to then build on the runner.
In irods/irods, the same runner is used, but container is set, so a container is created in the runner that then runs the code.
Not sure if this is what you meant, but just in case, the particular name under matrix does not matter. os could be labeled images-to-build-and-test-on for example.
Is there a difference between ubuntu-20.04 and ubuntu:20.04?
The runner image does have more on it than the base Ubuntu image, to allow for the running of various actions (e.g. ccache action).
There was a problem hiding this comment.
Got it.
So why do something different than the irods/irods repo?
Is what you're doing the recommended way of doing things?
I'm just curious.
There was a problem hiding this comment.
In a discuss:
We will use the container images instead of the runner images.
Reasoning is to be consistent with other projects.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [ ubuntu-20.04, ubuntu-22.04, ubuntu-24.04 ] |
There was a problem hiding this comment.
Might have to remove Ubuntu-20.04, or it grows its own file if there's a fix for this:
$ sudo apt-get update -qq
$ sudo apt-get install -qq \
> apt-transport-https \
> git \
> gnupg \
> lsb-release \
> wget \
> g++-11 \
> gcc-11 \
> libcurl4-gnutls-dev \
> libssl-dev \
> libssl3 \
> ninja-build
E: Unable to locate package gcc-11
E: Package 'libssl3' has no installation candidateThere was a problem hiding this comment.
Could try using uname or /etc/os-release to sniff the OS version and install different packages based on the results. That's if you want to keep it to one file.
Creating a new file is fine too, assuming GitHub won't complain about duplicate workflow names.
There was a problem hiding this comment.
In Discuss:
We will remove Ubuntu 20.04.
MartinFlores751
changed the title
Ccache can be removed if we want a clean build every run.