CDRIVER-4637 Remove ENABLE_SRV=AUTO, clean up SRV URI support #1301

vector-of-bool · 2023-06-08T01:36:04Z

This changeset builds upon #1289, so it will look messy with those additional changes.

Removing the AUTO value of ENABLE_SRV was relatively simple, but required some changes to the way we discover the DNS resolver, and it also led to adding a new feature bit to the handshake (that may be an excessive change...).

Changes in this PR

Bits and Flags

The file FindResSearch.cmake was renamed to ResSearch.cmake. The Find prefix is reserved for find-modules, to be used with the find_package() command. Based on the way we use it, I decided it would be simpler to rename the file than to make it into a find-module.
ResSearch was modified significantly enough that Git counts it as a completely different file. The following tweaks are made:
- The configure-time checks are no longer conditional, except for not running on Windows. On non-Windows, these checks will always occur, and this change will be reflected in the mongoc-config.h file.
- Because these checks result in bits on the handshake message, this may change telemetry we receive on these features. i.e. Previously, when the bits were conditional, a low-bit on a feature could either mean that an earlier check succeeded or that the check for that feature failed. Now that they are unconditional (except on Windows) these bits will reflect their true values.
Because the bit values from ResSearch now reflect features of the platform regardless of the user's config settings, we need a new way to toggle ENABLE_SRV. Previously, code was conditionally compiled based on MONGOC_HAVE_RES_[N]SEARCH and MONGOC_HAVE_DNSAPI. Since disabling ENABLE_SRV implicitly set these to zero, the code for SRV URIs was omitted.
- New compile-time constant/macro MONGOC_SRV_ENABLED/MONGOC_ENABLE_SRV are defined in mongoc-config.h. These reflect the user's choice of ENABLE_SRV, regardless of the features available on the platform.
- Because other bits were previously used as a proxy for whether a user had set ENABLE_SRV, a new bit is added to the handshake to represent the value of this setting.

Targets and CMake

Previously, FindResSearch conditionally defined a RESOLV_LIBRARIES variable with the library name. This variable was implicitly used all over the code. Sad.
ResSearch defines an interface library mongo::detail::c_resolve that exposes the usage requirements of the underlying name resolution system.
A new interface library is defined: mongo::detail::c_dependencies. This library is meant to capture the usage requirements of the platform and optional features going forward.
If ENABLE_SRV is true, then mongo::detail::c_dependencies uses mongo::detail::c_resolve.
The magic RESOLVE_LIBRARIES variable is gone. Instead, mongoc_shared and mongoc_static now link to mongo::detail::c_dependencies. Now all downstream users will receive the usage requirements of mongo::detail::c_dependencies, which will include mongo::detail::c_resolve if ENABLE_SRV is true.
I anticipate converting many more of these options going forward, but will take these one at a time.

pkg-config Changes

This aspect of the changeset was unexpected and may be excessively engineered. Oh well. If we don't like this one, we can seek another way.

Before: .pc file generation relied heavily on spooky-action-at-a-distance variables propagating across the build tree. before landing around configure_file.
- Variables (such as RESOLVE_LIBRARIES) would be picked up before configure_file() and shuffled about into the set of Libs: and Cflags: values to be inserted.
Now: .pc files are generated using a combination of target properties, configure_file, CMake generator expressions, and a sequence of REGEX REPLACE.
- There is a single .pc.in file for both the static and shared libraries, and it is used to generate two distinct .pc files for each.
- The following target properties are inserted into the .pc file for each target:
  - pc_INCLUDE_DIRECTORIES adds -I flags. Relative paths are prepended by ${prefix}.
  - pc_REQUIRES adds Requires: lines
  - pc_LIBS is an array that builds Libs. INTERFACE_LINK_OPTIONS will also be added to Libs.
  - pc_NAME, pc_VERSION, and pc_DESCRIPTION set the Name, Version, and Description values.
  - The CMake target properties INTERFACE_COMPILE_DEFINITIONS and INTERFACE_COMPILE_OPTIONS are added to Cflags automatically.
- The MONGOC_LIBRARIES variable is gone. This was used solely for the generation of the pkg-config file for the static library. The pc_LIBS target property takes its place.
- For pkg-config dependencies still using spooky-action variables, the logic that assembled MONGOC_LIBRARIES was rewritten to append them to pc_LIBS on mongoc_static

This AUTO wasn't doing anything, and behaved very similar to plain ON. Now we define an interface library that provides the resolving library. Some cleanup to the ResSearch lookup module.

This change uses target properties to fill in the slots in the generated pkg-config files. Previously this was unstructured strings and flag juggling.

rcsanchez97

Only some comments and suggested changes so far. I'll take another look at this later, after I've had a chance to take it all in.

src/libmongoc/CMakeLists.txt

rcsanchez97 · 2023-06-15T18:05:02Z

src/libmongoc/CMakeLists.txt

+   # Delete "#~" comments:
+   string(REGEX REPLACE "#~[^\n]*" "" content "${content}")
+   # Replace our simple "%define <name> <value>" macros:
+   while(content MATCHES "(.*)\n *%define *([a-zA-Z_\\$<>:-]+) *([^\n]*)\n(.*)")


Suggested change

while(content MATCHES "(.*)\n *%define *([a-zA-Z_\\$<>:-]+) *([^\n]*)\n(.*)")

while(content MATCHES "(.*)\n *%define\s+([a-zA-Z_\\$<>:-]+)\s+([^\n]*)\n(.*)")

What do you think about a more permissive notion of whitespace and also making it 1..* rather than 0..*?

CMake's regex engine doesn't support shorthand character classes, unfortunately. Yes on the first +, but for the second I had an impulse to may %define nothing replace "nothing" with nothing a-la #define nothing.

src/libmongoc/CMakeLists.txt

src/libmongoc/src/libmongoc.pc.in

rcsanchez97 · 2023-06-15T18:28:56Z

src/libmongoc/src/mongoc/mongoc-client.c

@@ -101,7 +101,11 @@ _mongoc_client_killcursors_command (mongoc_cluster_t *cluster,
   } while (0)


-#ifdef MONGOC_HAVE_DNSAPI
+#if MONGOC_ENABLE_SRV == 0 // ↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓ ENABLE_SRV disabled


I'm assuming that you have to use == 0 rather than !defined because if it is assigned a value of 0 it is treated as being defined. Is that right?

// Given: #define FOO 0 #define BAR 1 // All the following conditions evaluate to true: // #ifdef FOO // #ifdef BAR // #ifndef BAZ // #if FOO == 0 // #if BAR != 0 // #if BAZ == 0

I feel like I knew that, yet I also feel like I would have overlooked it had I been implementing it. Perhaps I've been spending too much time on Python.

Aside: The "best" way to do boolean macros, imo: https://godbolt.org/z/zdsT77fTP

If the macro is undefined, you get a compilation error

If the macro is defined to any value other than those permitted, you get a compilation error.

IS_ON/IS_OFF is usable in both preprocessor directives as well as C conditionals

Co-authored-by: Roberto C. Sánchez <[email protected]>

CMakeLists.txt

build/cmake/ResSearch.cmake

src/libmongoc/CMakeLists.txt

eramongodb · 2023-06-15T18:47:07Z

src/libmongoc/CMakeLists.txt

+   set_target_properties (mongoc_static PROPERTIES
+      VERSION 0.0.0
+      OUTPUT_NAME "${MONGOC_OUTPUT_BASENAME}-static-${MONGOC_API_VERSION}"
+      pc_REQUIRES "libbson-static-1.0"


Consider PKG_CONFIG_ as prefix for pkg-config properties instead of pc_.

I can rename it to pkgconfig_ or pkg_config_, but I'll still keep it lowercase, because that's the under-documented way to "correctly" name custom properties. CMake arcana:

CMake won't let you use arbitrary properties on INTERFACE libraries. CMake will yell at you if you even look for a property that isn't "allowed" on an INTERFACE library. I think this restriction is obnoxious and pointless, and even if they remove(d?) it then there are still CMake executables out there that will yell at you.

Of the properties permitted: anything that starts with an underscore or a lowercase letter. I'm still scratching my head about it. Oh well.

Perhaps the property must be prefixed with INTERFACE_? From https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#compatible-interface-properties:

Some target properties are required to be compatible between a target and the interface of each dependency. For example, the POSITION_INDEPENDENT_CODE target property may specify a boolean value [...] To be "compatible", the POSITION_INDEPENDENT_CODE property, if set must be either the same, in a boolean sense, as the INTERFACE_POSITION_INDEPENDENT_CODE property of all transitively specified dependencies on which that property is set. This property of "compatible interface requirement" may be extended to other properties by specifying the property in the content of the COMPATIBLE_INTERFACE_BOOL target property. Each specified property must be compatible between the consuming target and the corresponding property with an INTERFACE_ prefix from each dependency: [...]

This is confusing (I know it comes from the documentation):

if set must be either the same, in a boolean sense, as the INTERFACE_POSITION_INDEPENDENT_CODE property of all transitively specified dependencies on which that property is set

They used "either" but there is no corresponding "or". I wonder if this is a result of a change where an "or" had been present, but removed, and the structure of the rest of the sentence wasn't updated.

The INTERFACE_ target properties are "whitelisted" for INTERFACE libraries, as well as the COMPATIBLE_INTERFACE_ properties, but those are to deal with transitive usage requirements when linking targets together, not per-target custom properties.

src/libmongoc/CMakeLists.txt

eramongodb · 2023-06-15T19:56:21Z

src/libmongoc/src/mongoc/mongoc-client.c

@@ -101,7 +101,11 @@ _mongoc_client_killcursors_command (mongoc_cluster_t *cluster,
   } while (0)


-#ifdef MONGOC_HAVE_DNSAPI
+#if MONGOC_ENABLE_SRV == 0 // ↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓ ENABLE_SRV disabled


// Given: #define FOO 0 #define BAR 1 // All the following conditions evaluate to true: // #ifdef FOO // #ifdef BAR // #ifndef BAZ // #if FOO == 0 // #if BAR != 0 // #if BAZ == 0

src/libmongoc/CMakeLists.txt

src/libmongoc/src/mongoc/mongoc-handshake.c

kevinAlbs

LGTM. The improved accuracy to handshake metadata is appreciated.

src/libmongoc/src/libmongoc.pc.in

src/libmongoc/CMakeLists.txt

src/libmongoc/src/libmongoc.pc.in

rschwebel · 2023-12-14T21:26:55Z

This pull request removes libmongoc-1.0.pc, but I couldn't find the reasoning. Does this mean that using libmongoc-ssl-1.0.pc is the only supported way? Thanks for clarification.

(I'm working on build system integration tasks right now, so I don't know much about the details of all this; however, this looks like using mongo-c-driver without ssl is deprecated, which seems reasonable to me; is my understanding correct?)

rschwebel · 2023-12-15T15:27:18Z

Forget my last comment, I tried 1.25.0 now and it properly generates a libmongoc-1.0.pc file.

vector-of-bool changed the title ~~Cdriver 4637 enable srv~~ CDRIVER-4637 Remove ENABLE_SRV=AUTO, clean up SRV URI support Jun 8, 2023

vector-of-bool force-pushed the CDRIVER-4637-ENABLE_SRV branch 2 times, most recently from 56cbbd2 to bf343ec Compare June 13, 2023 20:24

vector-of-bool added 4 commits June 13, 2023 20:28

Remove AUTO for ENABLE_SRV

a84fa5a

This AUTO wasn't doing anything, and behaved very similar to plain ON. Now we define an interface library that provides the resolving library. Some cleanup to the ResSearch lookup module.

Use ENABLE_SRV to toggle SRV support rather than detection macros

6abe0f1

Add distinct feature flag for SRV toggle.

18146ac

Refactor pkg-config generation to use target properties

ea2ef8a

This change uses target properties to fill in the slots in the generated pkg-config files. Previously this was unstructured strings and flag juggling.

vector-of-bool force-pushed the CDRIVER-4637-ENABLE_SRV branch from bf343ec to ea2ef8a Compare June 13, 2023 20:32

vector-of-bool added 2 commits June 13, 2023 21:17

Tweak method of detecting name resolution

2c37650

Rename intermediate interface libs

9e42bfc

vector-of-bool marked this pull request as ready for review June 13, 2023 21:18

Swap resolv search order for with/without a link

5ea027b

vector-of-bool requested review from kevinAlbs, eramongodb and rcsanchez97 June 13, 2023 21:36

Sppeling

db385dc

rcsanchez97 reviewed Jun 15, 2023

View reviewed changes

vector-of-bool and others added 2 commits June 15, 2023 13:10

Spelling and tpyos

db2d403

Co-authored-by: Roberto C. Sánchez <[email protected]>

Another one

bb3d49d

Co-authored-by: Roberto C. Sánchez <[email protected]>

eramongodb reviewed Jun 15, 2023

View reviewed changes

vector-of-bool added 2 commits June 15, 2023 15:08

PR cleanup and feedback

416e5f9

Regex tweak

52b881e

kevinAlbs approved these changes Jun 20, 2023

View reviewed changes

src/libmongoc/src/libmongoc.pc.in Outdated Show resolved Hide resolved

src/libmongoc/CMakeLists.txt Outdated Show resolved Hide resolved

src/libmongoc/src/libmongoc.pc.in Show resolved Hide resolved

Tweak .pc generation

d997d82

vector-of-bool requested review from eramongodb and rcsanchez97 June 21, 2023 18:01

eramongodb approved these changes Jun 26, 2023

View reviewed changes

vector-of-bool merged commit 0dc1a4e into mongodb:master Jun 26, 2023

vector-of-bool mentioned this pull request Jun 28, 2023

Cleanup pkg-config #1326

Merged

kevinAlbs mentioned this pull request Oct 25, 2023

Update NEWS for upcoming 1.25.0 release #1454

Merged

rschwebel mentioned this pull request Dec 14, 2023

Use pkg-config to find libraries and include files FIWARE/context.Orion-LD#1502

Merged

	while(content MATCHES "(.)\n %define ([a-zA-Z_\\$<>:-]+) ([^\n])\n(.)")
	while(content MATCHES "(.)\n %define\s+([a-zA-Z_\\$<>:-]+)\s+([^\n])\n(.)")

CDRIVER-4637 Remove ENABLE_SRV=AUTO, clean up SRV URI support #1301

CDRIVER-4637 Remove ENABLE_SRV=AUTO, clean up SRV URI support #1301

Uh oh!

Conversation

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

Uh oh!

Changes in this PR

Bits and Flags

Targets and CMake

pkg-config Changes

Uh oh!

rcsanchez97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rschwebel commented Dec 14, 2023

Uh oh!

rschwebel commented Dec 15, 2023

Uh oh!

Uh oh!

vector-of-bool commented Jun 8, 2023 •

edited

Loading