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

ci(nix): Setup a head build for the cross_compatibility integ test #4567

Merged
merged 22 commits into from
Jul 26, 2024

Conversation

dougch
Copy link
Contributor

@dougch dougch commented May 23, 2024

Resolved issues:

Resolves #3841

Description of changes:

Modify the script checking out s2n-tls head and building s2nc_head/s2nd_head for use in the cross_compatibility integration test, in a nix devshell.

An ad-hoc CodeBuild test job with only cross_compatibility

Call-outs:

There might be a more idiomatic nix way to do this.

For the kTLS tests, building twice puts us over our goal of 1 hour CI runs, so an env. flag was added to disable these builds: S2N_NO_HEADBUILD.

Note: because the devShell contains the CMAKE_* environment variables, we don't need -DCMAKE_PREFIX_PATH and the s2nc/d built from head will use the same libcrypto as the regular build.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? locally/CI

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label May 23, 2024
@dougch dougch added the type/nix related to nix label May 23, 2024
@dougch
Copy link
Contributor Author

dougch commented May 24, 2024

Strange git clone failure fatal: unable to access 'https://github.com/aws/s2n-tls/': error:16000069:STORE routines::unregistered scheme

@dougch dougch marked this pull request as ready for review May 29, 2024 21:31
@dougch dougch requested review from lrstewart and maddeleine May 29, 2024 21:31
@dougch dougch marked this pull request as draft June 3, 2024 22:08
@dougch dougch marked this pull request as ready for review June 10, 2024 16:08
Comment on lines 53 to 54
echo "s2nc_head already exists; not rebuilding s2n_head"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this work locally / with nix? Like, I'm imagining that I'm developing a feature that requires me to run the cross_compatibility test. s2n_head could be quite old if I haven't run clean lately, and my understanding of this behavior is that I wouldn't really have any visibility into that beyond this one stdout message in the big block of build stdout messages.

Maybe we don't have to build every time, but should we have some sort of freshness check? Checking the version against github might be a bit too much-- Maybe as simple as the creation time for s2nc_head? At the bare minimum I'd try to add some visibility to this message, maybe noting which commit is in s2nc_head, how to force s2nc_head to rebuild, maybe some color.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had started down the path of checking s2n_head's commit, but thought, at least initially, it's a waste of time since the CI will always be running on a clean instance. Checking the age of the last commit in s2n_head might be lighter weight, let me try a few things.

dougch and others added 2 commits June 11, 2024 14:49
@dougch dougch requested a review from lrstewart June 19, 2024 21:53
@dougch dougch enabled auto-merge July 17, 2024 19:26
auto-merge was automatically disabled July 26, 2024 01:55

Merge commits are not allowed on this repository

@dougch dougch merged commit 3c0dfee into aws:main Jul 26, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s2n-core team type/nix related to nix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nix Improvements
3 participants