[mongo-cxx-driver] Add C++17 polyfill option (#13607)

[stalkyr]

Fixes #13607

[microsoft-cla-retired[bot]]

All CLA requirements met.

[JackBoosY]

Sadly, according to our policy, we shouldn't add any feature about the c/cxx standard. You can use overlay-port to use this feature locally. Sorry for that.

[stalkyr]

Hi @JackBoosY ,

I believe this PR fit in this guideline, section A feature may replace polyfills with aliases provided that replacement is baked into the installed tree

mongo-cxx-driver have provide several features for polyfills already, and this PR remove polyfills to use C++17 standard. It didn't change API nor add functionality.

Regards,

[JackBoosY]

@stalkyr See https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#do-not-use-features-to-control-alternatives-in-published-interfaces

[stalkyr]

@JackBoosY, please see next paragraph A feature may replace polyfills with aliases provided that replacement is baked into the installed tree Notwithstanding the above, ports may remove polyfills with a feature, as long as:

compile flag BSONCXX_POLY_USE_STD=1 will typedef an alias std::string_view for bsoncxx::stdx::string_view which fits

1. Turning on the feature changes the polyfills to aliases of the polyfilled entity

[JackBoosY]

@JackBoosY, please see next paragraph A feature may replace polyfills with aliases provided that replacement is baked into the installed tree Notwithstanding the above, ports may remove polyfills with a feature, as long as:

compile flag BSONCXX_POLY_USE_STD=1 will typedef an alias std::string_view for bsoncxx::stdx::string_view which fits

1. Turning on the feature changes the polyfills to aliases of the polyfilled entity

Any way, it's an option of this port and the user's compiler may not support cxx17.

[BillyONeal]

How is the polyfill setting baked into the installed bits? See https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#a-feature-may-replace-polyfills-with-aliases-provided-that-replacement-is-baked-into-the-installed-tree for more details.

[JackBoosY]

Ping for response.

[stalkyr]

I didn't get it

mongo-cxx-driver already have built-in defines for selecting polyfills, no need to patch it

just need to add a vcpkg feature for it, and this PR just doing it

[BillyONeal]

I didn't get it

mongo-cxx-driver already have built-in defines for selecting polyfills, no need to patch it

just need to add a vcpkg feature for it, and this PR just doing it

The reason this needs to be baked into the installed bits is because the C++ version selection is not necessarily the same for downstream consumers of the vcpkg "installed" tree as it is when building the port, which usually needs to match. The port needs to force the setting to match for users (for example, with a patch) that force enables all the polyfills when the feature is on and force disables them (thus requiring '17) when the feature is off.

[stalkyr]

I see

But look further into mongo-cxx-driver upstream cmake files, they already done the baked part

Prove: see .../vcpkg_installed/x64-linux/include/bsoncxx/config/config.hpp the polyfill selection already baked into header

so only thing left to do is add vcpkg feature option

[BillyONeal]

But look further into mongo-cxx-driver upstream cmake files, they already done the baked part

I believe what upstream is doing is trying to inspect __cplusplus, which means the user's selected standard version needs to match the feature selection, making it an alternative and not a feature.

[stalkyr]

vcpkg/ports/mongo-cxx-driver/portfile.cmake, around line 47

vcpkg_cmake_configure(
    SOURCE_PATH "${SOURCE_PATH}"
    OPTIONS
        -DMONGOCXX_HEADER_INSTALL_DIR=include
        -DBSONCXX_HEADER_INSTALL_DIR=include
        -DBSONCXX_POLY_USE_${BSONCXX_POLY}=1
        -DCMAKE_CXX_STANDARD=${BSONCXX_STANDARD}
        -DBUILD_VERSION=${VERSION_FULL}
)

polyfill choose by BSONCXX_POLY_USE_${BSONCXX_POLY} and ${BSONCXX_STANDARD}, not auto detected. and these two variables set by vcpkg feature at around line 21

as you can see, there were already features selecting polyfill, I have no idea why refuse to add one more. And this is the standard std one.

[JackBoosY]

Unless this port mandates the use of cxx17, it is our policy not to add a c/cxx standard feature.

[BillyONeal]

polyfill choose by BSONCXX_POLY_USE_${BSONCXX_POLY} and ${BSONCXX_STANDARD}, not auto detected. and these two variables set by vcpkg feature at around line 21 as you can see, there were already features selecting polyfill, I have no idea why refuse to add one more. And this is the standard std one.

There are a lot of features like this that we shouldn't have ever added, because features can't meaningfully be alternatives. I was looking for a pointer to how this is locked in upstream; the generated code seems to do that:

I found https://github.com/mongodb/mongo-cxx-driver/blob/1afd5102fd63322fa448f490d5f0dbaee78374bb/src/bsoncxx/config/config.hpp.in#L15 which seems to be the right thing.

(I wasn't necessarily saying that it was incorrect, only that I was looking for proof that it was correct)

Is the other part (that it is possible to write downstream that doesn't care about the feature) true?

Unless this port mandates the use of cxx17, it is our policy not to add a c/cxx standard feature.

Features like this can be OK, see https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#a-feature-may-replace-polyfills-with-aliases-provided-that-replacement-is-baked-into-the-installed-tree -- abseil 's cxx17 feature is explicitly called out as acceptable. The port needs to meet all 3 requirements there to do it.

[stalkyr]

A bit off topic, but I think the no-cxx17-in-feature policy make cxx17 user a hard time using vcpkg.

Right now I have to write overlay for every libraries need flags to turn on cxx17 support, and I need to review them when updating vcpkg.

Is there another facility to specify language spec?

[JackBoosY]

@stalkyr Maybe you should make a new triplet and set cxx17 as the cxx standard in the toolchain file instead.

[stalkyr]

A new triplet makes sense and will be very helpful.

But I cannot find any VCPKG_XXXX variable to set cxx standard ( https://github.com/microsoft/vcpkg/blob/master/docs/users/triplets.md ) If such variable exists, I can make a patch to detect it in port file and set build defines without feature options

[JackBoosY]

Maybe you need this: https://github.com/microsoft/vcpkg/blob/3f71620c2b348874346a6e78b3de32a53eb23248/docs/users/triplets.md#vcpkg_cxx_flags

[stalkyr]

set(VCPKG_CXX_FLAGS -std=c++17) (or set(VCPKG_CXX_FLAGS /std:c++17) in vc++) is not standard way to set language standard, it will have issue like this: https://github.com/microsoft/vcpkg/issues/8181

I wish there will be a set(VCPKG_CXX_STANDARD 17) which translate to set_properties(TARGET target PROPERTIES CXX_STANDARD 17) in CMakeLists.txt

Or will triplet file support set(CMAKE_CXX_STANDARD 17) directly? Which affect all CMakeLists.txt globally.

[JackBoosY]

@stalkyr Normally we should not set CXX_STANDARD/ C_STANDARD because the current compiler may not support it. The settings in triplet should be generic.

[stalkyr]

afaik, all modern compilers have default setting files to translate cmake variable CXX_STANDARD and CXX_EXTENSIONS to it's own language standard flags correctly.

Regardless, triplet variable set(VCPKG_CXX_STANDARD 17) is only used to build custom libraries. If someone's compiler did not support it, they can choose not using it.

VCPKG is a c++ package manager, setting language standard seems quite essential to me.

[JackBoosY]

@stalkyr For a compiled package manager, the user environment that uses it is diverse, and the language standards supported by the compiler are also different, so we cannot force users to use some newer language standards. But we don't prohibit you from adding this setting to the triplet.

[stalkyr]

@JackBoosY I have write my thought and idea in https://github.com/microsoft/vcpkg/discussions/26226 , let's discuss there

[JackBoosY]

Before we have the conclusion, I think we should keep this PR close.