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

Fixed building for catch2 (>=3.0) #84

Merged
merged 2 commits into from
Jun 14, 2023

Conversation

kotopesutility
Copy link

Fixed building for new catch2 (>=3.0). I don't like my solution for CMake but I wanted to save building for old catch2 (<3.0) for Fedora or something like that.

@kotopesutility kotopesutility changed the title Develop Fixed building for catch2 (>=3.0) Jun 5, 2023
@fpagliughi
Copy link
Owner

Oh, cool. I was thinking I should update the Catch2 support, but was a bit too busy to get to it.

I wasn't thinking of maintaining support for the older (2.x) version, but if it's this easy then maybe, why not?

But I wonder if it might be better to isolate the conditional compilation to a single, new test header file rather than leak it into all the test sources? Maybe make a tests/unit/catch2_version.h with the details:

#ifdef CATCH2_V2
    #include "catch2/catch.hpp"
#else
    #include "catch2/catch_all.hpp"
#endif

And the defined macro could be a little more descriptive, like CATCH_V2 or CATCH_V2X or something.

Either that, or say nevermind to maintaining v2, and just bump the requirement to v3.

@kotopesutility
Copy link
Author

Well, I have force-pushed my solution. It includes catch2v2 support for distributions like Fedora, Debian, and so on.

I wasn't thinking of maintaining support for the older (2.x) version, but if it's this easy then maybe, why not?

Cool, let it be: CATCH2_V2 like in your example^

And the defined macro could be a little more descriptive, like CATCH_V2 or CATCH_V2X or something.

@fpagliughi
Copy link
Owner

Cool. That looks good. I may not have time to test it until the weekend, but assuming it all works as expected, I'll get it merged ASAP.

Thanks for the contribution!

@kotopesutility
Copy link
Author

Any updates?)

@fpagliughi
Copy link
Owner

It seems to work for both v2 and v3. Nice.

I may get rid of the CMake warning for v2. If we support both versions (for now) then no need to complain about it. At least until we decide and state that v2 is obsolete and won't be maintained going forward.

Thanks for the help!

@fpagliughi fpagliughi merged commit ab522ec into fpagliughi:develop Jun 14, 2023
@kotopesutility kotopesutility deleted the develop branch June 15, 2023 08:15
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.

2 participants