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

change from ninja and custom cmake target run_test to using cmake & ctest. #1700

Closed
wants to merge 1 commit into from

Conversation

Martyrshot
Copy link
Member

@Martyrshot Martyrshot commented Feb 17, 2024

change from ninja and custom cmake target run_test to using cmake & ctest.

Upon reading issue #1494 I figured I'd give this a go.
This PR aims to remove the need for ninja, and make tests platform agnostic.

The new build process will work as follows:

cmake -B build
cmake --build build --parallel
ctest --test-dir build/tests --output-on-failure

Since the run_tests rule was removed, and cmake doesn't have a way to skip tests by default,
the OQS_ENABLE_LONG_TESTS variable is introduced. If you would like to run the long tests
(currently only test_kat_all) you would do the following:

cmake -B build -DOQS_ENABLE_LONG_TESTS=on
cmake --build build --parallel
ctest --test-dir build/tests --output-on-failure


On macOS, using a package manager of your choice (we've picked Homebrew):

brew install cmake ninja [email protected] wget doxygen graphviz astyle valgrind
brew install cmake [email protected] wget doxygen graphviz astyle valgrind
Copy link
Member

Choose a reason for hiding this comment

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

Yikes -- we still suggest using OpenSSL1.1??

Copy link
Member Author

Choose a reason for hiding this comment

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

oof. Good catch. I'll make another pr updating that to openssl@3.

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks @Martyrshot for this improvement. I like the idea -- but I think quite a bit of our CI machinery depends on the (now) "old" way of selecting tests: That'd surely needs to be changed over, too.

… ctest.

Upon reading issue #1494 I figured I'd give this a go.
This PR aims to remove the need for ninja, and make tests platform agnostic.

The new build process will work as follows:
```
cmake -B build
cmake --build build --parallel
ctest --test-dir build/tests --output-on-failure
```

Since the `run_tests` rule was removed, and cmake doesn't have a way to skip tests by default,
the `OQS_ENABLE_LONG_TESTS` variable is introduced. If you would like to run the long tests
(currently only `test_kat_all`) you would do the following:
```
cmake -B build -DOQS_ENABLE_LONG_TESTS=on
cmake --build build --parallel
ctest --test-dir build/tests --output-on-failure
```
@Martyrshot
Copy link
Member Author

Looking at the liboqs ci (I haven't looked at downstream projects) the majority of ci calls pytest directly.

I was trying to avoid having to re-write all the tests in order to remove pytest, so as a nice side benefit ci should work. In a perfect world we would switch from pytest so everything is using a single testing platform... But I wanted to get initial feedback before I committed to that whole mess. :-)

@Martyrshot Martyrshot changed the title change from using ninja and custom cmake target run_test to using cmake & ctest. change from ninja and custom cmake target run_test to using cmake & ctest. Feb 17, 2024
@baentsch
Copy link
Member

But I wanted to get initial feedback before I committed to that whole mess. :-)

My support (for resolving that mess :-) you have.

FWIW, the reason the latest oqs-provider release test fails is a combination of pytest fixture inflexibility (or misunderstanding of "that mess" on my side) and openssl crashing when dealing with too many KEXs :-(

@Martyrshot
Copy link
Member Author

Is the inflexibility you're referring to being that it tests all of the algorithms? Or are the other issues that I can look at addressing when I remove pytest?

@baentsch
Copy link
Member

baentsch commented Feb 17, 2024

Is the inflexibility you're referring to being that it tests all of the algorithms?

Yes and No: The "inflexibility" I mean is that the test fixture for server startup only parameterizes on the sigalgs (not on the KEMs) and thus has to enable all KEMs to prepare/allow for the subsequent (all-)KEM tests (against each sigalg test server). This is conceptually fine and logical (why create different servers for different KEMs?). Unfortunately, openssl obviously cannot handle more than 44 default groups without crashing (still need to understand why and when) -- and oqsprovider now has 52...

Add/edit: All that said, it just dawns on me that I can use OpenSSL's config mechanism (via env var) activating only the KEM the client wants to test... But that then requires each test to fire up both server and client for the SIG/KEM combination under test (right now, I'd hope only count(SIG) openssl s_server instances get started -- but then again, I don't really understand the pytest mechanism :-(

@Martyrshot
Copy link
Member Author

Ah, ok. I understand. Well, I'll see what I can do.

@Martyrshot
Copy link
Member Author

Add/edit: All that said, it just dawns on me that I can use OpenSSL's config mechanism (via env var) activating only the KEM the client wants to test... But that then requires each test to fire up both server and client for the SIG/KEM combination under test (right now, I'd hope only count(SIG) openssl s_server instances get started -- but then again, I don't really understand the pytest mechanism :-(

IIRC you can specify multiple algorithms at once. So, although not perfect, you could minimize the number of server/client spin ups to COUNT(algs)/44 ~= 2.

Anyway, I'm going to focus on removing pytest from liboqs. This will probably turn into converting the pytest scripts into vanilla python scripts and having ctest call them. I feel like this is more portable than writing bash scripts, but also far less painful then converting them to c binaries that need to be compiled, and that exec the test binaries with the right arguments...

@Martyrshot
Copy link
Member Author

Current tasks/conversations to have after yesterday's call:

Investigate if we can still generate vscode projects with out build system. If not, do we care?

  • I will look into this and report back later this week/early next week.

Do we want to keep pytest as the underlying test framework and just have cmake sit on top of it?

  • In the meeting @dstebila showed support for keeping pytest for now. At the very least we can do the change incrementally. @baentsch's comments seem to prefer removing pytest at the same time as switching to cmake. I am open to either, but I think converting from pytest is going to be a decently large task on its own, so it might be worth doing in a separate PR.

@baentsch
Copy link
Member

converting from pytest is going to be a decently large task on its own

It definitely is. Particularly if the goal is to obtain the same level of performance/parallelization as afforded by pytest-xdist...

@baentsch
Copy link
Member

Can I ask whether this PR can be shelved (closed) for the time being, @Martyrshot ?

@Martyrshot
Copy link
Member Author

Can I ask whether this PR can be shelved (closed) for the time being, @Martyrshot ?

Sorry I haven't had much time to work on personal projects. :( If others think this PR still makes sense then I can look into wrapping it up. But if there is no demand for it, then we can close it.

@baentsch
Copy link
Member

If others think this PR still makes sense then I can look into wrapping it up. But if there is no demand for it, then we can close it.

The one big benefit I see this PR would bring is much better testing (coverage) on non-UNIX'ish platforms. But as I don't see strong demand for liboqs there I'd think it'd be OK to close this -- maybe create an issue pointing to this instead for someone (or yourself) to pick it up as time/renewed (or external) interest permits/allows.

@SWilson4
Copy link
Member

@Martyrshot OK with you if I close this PR for the time being and leave a pointer to it in #1494? It would be nice to shorten our list of open PRs. Of course, it would be a welcome contribution should you find time to reopen it in the future.

@SWilson4
Copy link
Member

@Martyrshot OK with you if I close this PR for the time being and leave a pointer to it in #1494? It would be nice to shorten our list of open PRs. Of course, it would be a welcome contribution should you find time to reopen it in the future.

Closing in the interest of reducing our PR backlog, but please feel free to reopen at any time.

@SWilson4 SWilson4 closed this Sep 27, 2024
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