PCRE2Project / pcre2

PCRE2 development is now based here.
Other
918 stars 191 forks source link

Fix PCRE2_DEBUG for multi-configuration builds #542

Closed NWilson closed 2 weeks ago

NWilson commented 3 weeks ago

On Windows (MSVC) and Mac (Xcode), one config.h file is shared by both Debug and Release builds.

So, I think we should instead pass -DPCRE2_DEBUG on the commandline instead of including it in the config file, and set it to true only for Debug builds.

This ensures that CMake users on Windows & Mac will get their assertions checked automatically.

NWilson commented 3 weeks ago

Oh yes, the CTest integration has been done a little unusually.

On Linux I usually run the pcre2 tests with "ninja test" or "make test" (from the Cmake build directory - I never use autoconf if I can avoid it). On windows I use "Cmake --build . --target RUN_TESTS". So those variations work at least.

I can test with the multi config ninja and make sure it works.

NWilson commented 2 weeks ago

I've added support for "Ninja Multi-Config" and tested on Linux & Windows. The Windows support was already working (due to the support in pcre2_test.bat) but I had to tweak pcre2_test.sh a little bit. Not too hard, all works nicely.

carenas commented 2 weeks ago

Mac (Xcode)

Tested with Xcode 15.2 (which I never used before) but doesn't seem to be building anything in the IDE (although it says it succeeds)

it builds fine in the command line (even using the Xcode generator), but using ctest failed to find the binaries for both pcre2test and pcre2grep unless I use ctest -C Debug . while in the build directory.

NWilson commented 2 weeks ago

Interesting. I don't have a Mac or Xcode to test with.

On Windows, I can run tests with either cmake --build . --target test (when using ninja Multi-config), or cmake --build . --target RUN_TESTS (when using native Visual Studio cmake backend); or alternatively using ctest -C Debug. However just ctest on its own doesn't work.

NWilson commented 2 weeks ago

Updated commit description:


Fix PCRE2_DEBUG for multi-configuration builds

zherczeg commented 2 weeks ago

Is there anything more to do here, or is the patch ready?

NWilson commented 2 weeks ago

Carlo has asked for a change to the flags we test with (-O3 rather than -O2 in build.yml). I'll do that quickly, then it'll be ready.

NWilson commented 2 weeks ago

OK, ready to merge.

I have encountered two issues while fiddling for a while with different compile options:

These are pre-existing issues, so I've left them. We can address in a future PR.

carenas commented 2 weeks ago

LGTM

meant to hit the merge button, but failed somehow?

zherczeg commented 2 weeks ago

LGTM is looks good to me. Comes from my WebKit times.