-
Notifications
You must be signed in to change notification settings - Fork 216
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
catch2: Upgrade to v3.1.0 #643
Conversation
Whoops. Meant to just run the CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI is failing because this no longer provides catch2_dep = declare_dependency(...)
-- it would be apropos to provide one using link_with: catch2, include_directories: 'src'
.
Now that catch2-with-main.pc is an upstream thing, I think it would be nice to additionally add that to the [provide]
section of the wrap file.
'Catch2Main', | ||
[ 'internal/catch_main.cpp' ], | ||
include_directories : '..', | ||
dependencies : [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add link_with: catch2
, in order to guarantee that dependencies are traced.
libraries : catch2_with_main, | ||
version : meson.project_version(), | ||
description : 'A modern, C++-native, test framework for C++14 and above (links in default main)', | ||
requires : 'catch2 = ' + meson.project_version() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here, the link_with will cause pkg.generate to detect that catch2_with_main depends on the "catch2" pkg-config dependency, although this automatic handling doesn't add a version match, hmm...
Excellent analysis, I agree. EDIT: Note you can build them with BUILD_SHARED_LIBS=ON, but then their cmake configuration throws a warning at you. And in the FAQ:
|
Thank you for the comprehensive and positive review. I believe that I've addressed all your points. I did need to use I was also able to actually test the results rather better this time too by copying the wrap file and symlinking my Catch repository into another project that used Catch2. Is there a better way to do this that I've missed mention in the documentation for? |
Yeah, that's fine, it just depends on which directory you create it from. :)
The wrapdb repository itself can be used, via The other solution is to run This is not precisely documented, though. |
There are two CI failures:
|
# This isn't as good as the CMake tests, but it proves that we've | ||
# actually put something in the library files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like most of what the CMake tests are doing is regexes on the output of the test binary, implementing that would mean having to write a custom test runner python script... Probably not worth it. :D
b63043b
to
d3efbc0
Compare
I added Whilst reviewing the patch I did realise that I've been rather inconsistent in where I've put the closing parentheses. I can make them consistent, but to do that I've to decide whether they should be on a line on their own for multi-line function invocations (e.g. as currently used for Thanks. |
FWIW my personal preference tends to the former. |
d3efbc0
to
9faca7a
Compare
Catch v3 now builds as static libraries rather than being a single include file, which means that the meson.build files need to be rather more complex. In addition to the main libCatch2.a library there's also a libCatch2Main.a library that contains the main() function that was previously enabled by defining CATCH_CONFIG_MAIN. Catch v3's own CMake build system doesn't build dynamic libraries by default, as described in the FAQ[1], so the Meson build only builds static libraries. [1] https://github.com/catchorg/Catch2/blob/devel/docs/faq.md#can-i-compile-catch2-into-a-dynamic-library
9faca7a
to
596b4ac
Compare
eli-shwartz wrote:
After initially managing to do exactly the wrong thing, I believe that I've now done this correctly. I also switched to consistently using two spaces for indents. (Previously, for some reason Emacs wanted to use two characters for the function invocations but four characters for the |
Thanks! Looks great. |
Thank you for the reviews and for merging the change. |
Incorporate improvements suggested by Meson maintainer in mesonbuild/wrapdb#643 .
Incorporate improvements suggested by Meson maintainer in mesonbuild/wrapdb#643 .
This PR breaks cppzmq:
|
So "catch2" updating from version 2 to version 3 is incompatible, which is an interesting naming scheme but okay. |
It's a bit confusing, but "Catch2" is now the name of the project, not its version number. See also: catchorg/Catch2#769 The project tries to follow semantic versioning, so Catch2 v3.1.0 isn't API compatible with Catch2 v2.x. Does this mean that we need separate catch2-2 and catch2-3 in the wrapdb? Or should we just wait for cppzmq to catch up? |
Probably the latter. I don't know if this was fixed upstream. |
If software is intended to be parallel-installable across major versions, it usually comes with separate dependency names. See for example gtk2 vs. gtk3 vs. gtk4. Having multiple different providers for the same dependency in the wrapdb might not work 100% fantastically... |
Catch v3 now builds as a library rather than a single include file, which means that the meson.build files need to be rather more complex.
Catch v3's own CMake build system doesn't build dynamic libraries, and it doesn't seem like doing so would be worthwhile for such a project, so the Meson build only builds static libraries.