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.

Sign up for GitHub

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

Jump to bottom

Support system catch2 #23654

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support system catch2 #23654

wants to merge 1 commit into from

Conversation

Apteryks
Copy link
Contributor

Teach the Autoconf/Bakefile build system to use catch2 from the system.

@vadz
Copy link
Contributor

vadz commented Jun 19, 2023

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?

@Apteryks
Copy link
Contributor Author

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 make -f build/autogen.mk, I get:

[...]
no changes in ../../autoconf_inc.m4
no changes in ../../Makefile.in
0 files modified
make -C build/autoconf_prepend-include

make[1] : on entre dans le répertoire « /home/maxim/src/wxWidgets/build/autoconf_prepend-include »
autom4te -B . --language=Autoconf --freeze --output=autoconf/autoconf.m4f
autoconf/general.m4:301: error: m4_copy: won't overwrite defined macro: AC_PREREQ
autoconf/general.m4:301: the top level
autom4te: /gnu/store/2awzcd1jdd8h65ahdn62ydllrs2s2pk1-m4-1.4.19/bin/m4 failed with exit status: 1
make[1]: *** [Makefile:25 : autoconf/autoconf.m4f] Erreur 1
make[1] : on quitte le répertoire « /home/maxim/src/wxWidgets/build/autoconf_prepend-include »
make: [build/autogen.mk:29 : autoconf_m4f] Erreur 2 (ignorée)
autoconf -B build/autoconf_prepend-include

If based on master directly.

@vadz
Copy link
Contributor

vadz commented Jun 19, 2023

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).

@vadz vadz added the work needed Too useful to close, but can't be applied in current state label Jun 19, 2023
@Apteryks
Copy link
Contributor Author

Apteryks commented Jun 19, 2023 •
edited

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.

Maybe it's my version of Autoconf (2.69); there shouldn't be anything cached in my environment:

$ export LANG=C && git clean -xfdd && make -f build/autogen.mk 
Removing autom4te.cache/
make -C build/autoconf_prepend-include
make[1]: Entering directory '/home/maxim/src/wxWidgets/build/autoconf_prepend-include'
autom4te -B . --language=Autoconf --freeze --output=autoconf/autoconf.m4f
autoconf/general.m4:301: error: m4_copy: won't overwrite defined macro: AC_PREREQ
autoconf/general.m4:301: the top level
autom4te: /gnu/store/2awzcd1jdd8h65ahdn62ydllrs2s2pk1-m4-1.4.19/bin/m4 failed with exit status: 1
make[1]: *** [Makefile:25: autoconf/autoconf.m4f] Error 1
make[1]: Leaving directory '/home/maxim/src/wxWidgets/build/autoconf_prepend-include'
make: [build/autogen.mk:29: autoconf_m4f] Error 2 (ignored)
autoconf -B build/autoconf_prepend-include

I'll try cherry picking the single commit I expect to fix this (stop versioning Autoconf-provided M4 files).

@Apteryks Apteryks force-pushed the support-system-catch2 branch 2 times, most recently from 2974386 to 3f20bb6 Compare June 19, 2023 21:00
@Apteryks Apteryks mentioned this pull request Jul 19, 2023
Closed
@Apteryks
configure.in: Allow using catch2 from system.
7c2019f 
* 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.
vadz
vadz requested changes Aug 22, 2023
View reviewed changes
Copy link
Contributor

@vadz vadz left a 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.

build/bakefiles/config.bkl
@@ -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"/>
Copy link
Contributor

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.

configure.ac
$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
Copy link
Contributor

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?

configure.ac
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],, [
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadz vadz vadz requested changes

Assignees
No one assigned
Labels
work needed Too useful to close, but can't be applied in current state
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Apteryks @vadz