Skip to content

Conversation

siggmo
Copy link
Collaborator

@siggmo siggmo commented Sep 6, 2024

Closes #9

This workflow defines a job to build and test FANS upon pushes to main and develop as well as for every pull request. It works fine so far, but I think the implementation could be more elegant (see below).

P.S.: CTest test cases are not yet included. I plan to open another PR specifically for the test cases. So far CTest just outputs that it couldn't find any tests and exits with code 0, thus not breaking the workflow execution.

Features

  • Matrix configuation to run job for different Ubuntu versions and CPU architectures
  • Concurrency settings to
    • ensure that running jobs on pushes to main and develop are not cancelled by new pushes due to computing resource constraints
    • prevent race conditions involving shared resources (not relevant in our case I think)
    • prevent unnecessary workflow runs on outdated commits in PRs
  • Upload CMake logs in case of errors in the configure step
  • Upload test logs if a test fails

Issues regarding QEMU emulation and a readable workflow file

To support the linux/arm64 platform I use the QEMU emulation tool. This however seems to not be supported by the jobs.<job_id>.container option described here. I tried passing the --platform linux/... flag, but that caused the container to crash in the case of linux/arm64 when I tried running the workflow on my private repo. That's why I reverted back to use the explicit Docker commands for creating the container and running commands in it. Unfortunately this make the workflow file much less readable. For comparison the old version.

Running actions locally

With nektos/act one can run GitHub Actions locally, which I think is very useful for debugging them. To accomplish this, act sets up an own Docker container emulating a GitHub hosted runner. Unfortunately this seems to not support the explicit Docker commands mentioned above, I guess because that would mean to have nested Docker containers. With the jobs.<job_id>.container option it worked, however act explicitly forbids the usage of the --platform flag, thus making emulation not possible for local runs. If I understood it correctly, act with the container option creates another container next to the runner container and mounts the same volume into both of them to avoid nesting containers.

So in summary I am unsure how to combine the support for arm64, a readable workflow file and the possibility to run the workflow locally. GitHub is planning to release ARM runners for open source projects by the end of the year (source). This would make emulation obsolete and the workflow file more readable by allowing to use the container option again. However this would still not enable one to use QEMU emulation locally with act. But I think with a running workflow that's the least important of the three issues...

If you have any ideas how to solve the above problems please tell me :)

…iner' option

The container option doesn't support QEMU emulation and thus selecting a architecture
different from the runners/host's one crashed the container with a 'exec format error'.

Unfortunately this breaks running the workflows locally using nektos/act. I still have
to figure out why exactly.
@siggmo
Copy link
Collaborator Author

siggmo commented Sep 8, 2024

I did some more research on running the action locally: According to this article, sharing the Docker daemon socket with the runner container is the way to go to emulate Docker in Docker. act already bind mounts the socket into its runner container, it was just that the container user there was not part of the hosts docker group. Adding the option --container-options "--group-add $(stat -c %g /var/run/docker.sock)" to the act call solves that (source).

Using the hosts Docker daemon however means that bind mounts in the action will mount host directories into the created fans-ci container and thus circumvent the intended isolation of the CI workflow. But I think that's still better than nothing when it comes to debugging this workflow locally. One just needs to manually clean up the build directory and delete the created container, as well as make sure that only one of the possible matrix combinations is being executed, e.g.:

act --container-options "--group-add $(stat -c %g /var/run/docker.sock)" --job build --matrix UBUNTU_VERSION:jammy --matrix ARCH:amd64

I'm aware that bind mounting the Docker socket has huge security implications, but since nektos/act seems to be a widely used and accepted tool, I guess it can be trusted. What are your opinions on that?

Regarding the other two points from the post above I guess we just need to wait for GitHub to make the linux/arm64 runners available for open source projects (and hope they have Docker installed).

@IshaanDesai IshaanDesai linked an issue Sep 9, 2024 that may be closed by this pull request
Copy link
Collaborator

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to manually work with Docker inside a GitHub Action. You can simply use a container by stating it in the container parameter. For example, the container precice/precice:nightly is used like this. This is of course applicable if we are pushing images for every change, which we should not do.
I would suggest manually installing FANS inside the Action then. Using Docker over-complicates the whole thing.

@siggmo
Copy link
Collaborator Author

siggmo commented Sep 9, 2024

There is no need to manually work with Docker inside a GitHub Action. You can simply use a container by stating it in the container parameter.

I wanted to use the container: parameter, but unfortunately this parameter doesn't support specifying the platform. As stated here:

But building arm64 container images on Github still requires QEMU

We need Docker to be able to build for linux/arm64.

@IshaanDesai
Copy link
Collaborator

IshaanDesai commented Sep 9, 2024

Is there a reason to specifically target linux/arm64? If things work with a reasonable Linux distribution like Ubuntu, I would take this as a successful test. I would keep the test and Action simple: get the FANS source code, compile manually, and run the tests. Anything platform specific can be done separately.

Quickly asked @sanathkeshav and now I understand why ARM architecture testing is being done. This is to support MacOS. For this we can try container: macos-latest.

@siggmo
Copy link
Collaborator Author

siggmo commented Sep 9, 2024

Is there a reason to specifically target linux/arm64? If things work with a reasonable Linux distribution like Ubuntu, I would take this as a successful test. I would keep the test and Action simple: get the FANS source code, compile manually, and run the tests. Anything platform specific can be done separately.

We wanted to make sure it also runs on @sanathkeshav's MacBook Air which has an ARM CPU and thus cannot run linux/amd64 containers.

But I agree. In the draft ctest PR the QEMU emulated test runs take forever (already >1h), that's not practical... The amd64 ones are reasonably fast (~3min), would you thus agree to keep testing for the three Ubuntu versions? Or should we test for just one version?

@siggmo
Copy link
Collaborator Author

siggmo commented Sep 9, 2024

Quickly asked @sanathkeshav and now I understand why ARM architecture testing is being done. This is to support MacOS. For this we can try container: macos-latest.

Yes exactly. But using macos-latest would mean to adapt the CMake recipe not only to arm64, but also to MacOS which is a lot more effort I think. I suggest to temporarily drop arm64 support and wait for GitHub to make its linux/arm64 runners available.

@sanathkeshav
Copy link
Member

My 2 cents is that the only way FANS works on MacOS so far is via Docker. If the consensus is that supporting ARM is too complicated, We can choose to drop support. This is a very niche case as is (me).

But Is it is easy to do as @IshaanDesai suggests via container:macos-latest , that would be great

@IshaanDesai IshaanDesai added the enhancement New feature or request label Sep 10, 2024
@IshaanDesai IshaanDesai merged commit 6994621 into develop Sep 17, 2024
7 checks passed
@IshaanDesai IshaanDesai deleted the feature/gh-action-build-and-test branch September 17, 2024 11:39
This was referenced Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build and test FANS in a GitHub Action
3 participants