-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support system catch2 #23654
base: master
Are you sure you want to change the base?
Support system catch2 #23654
Conversation
Before looking at it further, this seems to include #23640, right? Does it really need it or could you remove all the other commits except for the last one here? |
I wasn't sure, but it seems like it does, at least using Autoconf 2.69. When attempting to regenerate the configure script with
If based on master directly. |
Sorry but I simply don't see how is this possible as the last commit doesn't do anything that materially changes anything. You must have some local changes/cached files breaking this. You should create a PR with just this commit (after fixing the problems in it which result in the CI failures). |
b55e838
to
2dbd68d
Compare
Maybe it's my version of Autoconf (2.69); there shouldn't be anything cached in my environment:
I'll try cherry picking the single commit I expect to fix this (stop versioning Autoconf-provided M4 files). |
2974386
to
3f20bb6
Compare
* configure.in (--with-catch2): New option. [PKG_CONFIG]: Search catch2 via pkg-config, fall back to use bundled copy. Add a new WX_TEST_CXXFLAGS Autoconf variable to configure the include directory. * build/bakefiles/config.bkl (WX_TEST_CXXFLAGS): Add option. * tests/test.bkl [autoconf]: Add WX_TEST_CXXFLAGS option to cxxflags. * configure: Regenerate via 'make -f build/autogen.mk'. * tests/Makefile.in: Likewise.
3f20bb6
to
7c2019f
Compare
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.
Thanks for the update but unfortunately there are still some things that could be improved.
I also think that it would make sense to default to built-in version: unlike the other libraries, this is not used for the binaries being distributed, so I don't see any drawbacks to using the built-in version by default and it would avoid warnings about not finding the system one, which would be annoying to see.
@@ -408,6 +408,7 @@ compiled .lib files and setup.h under the lib/ toplevel directory. | |||
<option name="WX_CFLAGS"/> | |||
<option name="WX_CXXFLAGS"/> | |||
<option name="WX_LDFLAGS"/> | |||
<option name="WX_TEST_CXXFLAGS"/> |
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.
I think it's a bit misleading to call it like this, it should probably be called CATCH2_CXXFLAGS
or something like this instead.
$srcdir/3rdparty/catch/single_include/catch2/catch.hpp couldn't be found. | ||
dnl Check for catch (C++ Automated Test Cases in Headers) availability. | ||
if test "$wxUSE_CATCH2" != "builtin"; then | ||
if test -n "$PKG_CONFIG"; then |
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.
We don't test for PKG_CONFIG
anywhere else, so either we shouldn't do it here neither or we should do it in all the other places too... But is it even possible for it to be empty? I.e. won't configure stop if PKG_PROG_PKG_CONFIG
doesn't find it?
if test -n "$PKG_CONFIG"; then | ||
dnl TODO: Support catch2 version 3, which does not | ||
dnl ship a catch2/catch.hpp header anymore. | ||
PKG_CHECK_MODULES([CATCH2], [catch2 < 3],, [ |
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.
We should use CATCH2_CFLAGS
somewhere, otherwise compilation will fail if it's not installed under /usr/include
.
Teach the Autoconf/Bakefile build system to use catch2 from the system.