-
Notifications
You must be signed in to change notification settings - Fork 11
Add cmake "run" target and use it in PR CI #53
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
Conversation
67580ed to
e693b26
Compare
jogme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thank you!
|
Interesting that the only Windows on master didn't pass. |
| endforeach() | ||
| set(cmds ${new_cmds}) | ||
| endif() | ||
| endforeach() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried it but make run does not respect or set (DY)LD_LIBRARY_PATH which should be fixed.
I would probably write custom target manually for each binary. The cmd options are much too different for a generic solution.
$ echo $DYLD_LIBRARY_PATH
/Users/npajkovsky/openssl/openssl
$ ll /Users/npajkovsky/openssl/openssl | grep ssl.4
-rwxr-xr-x 1 npajkovsky staff 960K Oct 17 10:21 libssl.4.dylib
$ make run
[ 2%] Built target perf
[ 3%] Built target x509storeissuer
[ 3%] Run x509storeissuer -t /Users/npajkovsky/openssl/openssl/test/certs 4
dyld[58574]: Library not loaded: /usr/local/lib/libssl.4.dylib
Referenced from: <7C191CC0-3566-3B84-9EB4-9AB01FD44703> /Users/npajkovsky/openssl/perftools/source/x509storeissuer
Reason: tried: '/usr/local/lib/libssl.4.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/usr/local/lib/libssl.4.dylib' (no such file), '/usr/local/lib/libssl.4.dylib' (no such file)
make[3]: *** [CMakeFiles/run-x509storeissuer--t--U_sers-npajkovsky-openssl-openssl-test-certs-4] Abort trap: 6
make[2]: *** [CMakeFiles/run-x509storeissuer--t--U_sers-npajkovsky-openssl-openssl-test-certs-4.dir/all] Error 2
make[1]: *** [CMakeFiles/run.dir/rule] Error 2
make: *** [run] Error 2There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you exported the variable? But that's a valid point, that cmake doesn't do any provisions to supply the provided OPENSSL_ROOT_DIR to the executables being run, the relevant overrides should be added to the CI recipes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine on OpenBSD where I run cmake as follows:
cmake -S . -B ./build -DOPENSSL_CONFIG_MODE=1 -DCMAKE_PREFIX_PATH=/path/to/openssl.install
cmake --build build
cmake --build build -t run
The build is setup such linker adds RUNPATH to elf header. The RUNPATH is set to /path/to/openssl.install I will take a look at macos.
In my case the failure happens later due to missing/wrong path to testcert dirs.
[ 42%] Built target run-evp-setpeer--k-ec256--t-4
[ 42%] Built target handshake
[ 42%] Run handshake -P -S 1048576 -t /test/certs 1
40C0882EE20C0000:error:80000002:system library:file_ctrl:No such file or directory:crypto/bio/bss_file.c:288:calling fopen(/test/certs/servercert.pem, r)
40C0882EE20C0000:error:10080002:BIO routines:file_ctrl:system lib:crypto/bio/bss_file.c:291:
40C0882EE20C0000:error:0A080002:SSL routines:SSL_CTX_use_certificate_file:system lib:ssl/ssl_rsa.c:329:
/home/sashan/work.openssl/perftools/source/handshake.c:196: Failed to create SSL_CTX pair
My understanding is it is it expects CMAKE_PREFIX_PATH pointing to openssl sources but it got the path to custom installation. If I fix the CMAKE_PREFIX_PATH so it points to sources where custom openssl got installed from I get a different failure:
lifty$ make run
[ 2%] Built target perf
[ 3%] Built target writeread
[ 3%] Run writeread -d -b 4096 /test/certs 4
Failed to create SSL_CTX pair
*** Error 1 in . (CMakeFiles/run-writeread--d--b-4096--test-certs-4.dir/build.make:71 'CMakeFiles/run-writeread--d--b-4096--test-certs-4': /...)
*** Error 2 in . (CMakeFiles/Makefile2:9133 'CMakeFiles/run-writeread--d--b-4096--test-certs-4.dir/all': make -s -f CMakeFiles/run-writeread...)
*** Error 2 in . (CMakeFiles/Makefile2:1125 'CMakeFiles/run.dir/rule': make -s -f CMakeFiles/Makefile2 CMakeFiles/run.dir/all)
*** Error 2 in /home/sashan/work.openssl/perftools/source/build (Makefile:306 'run': make -s -f CMakeFiles/Makefile2 run)
I understand the motivation to improve the CI experience. though I'm bit worried this makes the CMakeLists.txt bit too fancy to my taste. It was meant to be simple....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in ci runner it looks like the tool is doing the right thing. you may need to set DYLD_LIBRARY_PATH env. var explicitly in your shell when running cmake --build ... command, this may help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of local runs with CMAKE_PREFIX_PATH being used, one can explicitly set run_certdir, cmake -S . -B ./build -DOPENSSL_CONFIG_MODE=1 -DCMAKE_PREFIX_PATH=/path/to/openssl.install -Drun_certdir=/path/to/test/certs or so.
Sashan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea to improve CI tests. I just need more time to get familiar with cmake constructs which are used.
| endforeach() | ||
| set(cmds ${new_cmds}) | ||
| endif() | ||
| endforeach() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine on OpenBSD where I run cmake as follows:
cmake -S . -B ./build -DOPENSSL_CONFIG_MODE=1 -DCMAKE_PREFIX_PATH=/path/to/openssl.install
cmake --build build
cmake --build build -t run
The build is setup such linker adds RUNPATH to elf header. The RUNPATH is set to /path/to/openssl.install I will take a look at macos.
In my case the failure happens later due to missing/wrong path to testcert dirs.
[ 42%] Built target run-evp-setpeer--k-ec256--t-4
[ 42%] Built target handshake
[ 42%] Run handshake -P -S 1048576 -t /test/certs 1
40C0882EE20C0000:error:80000002:system library:file_ctrl:No such file or directory:crypto/bio/bss_file.c:288:calling fopen(/test/certs/servercert.pem, r)
40C0882EE20C0000:error:10080002:BIO routines:file_ctrl:system lib:crypto/bio/bss_file.c:291:
40C0882EE20C0000:error:0A080002:SSL routines:SSL_CTX_use_certificate_file:system lib:ssl/ssl_rsa.c:329:
/home/sashan/work.openssl/perftools/source/handshake.c:196: Failed to create SSL_CTX pair
My understanding is it is it expects CMAKE_PREFIX_PATH pointing to openssl sources but it got the path to custom installation. If I fix the CMAKE_PREFIX_PATH so it points to sources where custom openssl got installed from I get a different failure:
lifty$ make run
[ 2%] Built target perf
[ 3%] Built target writeread
[ 3%] Run writeread -d -b 4096 /test/certs 4
Failed to create SSL_CTX pair
*** Error 1 in . (CMakeFiles/run-writeread--d--b-4096--test-certs-4.dir/build.make:71 'CMakeFiles/run-writeread--d--b-4096--test-certs-4': /...)
*** Error 2 in . (CMakeFiles/Makefile2:9133 'CMakeFiles/run-writeread--d--b-4096--test-certs-4.dir/all': make -s -f CMakeFiles/run-writeread...)
*** Error 2 in . (CMakeFiles/Makefile2:1125 'CMakeFiles/run.dir/rule': make -s -f CMakeFiles/Makefile2 CMakeFiles/run.dir/all)
*** Error 2 in /home/sashan/work.openssl/perftools/source/build (Makefile:306 'run': make -s -f CMakeFiles/Makefile2 run)
I understand the motivation to improve the CI experience. though I'm bit worried this makes the CMakeLists.txt bit too fancy to my taste. It was meant to be simple....
a2d538e to
bbddd22
Compare
Sashan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this useful. I let @esyr to polish the code (if needed) and re-submit for review once he will be happy with the code. thanks.
e512c45 to
f1b9dc1
Compare
A helper function that prints version information, so far the version of the OpenSSL library a program is run with. Signed-off-by: Eugene Syromiatnikov <[email protected]>
When a test program is called with "-V" option, it prints version information and exits. Signed-off-by: Eugene Syromiatnikov <[email protected]>
Signed-off-by: Eugene Syromiatnikov <[email protected]>
Generate run-version-<test> target for each test from run_tests, make it a dependency of run-version target. and, if run_add_version_dep vesioble is set to ON (the default), set "run-version" as a dependency of the "run" target. Signed-off-by: Eugene Syromiatnikov <[email protected]>
…tage To increase readability and catch incorrect linking issues early in the run. Signed-off-by: Eugene Syromiatnikov <[email protected]>
| @@ -0,0 +1,18 @@ | |||
| include(GetPrerequisites) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm missing something... why do we need this? I would expect cmake's file copy as I prefer to keep things simple. thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a CMake module[1] for getting a list of DLLs required by an executable, so they can be copied in the executable's directory. I tried cmake -E copy_if_different, but was keeping hitting issues on concurrent runs, so opted for copy_configuration instead, as it first copied a file into a temporary path next to destination, does the comparison, and then a (supposedly atomic) rename (and copy_if_different seems to do comparison and then copy).
[1] https://cmake.org/cmake/help/latest/module/GetPrerequisites.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks for clarification. IMO doing copy $OPENSSL_SRC\*.dll $CMAKE_CURRENT_BINARY_DIR like copy all ddls from OPENSSL directory to perftools build dir. The reason I prefer doing simple/sufficient thing over taking the right/correct approach is we will have to pay attention to cmake version more closely as the current code here uses deprecated API. Starting cmake 3.16 one should be using file(GET_RUNTIME_DEPENDENCIES).
anyway. I think this should go in as-is. so we can get move on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecated since version 3.16: Use file(GET_RUNTIME_DEPENDENCIES) instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we can even use $<TARGET_RUNTIME_DLLS>[1] (3.21+), but due to cmake_minimum_required(VERSION 3.10) we can't have nice things (while supporting pre-3.16/3.21 CMake, anyway).
Sashan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good enough to go in.
| }, { | ||
| openssl-branch: "master", | ||
| cmakeopts: '', | ||
| configopts: 'no-apps no-tests', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason that 3.{5,6} and master skips -D "run_newrawkey_algos=newrawkey;-a;x25519" -D "run_evp_fetch_pqs=evp_fetch;;" -D "run_handshake_pool_size=handshake;;;-o 4"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are overrides for PQ-including defaults. I'm now thinking that those can just as well be moved to CMakeLists, at the expense of making it a bit more magical.
| @@ -0,0 +1,18 @@ | |||
| include(GetPrerequisites) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecated since version 3.16: Use file(GET_RUNTIME_DEPENDENCIES) instead.
| - "openssl-3.4" | ||
| - "openssl-3.5" | ||
| - "master" | ||
| release: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why we need this? It prolongs the CI pipeline extensively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be no other place where the tests are actually being run with various options. I plan to add universal support for the -T timeout option and run (the majority of) the test matrix with it, to alleviate the issue somewhat, but didn't want to pile on onto the existing patch set too much.
Introduce a run target, that executes all the test with different options. For now is used only for minimal sanity checking, that are test can be run and don't fail, but later can be extended to check the results for sanity as well, and the design has some provisions put in place to be useful for test data collection as well, for example. Reviewed-by: Neil Horman <[email protected]> (Merged from #53)
|
change is in via 3c994d3, thanks. |
Introduce a
runtarget, that executes all the test with different options. For now is used only for minimal sanity checking, that are test can be run and don't fail, but later can be extended to check the results for sanity as well, and the design has some provisions put in place to be useful for test data collection as well, for example.