CDRIVER-3433 support linking against windows static runtime #591
Conversation
This allows dependent projects to optionally enable overriding the default MSVC runtime (`/MD` for the dynamic mulitthreaded) to use the static runtime (`/MT`)
rcsanchez97 left a comment
rcsanchez97 left a comment There was a problem hiding this comment.
LGTM.
Question: is it your intent to have some Evergreen task that regularly exercises this option? If not, perhaps we want to consider adding here in the C driver project. That, however, can be done later on.
| @rcsanchez97 The build for the node addon will implicitly test this, but no I had not otherwise considered adding something to test it explicitly. |
| Since the node addon will be the primary (only?) user of this feature, are you comfortable with that implicit testing? Or would you prefer that we have some explicit testing in the C driver Evergreen builds? |
| @rcsanchez97 I'm totally fine with the addon being the lone tester for now |
kevinAlbs left a comment
kevinAlbs left a comment There was a problem hiding this comment.
I think it's worth adding an evergreen task in the mongo-c-driver project to test this option, similar to our link-with-x tasks. But I think we can defer that to a separate ticket.
| target_include_directories (bson_static INTERFACE $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src> $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/src>) | ||
| set_target_properties (bson_static PROPERTIES VERSION 0.0.0) | ||
| set_target_properties (bson_static PROPERTIES OUTPUT_NAME "${BSON_OUTPUT_BASENAME}-static-${BSON_API_VERSION}") | ||
| if (ENABLE_WINDOWS_STATIC_RUNTIME) |
There was a problem hiding this comment.
Can this be changed to a cached option? Similar to options here: https://github.com/mongodb/mongo-c-driver/blob/1.15.0/CMakeLists.txt/#L57
A cached option will show up after configuring with cmake, ending up in sort of a help menu. Users can get a list of cached options along with the help text by running cmake -LH or using cmake GUI or ccmake.
Arguably, the same should be done for mongodb/libmongocrypt#78, but I think it's much less likely anyone outside of our drivers team is using libmongocrypt directly.
There was a problem hiding this comment.
I'm not familiar with CMake, but should this option only exist on Windows platforms?
Coming from an Autotools perspective, I am imagining that this behaves similar to a configure option, where we'd simply prefer to have it omitted entirely from the list of supported options if we know it's not relevant to the platform. If that's not the case for CMake, please correct me.
There was a problem hiding this comment.
Sorry, some discussion/investigation has happened outside of this PR and I did not follow-up. This PR can hopefully be closed without merging. I was able to pass /MT through the relwithdebinfo Windows compile using -DCMAKE_C_FLAGS_RELWITHDEBINFO. @mbroadst if you can verify the changes on this branch work (https://github.com/kevinAlbs/libmongocrypt/tree/proposed-fix), shall we close this PR?
Though I do agree, if we did add this option, it'd probably make sense to have it Windows only.
| closing because @kevinAlbs found a way to do this without changing libbson or libmongocrypt ✌️ |
This allows dependent projects to optionally enable overriding the
default MSVC runtime (
/MDfor the dynamic mulitthreaded) to usethe static runtime (
/MT)