Cleanup pkg-config #1326

vector-of-bool · 2023-06-28T18:49:49Z

On my yak-shaving journey to remove the source-dist, I ended up needing to rewrite the generation of pkg-config files (again) (long story). Fortunately, this is much cleaner and more robust than my prior silliness in #1301. The following now Just Works™:

If you build with instrumentation (coverage, sanitizers), the installed pkg-config files will now include the necessary link flags as usage requirements. This came up while testing the link-with scripts, since my local system would enable sanitizers, but the generated example executable did not have the appropriate link flags when it consumed libmongoc/libbson via pkg-config. This will now work out-of-the-box.
The prefix value in the installed .pc values is now bound at install-time, and not at configure-time. This allows using cmake --install --prefix=<some-dir> to do an install into an arbitrary <some-dir> which may not be the CMAKE_INSTALL_PREFIX. This is a somewhat-obscure feature, but I've found it useful and it was easy enough to make it work.
Multi-config generation is now supported. Now you can cmake --install --config=<config> to install the named configuration, and the appropriate .pc file will be installed. (But multi-config .pc install requires each configuration to receive a different .pc filename, which is really weird for pkg-config and is not used here.)

This reverts commit c4a4008.

vector-of-bool · 2023-06-28T19:09:48Z

build/cmake/ResSearch.cmake

@@ -1,6 +1,9 @@
 include(CheckSymbolExists)
 include(CMakePushCheckState)

+# Required so that some libresolv symbols will be visible:
+set(CMAKE_C_EXTENSIONS ON)


This seemingly-unrelated change is due to changing cmake_minimum_required in libmongoc, which changed the way that the C_STANDARD property is propagated through try_compile

Can this be scoped or moved into a target property? I have reservations about enabling C extensions for potentially all targets that follow include(ResSearch). If this must be set unconditionally, suggest moving up into the root CMakeLists.txt file alongside other project-wide variables.

I also did not like this change, and it isn't obvious from the documentation that this variable effects try_compile. I believe the reason we get away with it is that we enable bits and pieces by using platform-specific feature toggle macros. I will narrow it down and find what macros actually need to be defined to enable libresolv to compile properly.

Looks to actually be _BSD_SOURCE, which we already set, and I'm dropping it on the floor by using cmake_push_check_state(RESET). Oops.

rcsanchez97

LGTM, modulo the one typo I found.

rcsanchez97 · 2023-06-28T19:43:44Z

build/cmake/GeneratePkgConfig.cmake

+
+      DESTINATION <dir>
+        - If provided, specify the *directory* (relative to the install-prefix)
+          in which the generated file will be installed. If unspecfied, the default


Suggested change

in which the generated file will be installed. If unspecfied, the default

in which the generated file will be installed. If unspecified, the default

eramongodb · 2023-06-28T21:05:02Z

build/cmake/ResSearch.cmake

@@ -1,6 +1,9 @@
 include(CheckSymbolExists)
 include(CMakePushCheckState)

+# Required so that some libresolv symbols will be visible:
+set(CMAKE_C_EXTENSIONS ON)


Can this be scoped or moved into a target property? I have reservations about enabling C extensions for potentially all targets that follow include(ResSearch). If this must be set unconditionally, suggest moving up into the root CMakeLists.txt file alongside other project-wide variables.

eramongodb · 2023-06-28T21:06:01Z

src/libbson/CMakeLists.txt

@@ -1,4 +1,4 @@
-cmake_minimum_required (VERSION 3.1)
+cmake_minimum_required (VERSION 3.15)


This is a significant change. I suggest at least creating a ~~CXX~~ C ticket to track/document this new requirement.

Side question: is there a compelling reason we should continue to support a per-library CMake Project configuration instead of using only the root CMake project for all targets defined in the ~~CXX Driver~~ C Driver? I don't think we intend to support add_subdirectory() of a ~~CXX Driver~~ C Driver library (i.e. ~~bsoncxx~~ libbson) without going through the root directory, do we?

We're already requiring this at the top level, so this line could likely actually just be removed. It's actually probably bad that we had 3.1 there, since it would rewind a bunch of CMake policies and probably had some unintentional effects. (Such as the problem in ResSearch).

I don't think we intend to support add_subdirectory() of a CXX Driver library (i.e. bsoncxx) without going through the root directory, do we?

AFAIK there is no expectation to support add_subdirectory() without going through the root directory.

I expect C++ driver can be included with add_subdirectory of the root CMakeLists.txt, and CXX-2104 was filed to document.

This PR is for the C driver (not C++).

Had my repos mixed up. Sorry for the confusion.

eramongodb · 2023-06-28T21:20:08Z

build/cmake/GeneratePkgConfig.cmake

+All named parameters accept generator expressions.
+
+]==]
+function(mongo_generate_pkg_config target)


It baffles me that CMake doesn't provide any utilities to support smart target-based pkg-config file generation. 😞

build/cmake/GeneratePkgConfig.cmake

kevinAlbs

GeneratePkgConfig.cmake seems generally useful.

kevinAlbs · 2023-06-29T18:18:49Z

src/libmongoc/CMakeLists.txt

@@ -807,6 +807,7 @@ set_target_properties (mongoc_shared PROPERTIES
   SOVERSION 0
   pkg_config_REQUIRES "libbson-1.0"
   )
+mongo_generate_pkg_config(mongoc_shared INSTALL RENAME libmongoc-${MONGOC_API_VERSION}.pc)


Consider using FILENAME for consistency with other mongo_generate_pkg_config calls.

Is there a use case for setting different values for RENAME and FILENAME? Consider removing RENAME.

RENAME comes from install(FILES ...). The split in this case helps with multi-config generators. We want to allow multi-conf generation to work in the build, but we only need to install one version at a time.

vector-of-bool added 7 commits June 27, 2023 20:44

New generations of pkg-config files

3896bd6

Remove unused pc templates

e421311

C extensions are required for libresolv

8383802

Don't make cat sounds

195d91a

Disable legacy-shell for the releng variant

c4a4008

Revert "Disable legacy-shell for the releng variant"

163a21f

This reverts commit c4a4008.

Cleanup and comment more pkg-config gen code

626b615

vector-of-bool marked this pull request as ready for review June 28, 2023 19:07

vector-of-bool requested review from rcsanchez97, eramongodb and kevinAlbs June 28, 2023 19:07

vector-of-bool commented Jun 28, 2023

View reviewed changes

rcsanchez97 approved these changes Jun 28, 2023

View reviewed changes

eramongodb reviewed Jun 28, 2023

View reviewed changes

Fix: Allow _BSD_SOURCE to affect ResSearch.cmake

f333f71

kevinAlbs approved these changes Jun 29, 2023

View reviewed changes

sppel

15f08cc

vector-of-bool requested a review from eramongodb June 29, 2023 20:49

eramongodb approved these changes Jun 29, 2023

View reviewed changes

Merge branch 'master' into cleanup-pkg-config

353fb76

vector-of-bool merged commit 3c7951b into mongodb:master Jun 30, 2023

	in which the generated file will be installed. If unspecfied, the default
	in which the generated file will be installed. If unspecified, the default

		@@ -1,4 +1,4 @@
		cmake_minimum_required (VERSION 3.1)
		cmake_minimum_required (VERSION 3.15)

Cleanup pkg-config #1326

Cleanup pkg-config #1326

Uh oh!

Conversation

vector-of-bool commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcsanchez97 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eramongodb Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vector-of-bool commented Jun 28, 2023 •

edited

Loading

eramongodb Jun 28, 2023 •

edited

Loading