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.

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

A clean and modern CMake build system #115

Open
dg0yt opened this issue May 3, 2022 · 14 comments
Open

A clean and modern CMake build system #115

dg0yt opened this issue May 3, 2022 · 14 comments
Labels
enhancement New feature or request
Milestone

Comments

@dg0yt
Copy link

dg0yt commented May 3, 2022

This is a summary of points for discussion about improving the CMake build system.

  • Do actually export config.
    The current config is written like a find module (legacy: inspecting the system), instead of using build-time information (modern: export from the build).
  • Do not build static and shared libs in the same time.
    This looks familiar for users of autotools, but it comes with a price: It adds complexity, including additional user-facing variables. Users must select the matching target name even if they just build a single config. It requires a different name for MSVC import libs, currently breaking the unmodified pc files. ...
  • Add aliases of the exported targets, for embedding the PCRE2 build via add_directory.
  • Document the canonical CMake config usage.
  • Add a post-install usage test to CI.
  • Follow widely used CMake style.
    lower_case(commands). Don't repeat condition in else() and endif(). Less whitespace.

References to previous discussion:

Who uses the CMake build system?

  • vcpkg. Apart from avoiding slow autotools builds on Windows, the default configurations build for release and debug, for the benefit of Visual Studio users.
  • (I can add more here base on comments.)
@PhilipHazel
Copy link
Collaborator

Thank you for picking this up. This project is entirely reliant on the CMake community for the maintenance of the CMake files, for which I'm grateful. (Come to think of it, the Autotools files were also a user contribution - I'm pretty ignorant about both build systems.) I'm happy to go along with whatever comes out of this discussion. Note that there is also some information about previous changes in the PCRE2 ChangeLog file, release 10.38 comment #2.

@dg0yt
Copy link
Author

dg0yt commented May 3, 2022

Note that there is also some information about previous changes in the PCRE2 ChangeLog file, release 10.38 comment #_2.

pcre2/ChangeLog

Lines 196 to 219 in a334ea2

2. Installed revised CMake configuration files provided by Jan-Willem Blokland.
This extends the CMake build system to build both static and shared libraries
in one go, builds the static library with PIC, and exposes PCRE2 libraries
using the CMake config files. JWB provided these notes:
- Introduced CMake variable BUILD_STATIC_LIBS to build the static library.
- Make a small modification to config-cmake.h.in by removing the PCRE2_STATIC
variable. Added PCRE2_STATIC variable to the static build using the
target_compile_definitions() function.
- Extended the CMake config files.
- Introduced CMake variable PCRE2_USE_STATIC_LIBS to easily switch between
the static and shared libraries.
- Added the PCRE_STATIC variable to the target compile definitions for the
import of the static library.
Building static and shared libraries using MSVC results in a name clash of
the libraries. Both static and shared library builds create, for example, the
file pcre2-8.lib. Therefore, I decided to change the static library names by
adding "-static". For example, pcre2-8.lib has become pcre2-8-static.lib.
[Comment by PH: this is MSVC-specific. It doesn't happen on Linux.]

Let's CC @jwsblokland and hear his POV on simultaneous static + dynamic.

@GregThain
Copy link
Contributor

Thanks again, @dg0yt. I don't claim to be a CMake expert, but I'm willing to help out here. Some other suggestions for improvements:

  • pcre2 creates separate libraries for 8 bit, 16 bit and 32 bit code-points by replicating the several dozen mostly identical cmake instructions for each of them. A cmake function/macro would avoid this duplication.
  • Before including pcre2.h, one needs to #define the appropriate word size. Typically, this is done in the source code right before the #include, but cmake can attach this compile definition to the appropriate library to improve the programmer's quality of life, so that it shows up on the compile command line.
  • A style issue, but cmake generator expressions can save a lot of repeated if/then/else code by using a more functional style.

@dg0yt
Copy link
Author

dg0yt commented May 4, 2022

Last not least, maybe the minimum version of CMake (for building, not for using) could be raised a little bit further. Dealing with limitations of old versions is a major barrier to maintaining the build system:
Who is forced to build with CMake 3.0 (2014) when building the latest PCRE2 (2022), and why?

For reference, Ubuntu 18.04 LTS (2018) comes with CMake 3.10.2. More versions:
https://repology.org/project/cmake/versions

@jwsblokland
Copy link

Hello @dg0yt,
Improving the current CMake build system is a good idea. Especially these days with CMake 3 you can make it much more cleaner.

Let me give me some context of this simultaneous static and dynamic build I have implemented. As you may have read, at the time I was using autotools to build PCRE2 standalone on Linux. To be more precise. These was a version installed by our HPC team but unfortunately it was an old version therefore I build it myself. My build was similar as the one installed by our HPC team, meaning it contains both static and shared library for 8 bit, 16 bit and 32 bit PCRE2. PCRE2 itself is used in another CMake project as an external library. The main difficult at the time was how to include it this CMake project and therefore I wrote this CMake config file. To be able to persuade the HPC team to make use of the CMake build, I made sure that the CMake build can mimic the autotools build configuration. It is not forced. You want switch off for example static build via a CMake option. Furthermore, the provided CMake config has been written such one can easily switch between a static or shared version of PCRE2, like

set(PCRE2_USE_STATIC_LIBS ON)
find_package(PCRE2 10.38.0 REQUIRED COMPONENTS 8BIT CONFIG)

This all works very well. It is a drop-in-replacement of the autotools with the benefit of easy inclusion in other CMake projects. I must admit I forgot to take into account the case that one could or even want to build PCRE2 within another CMake project.

@dg0yt
Copy link
Author

dg0yt commented May 4, 2022

Thanks @jwsblokland! I see my guess about the underlying rationale (autotools style) confirmed.

This all works very well

Indeed, you did an exellent job in implementing and documenting this static/shared behaviour. That's the "Do it right".
Still, sometimes this is seen as a CMake anti-pattern. That's the "Do the right thing".

Maybe the key question is: Is the following really relevant?

one can easily switch between a static or shared version of PCRE2, like

set(PCRE2_USE_STATIC_LIBS ON)

So here is my background:

The way I build my dependencies for years now, I never had use for mixed shared and dynamic linkage in the same build tree. To be honest I didn't need static linkage until starting to move to vcpkg one year ago. In vcpkg, linux and macos builds are static by default. It took me many months to get some ports into a usable state, cross-platform, given many dependencies that had their own ideas of "easy" things with static usage, or with debug postfix, or packages simply being ignorant of transitive usage requirements, or packages using different build systems on different platforms.

It is cheap and save to have another build tree or installation prefix with a single consistent configuration, but it needs quite some care to keep consistency in a mixed configuration with many dependencies and transitive usage requirements in particular for static linkage. So these are my preferences in fixing or updating ports in vcpkg:

  • I don't want (packages I build) to switch between static and dynamic PCRE2 via variables (non-standard behaviour), but I want that they to just work with the configuration that I provide explicity using CMAKE_PREFIX_PATH or <Pkg>_DIR (standard behaviour).
  • I don't want (CMake Config I use) to do Find module style introspection subject to downstream variable manipulations (non-standard behaviour) but I want to get the targets with original properties exported by CMake (standard behaviour).
  • I don't want packages to provide different target names for static vs. dynamic linkage.
  • I don't want (packages I build) to be required to handle alternate MSVC (import) library names. Not every project uses CMake to discover PCRE2.
    (I started to take a closer look at PCRE2 because wxWidgets currenly uses pkg-config (from CMake!) to look for PCRE2, which worked for dynamic configurations but not for static configurations.)
  • Basically, I don't want to be forced to explicitly configure both BUILD_SHARED_LIBS (standard) and BUILD_STATIC_LIBS (non-standard) when the standard variable is enough to control the alternative configurations.

While issues like the MSVC import lib name might be resolved with more CMake code and variables, I would prefer to resolve all this with less complexity, by dropping support for mixed configurations.

@carenas
Copy link
Contributor

carenas commented May 4, 2022

Last not least, maybe the minimum version of CMake (for building, not for using) could be raised a little bit further

We have no reason not to raise it, indeed I just raised it to 3.1 with #110.

AFAIK the ONLY reason why it was 3.0 is because cmake used to complain loudly that the previous number there was obsolete and 3.0 was the one that stopped the complaining

@jwsblokland
Copy link

To answer your question:

Maybe the key question is: Is the following really relevant?

one can easily switch between a static or shared version of PCRE2, like

set(PCRE2_USE_STATIC_LIBS ON)

Yes this is relevant. At least in a HPC (High Performance Computing) environment. There are many different codes with different needs or preferences. Some developers prefer shared libraries while others static ones. Typically in such environment they typically install a version of a library and ideally it contains both the static and shared version such that developers can choose which one they want to use.

@carenas is right, upgrading to 3.0 was only done to get right of this annoying warning message. Personally, I am in favor of going to a more recent CMake version but we need to be careful not to force users to upgrade to a recent version.

@PhilipHazel
Copy link
Collaborator

I have merged #116 as it seemed reasonable.

@wrowe
Copy link
Contributor

wrowe commented May 7, 2022

Thank you so much for addressing this, as was pointed out the community "builds the build" environment, and autostuff and cmake community users don't see eye to eye.

But I'd like them all to accomplish similar results, so I'm happy to add review passes on your PR this week, and figure out the other oddball questions to solve on CMake bulds.

Finally although it might seem odd, envoyproxy and many other bazel built projects rely on solid, multiplatform cmakelists.txt constructions of their projects, it's not simply Windows. So let's see how we can keep pcre2 useful everywhere with cmake builds. TYVM

@elfring
Copy link

elfring commented May 22, 2022

I became also curious how the software will evolve further together with improved build systems. 🤔

@teo-tsirpanis
Copy link
Contributor

#260 will address the first bullet point and pave the way to improved pcre2 support in vcpkg. I would appreciate your comments.

@NWilson
Copy link
Member

NWilson commented Dec 8, 2024

@dg0yt @teo-tsirpanis

I have just taken over this week as the new maintainer of PCRE2. I can see that this CMake issue has been open for a long time!

What I would like to do, is to make a 10.45-RC1 as soon as possible in December, and then a 10.45 release in Jan/Feb. There have been a lot of code changes since 10.44 ("a lot" is relative - small for a commercial project, but more than is typical for a PCRE2 release).

So, I would like to delay fixing this CMake issue until a future release, probably 10.46 in Spring 2025.

I can see that Theodore has even made commits on this issue, which were then reverted 😢. So I would like to apologise for the delay, but I hope you understand that as a new maintainer my first priority is to release from master as it is today, and then start working through the backlog of issues in future releases.

I hope this is acceptable to our downstream CMake users.

@carenas
Copy link
Contributor

carenas commented Dec 8, 2024

I can see that Theodore has even made commits on this issue, which were then reverted 😢.

AFAIK, the revert was needed because the changes prevented a release of PCRE2, and indeed it was a last resort, since the work from Theodore was otherwise well rounded (might be even be used in vcpkg's fork)

@NWilson NWilson self-assigned this Jan 8, 2025
@NWilson NWilson added this to the 10.46 milestone Jan 8, 2025
@NWilson NWilson removed their assignment Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants