Skip to content

CXX-3266 avoid inheriting BUILD_VERSION by auto-downloaded C Driver #1373

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

Merged
merged 5 commits into from
Apr 8, 2025

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Apr 4, 2025

Resolves CXX-3266.

Important

This PR is only concerned with a C Driver project whose CMake targets are imported via add_subdirectory() instead of find_package(). This may either be an auto-downloaded C Driver (via FetchMongoC.cmake) or a manual call to add_subdirectory() by a higher-level CMake project.

The API version numbers embedded in the C Driver v2 install paths revealed suspicious behavior during local development and testing for CXX-3101. Given LIBMONGOC_DOWNLOAD_VERSION (+ related variables) set to 2.0.0 and using the latest commit (targeting a 4.1.0 release), the following install directory structure was observed (the API version for C Driver libraries is 0.0.0?!):

<prefix>
├── include
│   ├── bson-0.0.0
│   ├── mongoc-0.0.0
│   └── ...
└── lib
    ├── cmake
    │   ├── bson-0.0.0
    │   ├── mongoc-0.0.0
    │   └── ...
    ├── pkgconfig
    │   ├── bson0.pc
    │   ├── mongoc0.pc
    │   └── ...
    ├── libbson0.so.0.0.0
    ├── libmongoc0.so.0.0.0
    └── ...

This led down a rabbit hole of tracking down the root cause of this issue. Investigation revealed this bug is actually present since C++ Driver 3.9.0, which introduced auto-download support in #967 (CXX-2695). However, it was not observed due to the 3.9.0 release auto-downloading C Driver 1.25.0, which temporarily disabled support for BUILD_VERSION as a config option in mongodb/mongo-c-driver#1382 (CDRIVER-4777). Support was restored in 1.26.0 by mongodb/mongo-c-driver#1462 (CDRIVER-4763), but the C++ Driver continued to use 1.25.0 until #1162 (CXX-2828) in the 3.11.0 release.

Important

This bug is present since C++ Driver 3.9.0 when the auto-downloaded C Driver was introduced.

Important

This bug is present since C Driver 1.14.0 when BUILD_VERSION was introduced, except for 1.25.0 when support for BUILD_VERSION as a CMake config option was temporarily removed, which also happened to be the version auto-downloaded by C++ Driver 3.9.0.

The problem is twofold. The C Driver may incorrectly inherit BUILD_VERSION during initial configuration:

# Initial configuration with BUILD_VERSION=5.0.0.
$ cmake -S . -B build -D BUILD_VERSION=5.0.0

# Auto-downloaded C Driver incorrectly inherited BUILD_VERSION.
$ grep -Rho --include 'bson-version.h' '#define BSON_.*VERSION (.*' build
#define BSON_MAJOR_VERSION (5)
#define BSON_MINOR_VERSION (0)
#define BSON_MICRO_VERSION (0)
#define BSON_PRERELEASE_VERSION ()
#define BSON_VERSION (2.0.0)

or on subsequent reconfiguration using an existing CMake cache:

# Initial configuration without BUILD_VERSION.
$ cmake -S . -B build

# Auto-downloaded C Driver computed its version correctly (for now...).
$ grep -Rho --include 'bson-version.h' '#define BSON_.*VERSION (.*' build
#define BSON_MAJOR_VERSION (2)
#define BSON_MINOR_VERSION (0)
#define BSON_MICRO_VERSION (0)
#define BSON_PRERELEASE_VERSION ()
#define BSON_VERSION (2.0.0)

# Reconfigure the project without making any changes.
$ cmake -S . -B build

# Auto-downloaded C Driver incorrectly inherited BUILD_VERSION.
$ grep -Rho --include 'bson-version.h' '#define BSON_.*VERSION (.*' build
#define BSON_MAJOR_VERSION (0)
#define BSON_MINOR_VERSION (0)
#define BSON_MICRO_VERSION (0)
#define BSON_PRERELEASE_VERSION ()
#define BSON_VERSION (2.0.0)

Note also the confusing mix of version numbers, which depend on whether the computation was derived from the BUILD_VERSION cache variable (via BuildVersion.cmake) or from the C Driver's own copy of the VERSION_CURRENT file (via LoadVersion.cmake).

This problem is further exacerbated by the following conditions:

  • configuration output suggests the correct C Driver version is being computed and used: emits libmongoc version (from VERSION_CURRENT file): 2.0.0 despite bad version numbers.
  • requiring a specific configuration which selects auto-downloaded C Driver with either BUILD_VERSION set or reconfiguration step to trigger reuse of the BUILD_VERSION cache variable.
  • a deliberate(?) lack of version checks when C Driver targets are imported via add_subdirectory().
  • the absence of API version checks in CMake dependency requirements for downstream projects (no <version> included in call to find_dependency(bson-1.0) in bsoncxx-config.cmake).
  • the absence of API version numbers in directory or file names (to make the discrepancy more immediately obvious on inspection of the install directory contents).
  • the absence of auto-downloaded C Driver installation integrity checks as part of the C++ Driver's EVG configuration.
  • most of our EVG config having migrated to using a pre-installed C Driver with find_package() (for CSFLE support) rather than the auto-downloaded C Driver (only used by benchmarks, macro guard tests, mongohouse tests, scan-build tests, uninstall checks, and versioned API tests).

Most of these issues will be addressed in a followup PR as part of CXX-3101.

The changes in PR are planned to be cherry-picked onto the v4.0 and v3.11 release branches.

@eramongodb eramongodb self-assigned this Apr 4, 2025
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor comments. Thank you for investigating and fixing. I like the use of a function scope to isolate the normal CMake variables.

Comment on lines 45 to 48
# Restore prior value of BUILD_VERSION only when they were previously set.
if(DEFINED OLD_BUILD_VERSION)
set(BUILD_VERSION ${OLD_BUILD_VERSION})
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Restore prior value of BUILD_VERSION only when they were previously set.
if(DEFINED OLD_BUILD_VERSION)
set(BUILD_VERSION ${OLD_BUILD_VERSION})
endif()
# Restore prior value of cached BUILD_VERSION only when previously set.

IIUC restoring the normal variable is not needed, since the unset(BUILD_VERSION) will not impact the parent scope.


message(STATUS "Download and configure C driver version ${LIBMONGOC_DOWNLOAD_VERSION} ... end")
function(fetch_mongoc)
message(STATUS "Download and configure C driver version ${LIBMONGOC_DOWNLOAD_VERSION} ... end")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
message(STATUS "Download and configure C driver version ${LIBMONGOC_DOWNLOAD_VERSION} ... end")
message(STATUS "Download and configure C driver version ${LIBMONGOC_DOWNLOAD_VERSION} ... begin")

FetchContent_Declare(
mongo-c-driver
GIT_REPOSITORY https://github.com/mongodb/mongo-c-driver.git
GIT_TAG ${MONGOC_DOWNLOAD_VERSION}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
GIT_TAG ${MONGOC_DOWNLOAD_VERSION}
GIT_TAG ${LIBMONGOC_DOWNLOAD_VERSION}

This may fix failing tasks that seem to be using the auto-downloaded C driver. Example.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (+1 to Kevin's comments)

@eramongodb eramongodb merged commit 4d2be83 into mongodb:master Apr 8, 2025
6 of 9 checks passed
@eramongodb eramongodb deleted the cxx-3266 branch April 8, 2025 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants