Add C++ test to ci

[Martyrshot]

There was an issue compiling the stateful-sigs branch for C++, but CI was passing. We should probably add a simple c++ test to CI to prevent issues like this in the future.

[aidenfoxivey]

Would this necessarily need to be repeated for all targets? Like (C++, GNU/Linux, x86_64), (C++, GNU/Linux, ARM64). Or would it be enough to try with MSVC on AMD64, g++ on amd64 linux, and clang++ on amd64 linux?

[aidenfoxivey]

Interested in helping with this, but want to make sure I understand the specific requirements.

[Martyrshot]

This is more of a syntax sanity check than a platform compatibility thing. So I would say the CI test would only have to build and link against a simple C++ target on a single primary platform. I would heir on the side of amd64 linux myself. 😄

Do you have any thoughts @baentsch @dstebila?

[dstebila]

This is more of a syntax sanity check than a platform compatibility thing. So I would say the CI test would only have to build and link against a simple C++ target on a single primary platform. I would heir on the side of amd64 linux myself. 😄

Do you have any thoughts @baentsch @dstebila?

I'm not a C++ developer myself (traumatic memories from undergraduate concurrency classes) so I defer to those who work with C++ regularly.

[aidenfoxivey]

I suppose the other relevant question is which version of C++ to support. A common one is C++17 right now I believe. C++11 is probably an okay baseline.

[Martyrshot]

I don't think we take advantage of any super modern C++ features. I also don't see us needing anything newer than C++11 in the near future. So in the interest of compatibility I would agree with using C++11 as the primary version we test against.

[aidenfoxivey]

That sounds good to me. I can write something up for that. The flow is basically just compile with `g++` in C++11 mode instead of a C compiler and ensure that it passes, right? Not some sort of static check instead?

[Martyrshot]

I think that's sufficient for now. Maybe have it run the test suite as well (ninja run_tests). It should still pass, unless we've done something horribly wrong. 😄

[aidenfoxivey]

Does this warrant having a compile option like OQS_USE_CPP?

[Martyrshot]

I don't think so. liboqs is a C first library. If someone wants to explicitly compile it using a C++ compiler instead of a C compiler then I think they can use the appropriate cmake definition prior to calling ninja to do so.

[aidenfoxivey]

I don't think so. liboqs is a C first library. If someone wants to explicitly compile it using a C++ compiler instead of a C compiler then I think they can use the appropriate cmake definition prior to calling ninja to do so.

Sounds good!

[aidenfoxivey]

Okay, sweet, I've got it working by modifying CMakeLists.txt and src/CMakeLists.txt. It's building as expected and passing the tests with ninja run-tests.

Is it poor form to have a GitHub Action that modifies the aforementioned files during runtime to insert the directives? I was considering using environment variables, but my initial attempt was too crude.

[baentsch]

Is it poor form to have a GitHub Action that modifies the aforementioned files during runtime to insert the directives?

IMO this would be "suboptimal". In my view there should be cmake toolchain files/directives that can be triggered by a single command (plus whatever support files needed to facilitate that -- which of course may not wreak havoc on "standard builds"). The minimal solution may be a script that does these changes "on the fly" but that is self-contained and can be run outside of CI too (accompanied by documentation as to how to run/maintain as & when the code base changes).

[aidenfoxivey]

Hmm, fair point. I'll look through the CMake documentation. I'm sure there's a way via switches or environment variables to tell CMake to set the language to CXX or to recognize the existing files as being C++.

[Martyrshot]

I'm a bit curious why it was required to modify CmakeLists.txt. I was expecting it to be something like cmake -GNinja -DCMAKE_C_COMPILER=$CXXCOMPILER.

It's also worth mentioning that as part of this we would want to link against a simple c++ test program. It could be something as simple as creating a signature context and exiting. The original reason for this issue was because the headers we were using were fine when linking with a C program because the problematic code blocks were hidden behind #ifdef blocks. When we are linking to a c++ binary, the defines being checked in the ifdef blocks are set and the problematic code is then included and causes compilation errors.

So we will for sure want to have a test that:

1. Compiles liboqs (using gcc is fine. but using g++ could be a nice sanity check)
2. Compiles a simple test c++ program and links against the liboqs compiled in step 1.
3. Runs the simple test c++ program to sanity check that linking actually worked.

[aidenfoxivey]

I'm a bit curious why it was required to modify CmakeLists.txt. I was expecting it to be something like cmake -GNinja -DCMAKE_C_COMPILER=$CXXCOMPILER.

It's also worth mentioning that as part of this we would want to link against a simple c++ test program. It could be something as simple as creating a signature context and exiting. The original reason for this issue was because the headers we were using were fine when linking with a C program because the problematic code blocks were hidden behind #ifdef blocks. When we are linking to a c++ binary, the defines being checked in the ifdef blocks are set and the problematic code is then included and causes compilation errors.

So we will for sure want to have a test that:

1. Compiles liboqs (using gcc is fine. but using g++ could be a nice sanity check)
2. Compiles a simple test c++ program and links against the liboqs compiled in step 1.
3. Runs the simple test c++ program to sanity check that linking actually worked.

My initial thinking was to set the project language to include C++ and then set the targets file type to C++. Your solution seems preferable. One thing I have to check is whether it compiles all of the files as C++ and not just a subset. I'll write a little C++ test program.

[aidenfoxivey]

Seems like cmake -GNinja -DCMAKE_C_COMPILER=$CXXCOMPILER is resulting in mostly Building C object instead of Building CXX object. I have a feeling there's some subtlety to how this works, probably complicated by the fact I've never used CMake before. I'll poke around some more.

[aidenfoxivey]

Can confirm that the behaviour before was an anomaly.

CCompiler.c:2:3: error: "The CMAKE_C_COMPILER is set to a C++ compiler"
        2 | # error "The CMAKE_C_COMPILER is set to a C++ compiler"
          |   ^
    1 error generated.
    ninja: build stopped: subcommand failed.

Seems ninja and/or CMake are resistant to the idea of having CMAKE_C_COMPILER set to clang++ for example.

[Martyrshot]

Ah I see. Then I think it's fine that we simply test linking liboqs (compiled with a c compiler) with a c++ test program. That will at least solve the concerns I had. :)

Thanks for working on this btw!

[aidenfoxivey]

https://gist.github.com/aidenfoxivey/242ec40423b086a242c46993d7a4e22f

Maybe something like this is a good choice? It's a minimally modified version of example_sig.c. Essentially it uses nullptr instead of NULL, static_cast<uint8_t*> instead of C-style casts, and uses the C++ version of the imports.

I've got it compiling (not linking yet) with C++11 standard.

[Martyrshot]

I think this is on the right track. I think if we can make it "a little more c++y" that would be great. maybe consider using unique_pointers instead of raw C pointers where you can. Then once it links successfully against liboqs.a I think I'll be happy. :)

[aidenfoxivey]

Got it working! I'm going to make sure it's reliable and then I'll open a PR.

[Martyrshot]

Nice work! If you'd like to try sanity checking that the linking fails when it should, try removing this curly brace

liboqs/src/sig/sig.h

Line 323 in 7f4c89b

} // extern "C"

.

This should cause some wild compiler errors because of messed up scoping. :)

Edit: Make sure that you edit the sig.h that is being linked against. If as part of your CI job you ninja install liboqs, it would be where the includes are installed!

[aidenfoxivey]

https://gist.github.com/aidenfoxivey/242ec40423b086a242c46993d7a4e22f

Perhaps this is more C++ friendly.

[aidenfoxivey]

tests/example_sig.cpp:182:2: error: expected '}'
  182 | }
      |  ^
build/include/oqs/sig.h:31:12: note: to match this '{'
   31 | extern "C" {
      |            ^
1 error generated.

Seems that for the first time in my experience of using clang, the compiler warning is simple and not bonkers.

[aidenfoxivey]

I also checked this with -fsanitize=address and ASAN_OPTIONS=detect_leaks=1. It showed the same output as running the original file in C. (120 byte(s) leaked in 3 allocation(s).) I take it this is just part of how liboqs works. I'd try with valgrind, but I'm on an Aarch64 Mac right now, so I can't run it.

I'm pretty sure I can conclude then that I've not introduced new leaks.

[aidenfoxivey]

https://gist.github.com/aidenfoxivey/7a41d2c6088eae59ae2960b8eede21d5

Here's what I've got for a GitHub action. The bit I'm still trying to figure out is whether -lcrypto is required and whether I need to specify where OpenSSL is installed on the machine.

[aidenfoxivey]

I believe this issue can be closed now.

#1971