-
Notifications
You must be signed in to change notification settings - Fork 547
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
kevinAlbs
left a comment
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.
vector-of-bool
left a comment
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.0and 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_VERSIONas 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_VERSIONwas introduced, except for 1.25.0 when support forBUILD_VERSIONas 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_VERSIONduring 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_VERSIONcache variable (viaBuildVersion.cmake) or from the C Driver's own copy of theVERSION_CURRENTfile (viaLoadVersion.cmake).This problem is further exacerbated by the following conditions:
libmongoc version (from VERSION_CURRENT file): 2.0.0despite bad version numbers.BUILD_VERSIONset or reconfiguration step to trigger reuse of theBUILD_VERSIONcache 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.