Skip to content
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

Run s2n tests in GitHub Actions #823

Merged
merged 31 commits into from
Aug 27, 2020
Merged

Run s2n tests in GitHub Actions #823

merged 31 commits into from
Aug 27, 2020

Conversation

chameco
Copy link
Contributor

@chameco chameco commented Aug 24, 2020

This PR updates the GitHub Actions configuration to run the s2n proof tests.

Since reproducing these tests outside of CI has been troublesome in the past, the s2nTests/ directory now contains Docker configurations to make this easier. Running make saw-script in that directory will build a saw executable on Ubuntu 18.04 from the current source tree in s2nTests/bin. After this, running (e.g.) make bike will use that executable to run the BIKE proof.

@chameco
Copy link
Contributor Author

chameco commented Aug 25, 2020

I think everything is working properly at this point - the remaining Actions failure is GHC v8.8.3 on MacOS, which seems to be failing on master as well.

@chameco chameco marked this pull request as ready for review August 25, 2020 19:23
@chameco chameco requested review from hazelweakly and atomb August 25, 2020 19:24
Copy link
Contributor

@andreistefanescu andreistefanescu left a comment

Choose a reason for hiding this comment

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

Looks ok, just the comment about how to split things between docker build and docker run

s2nTests/docker/s2n.dockerfile Show resolved Hide resolved
s2nTests/docker/s2n.dockerfile Outdated Show resolved Hide resolved
s2nTests/docker-compose.yml Outdated Show resolved Hide resolved
@@ -6,20 +6,12 @@ on:

jobs:
build:
runs-on: ${{ matrix.os }}
runs-on: ubuntu-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of duplication between this one and the macos build.

As far as I can tell, the duplication seems to be in order to avoid uploading artifacts on macos that we won't use? If so, it'd be nice to make the upload artifact step conditional so that it's only ran on linux. Code duplication is very error-prone, particularly inside yaml files, so it's good to be able to avoid it when possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Splitting the build job like this was done to ensure that the s2n-tests job runs even if the build-macos job fails. I agree that this is less than ideal; if there's a better way to accomplish this, let's change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The continue-on-error should've made it so that the s2n-tests job would run even if the macos jobs failed. Was that not the case?

Copy link
Contributor Author

@chameco chameco Aug 27, 2020

Choose a reason for hiding this comment

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

My impression was that it was allowing the job to complete (i.e. not immediately killing the Ubuntu builds) but still skipping the dependent job. You can see this here: https://github.com/GaloisInc/saw-script/runs/1022813322. It's very possible that I've misinterpreted this, though.

@hazelweakly
Copy link
Contributor

Left a few comments, but nothing that would stop this from going in. They can always be addressed later. Let's not make perfect the enemy of good when it come to getting more tests into the CI :)

Copy link
Contributor

@andreistefanescu andreistefanescu left a comment

Choose a reason for hiding this comment

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

@chameco @jared-w thank you guys!

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.

3 participants