Run integv2 tests with nix #3824

harrisonkaiser · 2023-02-11T09:06:41Z

Resolved issues:

This PR adds the ability run a devShell using nix develop.

That devShell gives you access to all the packages you need installed to run most of the integration tests, unit tests, and even clang-format the repo:

$ nix develop
...
[in-dev-shell] $ configure
<runs-cmake-configure-stage>...

[in-dev-shell] $ build
<runs-cmake-build-stage>...

[in-dev-shell] $ integ happy
<runs-happy-path-integ-test>...

[in-dev-shell] $ unit
<runs-all-unit-tests>...

[in-dev-shell] $ check-clang-format
<running clang-format checks>...

[in-dev-shell] $ do-clang-format
<running clang-format in place>...

Description of changes:

This PR adds many derivations.

Call-outs:

This is only tested on x86-linux. See: #3841

Testing:

Locally -- lots of building and rebuilding. Good thing there is a cache.

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

This commit adds several dependacies to facilitate development.

nix/openssl_1_0_2_fips.nix

nix/gnutls.nix

flake.nix

camshaft · 2023-02-21T00:10:32Z

flake.nix

+            # openssl_0_9_8 openssl_1_0_2 openssl_3_0
+            # libressl pkgs.boringssl


Is it possible to have multiple dev shells where you can select these?

We do that for build, but it doesn't make as much sense here. Often if you are debugging something with openssl you'll want access to all of them. I do think that it is a little silly to have them all commented out here though. Instead I propose putting them all in the environment and creating alias so you can run all the openssls.

@dougch brought up a good point: adding all the libcryptos here makes it difficult to build different versions of the library in the devShell. We might need to make different devShells for that reason alone.

flake.nix

CMakeLists.txt

…ls into cmake+flake-integv2

dougch

Good stuff. Still on the fence about including all of the various libcrypto's in the default devShell, but it does make for less work to flip between them this way. Local testing (minus offline discussion) worked on x86.

dougch · 2023-02-22T00:15:52Z

flake.nix

+          '';
+
+          checkPhase = ''
+            echo Not running tests here. Run `nix develop` to run tests.


Is this a common pattern? The checkPhase seems meant for unit tests?

I found some evidence online that the checkPhase is for flame tests. Like "is the binary actually there", "does it execute" ect... The problem with tests is that they add non-determinism in the build. The advice I got was to either 1) build the tests and install them or 2) just use the devShell. There isn't really a way to "build" and "install" the Python tests so I figured I'd cater to the developer and make it easy to build/run the tests.

There is also a non-nix reason not to build the tests in the main build. If we want to run the unit tests we have to build them. Building them means we aren't able to take advantage of LTO optimization. This PR is mostly focused on the dev shell, but one area for improvement is to make the build here match the release build as exactly as possible. See 4. on #3841

.gitignore

nix/amazon-corretto-8.nix

camshaft · 2023-02-23T18:06:39Z

nix/shell.sh

+
+function build {
+    banner "Running Build"
+    javac tests/integrationv2/bin/SSLSocketClient.java


This should really be added as a target in the cmake integ test section. I'm assuming that's possible?

This simply reflects what the codebuild bin script does already. Moving the Java dependency into CMakeLists.txt is a goal but to do that idiomatically requires the use of ctests fixtures which requires at least cmake 3.7 (see here). Our minimum required cmake version is 3.0. One alternative to using fixtures is to create a script that cmake invokes (the script could handle the java compile, as well as apache setup, for the appropriate test cases).

I think this change is somewhat involved and should go in its own PR.

camshaft · 2023-02-23T18:30:15Z

nix/shell.sh

+    cmake -S . -B./build \
+          -DBUILD_TESTING=ON \
+          -DS2N_INTEG_TESTS=ON \
+          -DS2N_INSTALL_S2NC_S2ND=ON \
+          -DS2N_FAST_INTEG_TESTS=ON \
+          -DBUILD_SHARED_LIBS=ON \
+          -DCMAKE_BUILD_TYPE=RelWithDebInfo


This is fine for now but it's a bit limiting. I'm not sure if there's a better way to expose the matrix of options one might want to test, though.

My intention here is to be an opinionated default. If you want to build with your own options you can invoke CMake directly. Perhaps there is a limited set of default options that invoke CMake in a few different ways. But I don't think that exposing the whole matrix of options makes sense, CMake already does that and you can use it directly for that purpose.

…flake-integv2

harrisonkaiser · 2023-02-24T21:53:12Z

Waiting a re-approval from @dougch.

dougch

We'll iterate some on this in a future PR.

harrisonkaiser · 2023-02-27T17:40:56Z

We'll iterate some on this in a future PR.

#3841

github-actions bot added the s2n-core team label Feb 11, 2023

harrisonkaiser force-pushed the cmake+flake-integv2 branch from 71f5316 to fcf102e Compare February 20, 2023 14:55

Add dev shell to flake

56b8459

This commit adds several dependacies to facilitate development.

harrisonkaiser force-pushed the cmake+flake-integv2 branch from 7cdcc2c to 56b8459 Compare February 20, 2023 16:17

harrisonkaiser marked this pull request as ready for review February 20, 2023 16:19

harrisonkaiser requested review from dougch and camshaft February 20, 2023 16:19

Gate s2nc, s2nd

174feea

harrisonkaiser mentioned this pull request Feb 20, 2023

Nix Improvements #3841

Closed

14 tasks

uwaces and others added 3 commits February 20, 2023 11:58

Cleanup

b43003b

nix fmt

c50921a

Merge branch 'main' into cmake+flake-integv2

3356424

camshaft reviewed Feb 21, 2023

View reviewed changes

uwaces and others added 2 commits February 21, 2023 15:37

Respond to comments

5a212fe

Merge branch 'main' into cmake+flake-integv2

cf15592

harrisonkaiser requested a review from camshaft February 21, 2023 20:40

uwaces and others added 3 commits February 21, 2023 18:18

Update the nix/shell with corrected cmake flag

f1b0ebd

Merge branch 'cmake+flake-integv2' of github.com:harrisonkaiser/s2n-t…

062041c

…ls into cmake+flake-integv2

Merge branch 'main' into cmake+flake-integv2

196fe03

harrisonkaiser mentioned this pull request Feb 21, 2023

Rename a nixpkgs variabe; additional comments. #3835

Closed

dougch mentioned this pull request Feb 21, 2023

ci: nix action #3834

Merged

dougch approved these changes Feb 22, 2023

View reviewed changes

dougch self-requested a review February 22, 2023 22:49

Merge branch 'main' into cmake+flake-integv2

69403ce

camshaft reviewed Feb 23, 2023

View reviewed changes

uwaces added 3 commits February 24, 2023 11:15

Respond to comments

c94da4c

Merge remote-tracking branch 'origin/cmake+flake-integv2' into cmake+…

5c12f75

…flake-integv2

Merge branch 'main' into cmake+flake-integv2

43f9550

harrisonkaiser requested a review from camshaft February 24, 2023 16:38

camshaft approved these changes Feb 24, 2023

View reviewed changes

harrisonkaiser added 2 commits February 24, 2023 14:28

Merge branch 'main' into cmake+flake-integv2

a9a63ce

Merge branch 'main' into cmake+flake-integv2

765761b

dougch approved these changes Feb 27, 2023

View reviewed changes

harrisonkaiser merged commit 6d4eb1f into aws:main Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run integv2 tests with nix #3824

Run integv2 tests with nix #3824

harrisonkaiser commented Feb 11, 2023 •

edited

Loading

camshaft Feb 21, 2023

harrisonkaiser Feb 21, 2023

harrisonkaiser Feb 24, 2023

dougch left a comment

dougch Feb 22, 2023

harrisonkaiser Feb 22, 2023 •

edited

Loading

camshaft Feb 23, 2023

harrisonkaiser Feb 24, 2023

camshaft Feb 23, 2023

harrisonkaiser Feb 24, 2023 •

edited

Loading

harrisonkaiser commented Feb 24, 2023

dougch left a comment

harrisonkaiser commented Feb 27, 2023

		# openssl_0_9_8 openssl_1_0_2 openssl_3_0
		# libressl pkgs.boringssl

Run integv2 tests with nix #3824

Run integv2 tests with nix #3824

Conversation

harrisonkaiser commented Feb 11, 2023 • edited Loading

Resolved issues:

Description of changes:

Call-outs:

Testing:

camshaft Feb 21, 2023

Choose a reason for hiding this comment

harrisonkaiser Feb 21, 2023

Choose a reason for hiding this comment

harrisonkaiser Feb 24, 2023

Choose a reason for hiding this comment

dougch left a comment

Choose a reason for hiding this comment

dougch Feb 22, 2023

Choose a reason for hiding this comment

harrisonkaiser Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

camshaft Feb 23, 2023

Choose a reason for hiding this comment

harrisonkaiser Feb 24, 2023

Choose a reason for hiding this comment

camshaft Feb 23, 2023

Choose a reason for hiding this comment

harrisonkaiser Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

harrisonkaiser commented Feb 24, 2023

dougch left a comment

Choose a reason for hiding this comment

harrisonkaiser commented Feb 27, 2023

harrisonkaiser commented Feb 11, 2023 •

edited

Loading

harrisonkaiser Feb 22, 2023 •

edited

Loading

harrisonkaiser Feb 24, 2023 •

edited

Loading