-
Notifications
You must be signed in to change notification settings - Fork 546
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
Conversation
There was a problem hiding this 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.
cmake/FetchMongoC.cmake
Outdated
# Restore prior value of BUILD_VERSION only when they were previously set. | ||
if(DEFINED OLD_BUILD_VERSION) | ||
set(BUILD_VERSION ${OLD_BUILD_VERSION}) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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.
cmake/FetchMongoC.cmake
Outdated
|
||
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message(STATUS "Download and configure C driver version ${LIBMONGOC_DOWNLOAD_VERSION} ... end") | |
message(STATUS "Download and configure C driver version ${LIBMONGOC_DOWNLOAD_VERSION} ... begin") |
cmake/FetchMongoC.cmake
Outdated
FetchContent_Declare( | ||
mongo-c-driver | ||
GIT_REPOSITORY https://github.com/mongodb/mongo-c-driver.git | ||
GIT_TAG ${MONGOC_DOWNLOAD_VERSION} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this 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)
Resolves CXX-3266.
Important
This PR is only concerned with a C Driver project whose CMake targets are imported via
add_subdirectory()
instead offind_package()
. This may either be an auto-downloaded C Driver (viaFetchMongoC.cmake
) or a manual call toadd_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 to2.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 is0.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 forBUILD_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:or on subsequent reconfiguration using an existing CMake cache:
Note also the confusing mix of version numbers, which depend on whether the computation was derived from the
BUILD_VERSION
cache variable (viaBuildVersion.cmake
) or from the C Driver's own copy of theVERSION_CURRENT
file (viaLoadVersion.cmake
).This problem is further exacerbated by the following conditions:
libmongoc version (from VERSION_CURRENT file): 2.0.0
despite bad version numbers.BUILD_VERSION
set or reconfiguration step to trigger reuse of theBUILD_VERSION
cache variable.add_subdirectory()
.<version>
included in call tofind_dependency(bson-1.0)
inbsoncxx-config.cmake
).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.