Fix PCRE2_DEBUG for multi-configuration builds #542
Conversation
There was a problem hiding this comment.
Technically the same is true of the "Ninja Multi-Config" generator that is available in more platforms and will be a good option if added to the CI (ex: changing the Perl job in dev.yml)
This will also require further changes to CMakeFiles.txt so it can generate a working pcre2_test.sh to be able to complete ctest.
|
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. |
|
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. |
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 |
|
Interesting. I don't have a Mac or Xcode to test with. On Windows, I can run tests with either |
ad33685 to
9488837
Compare
|
Updated commit description: Fix PCRE2_DEBUG for multi-configuration builds
|
.github/workflows/build.yml
Outdated
| @@ -77,7 +77,7 @@ jobs: | |||
| submodules: true | |||
|
|
|||
| - name: Configure | |||
| run: cmake -DPCRE2_SUPPORT_JIT=ON -DPCRE2_BUILD_PCRE2_16=ON -DPCRE2_BUILD_PCRE2_32=ON -DCMAKE_OSX_ARCHITECTURES='arm64;x86_64' -DCMAKE_C_FLAGS='-Wall -Wextra' -DCMAKE_COMPILE_WARNING_AS_ERROR=ON -B build | |||
| run: cmake -DPCRE2_SUPPORT_JIT=ON -DPCRE2_BUILD_PCRE2_16=ON -DPCRE2_BUILD_PCRE2_32=ON -DCMAKE_OSX_ARCHITECTURES='arm64;x86_64' -DCMAKE_C_FLAGS='-Wall -Wextra' -DCMAKE_COMPILE_WARNING_AS_ERROR=ON -DCMAKE_BUILD_TYPE=RelWithDebInfo -B build | |||
There was a problem hiding this comment.
I think all the jobs in build.yml should be using RelWithDebInfo (for the jobs using CMake of course). And, we want PCRE2_DEBUG to be left to its default, which will disable the assertions.
This mirrors the configuration which we expect customers and downstream packages to be using.
There was a problem hiding this comment.
The thinking was that we use the default of the generator used since they are meant to match what a developer runs localy, but you probably have a lot more experience with cmake as a developer than any of us.
There was a problem hiding this comment.
I never run CMake without specifying a CMAKE_BUILD_TYPE. Some generators (like the Makefiles backend) just do weird things, and generate configuration that nobody really wants.
I guess we could have one example of that in the dev.yml... but it would only apply on Linux anyway, and we're expecting most Linux users to be using autoconf instead. So I'd just simplify things, and only test on actual proper CMake configurations (Debug, Release, RelWithDebInfo).
There was a problem hiding this comment.
I realize I misread the context.
My comment was about jobs in dev.yml; for build.yml we want to mirror what end users would do for production, so while I think you are correct RelWithDebInfo is valid Release might be better (which will also test -O3 vs -O2 with gcc/clang)
There was a problem hiding this comment.
Interesting. I've personally always considered "RelWithDebInfo" to be the best for actual production releases.
To start with, no-one should be shipping binaries without having the debug info stored somewhere. Linux distributions always build with debug info, and ship the ELF+DWARF binaries in special "-debug" variant of the package, then run strip to publish the ordinary packages. That way, when users report a crash, you can symbolise it. Commercial vendors do the same thing (on all the companies and projects I've worked on) - build with symbols, store them, and ship the stripped binaries.
I also (personally) dislike O3 because it does too much inlining with GCC. Binary size for a C++ app can double, with no noticeable performance increase. For apps that are bigger than 10-20MB binary size, -Os is often the fastest in fact, because -O3 does so much loop unrolling it forces the code out of the L1 cache.
But yes, I totally acknowledge that people ship with -O2, -O3, or -Os.
Here's what I'll do: change build.yml to use Release (-O3), and in build.yml let's make sure we run the unit tests for each of -O2, -O3, and -Os.
CMakeLists.txt
Outdated
| MESSAGE(STATUS " Install prefix .................... : ${CMAKE_INSTALL_PREFIX}") | ||
| MESSAGE(STATUS " C compiler ........................ : ${CMAKE_C_COMPILER}") | ||
| if (CMAKE_CONFIGURATION_TYPES) | ||
| FOREACH (config IN LISTS CMAKE_CONFIGURATION_TYPES) |
There was a problem hiding this comment.
nitpick: can we use lowercase foreach?
There was a problem hiding this comment.
OK, done.
But someone really needs to just run an autoformatter over the file, it's totally inconsistent at the moment. I can do that in a follow-up.
There was a problem hiding this comment.
the inconsistency is by design, see #115
There was a problem hiding this comment.
Well 23 instances of "if" lowercase and 124 instances of "IF" can't be by design. The formatting should at least be consistent. It's not important though, given it has absolutely no functional impact.
Making it more idiomatic CMake would be nice in future, there are some good ideas in that thread.
There was a problem hiding this comment.
Sorry, I meant "by design" ironically, since that file has been hacked without any for way too long IMHO.
Indeed, I have to admit I was a little surprised no one mentioned taking over maintainer-ship of at least this file as part of our previous call for things that were needed to integrate with Excell
9488837 to
c849d82
Compare
|
Is there anything more to do here, or is the patch ready? |
|
Carlo has asked for a change to the flags we test with ( |
397be09 to
bff90f5
Compare
* Add support for Ninja Multi-Config generator on all platforms * Ensure one build job is using CMake 3.15 * Provide a build type (Debug or RelWithDebInfo) on all CMake builds * Ensure PCRE2_DEBUG=ON is set for: dev.yml builds which use the Release configuration
9139a36 to
69c6712
Compare
|
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. |
meant to hit the merge button, but failed somehow? |
|
LGTM is looks good to me. Comes from my WebKit times. |
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_DEBUGon 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.