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

Running cmake -LA changes library configuration #1847

Closed
SWilson4 opened this issue Jul 18, 2024 · 6 comments
Closed

Running cmake -LA changes library configuration #1847

SWilson4 opened this issue Jul 18, 2024 · 6 comments
Labels
bug Something isn't working; high priority to fix

Comments

@SWilson4
Copy link
Member

Describe the bug
If cmake -LA .. is run between the library configuration (cmake -GNinja <config options> ..) and build (ninja) commands, the library configuration may be changed.

To Reproduce
From the liboqs directory:

rm -rf build && mkdir build && cd build
cmake -GNinja -DOQS_MINIMAL_BUILD="KEM_kyber_512;SIG_dilithium_2" -DCMAKE_BUILD_TYPE=Debug ..
ninja
gdb -ex "b pqcrystals_dilithium2_avx2_keypair" -ex r --args ./tests/test_sig Dilithium2

avx2_enabled

As expected, the breakpoint is hit: the AVX2 code is enabled in the build.

Here is the exact same command sequence with cmake -LA .. added between config and build:

rm -rf build && mkdir build && cd build
cmake -GNinja -DOQS_MINIMAL_BUILD="KEM_kyber_512;SIG_dilithium_2" -DCMAKE_BUILD_TYPE=Debug ..
cmake -LA ..
ninja
gdb -ex "b pqcrystals_dilithium2_avx2_keypair" -ex r --args ./tests/test_sig Dilithium2

avx2_disabled

The breakpoint cannot be set: the AVX2 code is disabled. Also note that the number of targets built is greater in the first case (127) than in the second (101).

Expected behavior
The cmake -LA .. call should have no side effects.

Screenshots
See above

Environment (please complete the following information):

  • OS: Ubuntu 22.04
  • OpenSSL version: 3.0.2
  • Compiler version used: GCC 11.4.0
  • Build variables used: see above
  • liboqs version: main, at commit 5670edf

Additional context
We use this pattern basically everywhere in CI to print config variables, so the bug's impact could have been huge. Thankfully, it looks like there were no failures hidden by the bug. This branch removes all occurrences of cmake -LA .. from CI, and everything passes. Based on some older investigation, I think we can infer that the weekly tests are unaffected as well.

I believe that the issue lies in our filtering logic when an option like OQS_MINIMAL_BUILD or OQS_ALGS_ENABLED is set. I'll investigate further.

@SWilson4 SWilson4 added the bug Something isn't working; high priority to fix label Jul 18, 2024
@SWilson4
Copy link
Member Author

This seems to me like something we should fix before the next release. Thoughts @dstebila @praveksharma @bhess?

@dstebila
Copy link
Member

Seems peculiar, and worth fixing. I notice from the cmake documentation that there's a command-line option -N which means "View mode only. Only load the cache. Do not actually run configure and generate steps." So I wonder if cmake -LA -N .. would have the desired effect? I'm not quite able to reproduce locally because running your gdb command on my Intel Mac seems to be throwing some errors. But I do notice that the number of files that ninja builds for me is as follows in the various scenarios:

  • cmake -GNinja -DOQS_MINIMAL_BUILD="KEM_kyber_512;SIG_dilithium_2" -DCMAKE_BUILD_TYPE=Debug ..; ninja: 127 files
  • cmake -GNinja -DOQS_MINIMAL_BUILD="KEM_kyber_512;SIG_dilithium_2" -DCMAKE_BUILD_TYPE=Debug ..; cmake -LA ..: 107 files
  • cmake -GNinja -DOQS_MINIMAL_BUILD="KEM_kyber_512;SIG_dilithium_2" -DCMAKE_BUILD_TYPE=Debug ..; cmake -LA -N ..: 127 files

So I'm cautiously optimistic about the -N option.

@SWilson4
Copy link
Member Author

SWilson4 commented Jul 19, 2024

cmake -LA -N does indeed produce the desired behaviour. Thanks! I've made a PR replacing all occurrences of cmake -LA with cmake -LA -N to get the quick fix out, at least.

As a long-term solution, I feel like we should refactor our CMake files so that this issue doesn't occur, -N switch or not. It's unintuitive behaviour (even we, the developers, didn't notice it for some time), and I'm not sure we want to count on users remembering to pass -N.

@SWilson4
Copy link
Member Author

SWilson4 commented Jul 19, 2024

As a long-term solution, I feel like we should refactor our CMake files so that this issue doesn't occur, -N switch or not. It's unintuitive behaviour (even we, the developers, didn't notice it for some time), and I'm not sure we want to count on users remembering to pass -N.

Following up on this, I've confirmed that the unwanted side effects occur with -DOQS_ALGS_ENABLED=STD as well, which I would imagine is a more widely used config option than OQS_MINIMAL_BUILD.

@dstebila
Copy link
Member

Can we close this since #1848 has landed?

@SWilson4
Copy link
Member Author

Can we close this since #1848 has landed?

Seems reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working; high priority to fix
Projects
None yet
Development

No branches or pull requests

2 participants