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.

Sign up for GitHub

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

Jump to bottom

Use modern CMake syntax #1494

Open
vsoftco opened this issue Jun 9, 2023 · 6 comments
Open

Use modern CMake syntax #1494

vsoftco opened this issue Jun 9, 2023 · 6 comments
Assignees
@vsoftco
Labels
documentation Changes only about documentation

Comments

@vsoftco
Copy link
Member

vsoftco commented Jun 9, 2023 •
edited
Loading

Instead of depending on make/msbuild etc, we should use only modern cmake and ctest, such as

cmake -B build -DCMAKE_FLAGS...
cmake --build build --parallel 4 [--target ... ]
ctest --test-dir build

This is platform-independent and saves a lot of headaches, and completely removes make and msbuild explicit dependencies. See https://github.com/open-quantum-safe/liboqs-cpp/blob/main/.github/workflows/cmake.yml for an example.
@vsoftco vsoftco self-assigned this Jun 9, 2023
@vsoftco vsoftco added the enhancement New feature or request label Jun 9, 2023
@dstebila
Copy link
Member

So cmake --build build would also replace ninja?

I don't mind, though I haven't had any trouble with our current build sequence.

The first two commands work fine for me, but ctest --test-dir build doesn't do anything. I'm guessing this is because our test suite is under a custom target (run_tests) rather than some default name?

@vsoftco
Copy link
Member Author

vsoftco commented Jun 12, 2023 •
edited
Loading

Yes, it'll replace ninja, and is completely platform-independent. I'll look into the test suite...

cmake --build build --target run_tests runs the test suite, but it'll be preferably to be able to use ctest instead.

@baentsch
Copy link
Member

I like the platform-independence aspect (particularly looking at oqs-provider for Windows right now). What makes me wonder, though, is how this will impact build+test/CI runtime: ninja always nicely & automatically takes the maximum amount of CPUs available. The proposal above seems to require a hardcoded number of CPUs (that may or may not be available, i.e., run too slow if more CPUs were available and may overwhelm the machine if too many are spec'd).

@dstebila
Copy link
Member

I did --parallel without a number, and it seemed to use the maximum number of CPUs on my machine, running about as fast as ninja does.

@vsoftco
Copy link
Member Author

vsoftco commented Jun 12, 2023

@dstebila Thanks, that's good to know, I didn't know we can use the flag without a number

@dstebila dstebila added documentation Changes only about documentation and removed enhancement New feature or request labels Jul 5, 2023
Martyrshot pushed a commit that referenced this issue Feb 17, 2024
change from using ninja and custom cmake target run_test to using c…
c45b9df 
…make & ctest.

Upon reading issue #1494
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
```

Unfortunately, cmake does not have the nice pytest --ignore feature. So to ensure
test_kat_all doesn't run by default, the `OQS_ENABLE_LONG_TESTS` variableis 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 Martyrshot mentioned this issue Feb 17, 2024
Closed
Martyrshot pushed a commit that referenced this issue Feb 17, 2024
change from ninja and custom cmake target run_test to using cmake &…
b64b12a 
… 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
```
@ajbozarth ajbozarth moved this to Todo in liboqs planning Jul 23, 2024
@SWilson4
Copy link
Member

@Martyrshot took an initial stab at this in #1700. That PR would be a good jumping-off point for anyone who would like to take this up in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vsoftco vsoftco

Labels
documentation Changes only about documentation
Projects
liboqs planning
Status: Todo
Milestone
No milestone
Development

No branches or pull requests
4 participants
@vsoftco @dstebila @SWilson4 @baentsch