Skip to content
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

[ceres] Should not set CMAKE_CXX_STANDARD #22980

Closed
reynoldsbd opened this issue Feb 7, 2022 · 8 comments · Fixed by #22983
Closed

[ceres] Should not set CMAKE_CXX_STANDARD #22980

reynoldsbd opened this issue Feb 7, 2022 · 8 comments · Fixed by #22983
Assignees
Labels
category:port-bug The issue is with a library, which is something the port should already support

Comments

@reynoldsbd
Copy link
Member


Description of problem

The portfile for Ceres contains the following logic:

file(READ ${CURRENT_PACKAGES_DIR}/share/ceres/CeresConfig.cmake CERES_CONFIG)
string(REPLACE "set_target_properties(ceres PROPERTIES INTERFACE_LINK_LIBRARIES Ceres::ceres)"
"set_target_properties(ceres PROPERTIES INTERFACE_LINK_LIBRARIES Ceres::ceres)
set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)" CERES_CONFIG "${CERES_CONFIG}")
file(WRITE ${CURRENT_PACKAGES_DIR}/share/ceres/CeresConfig.cmake "${CERES_CONFIG}")

This can create issues for users who import Ceres into their projects using find_package(Ceres), because this unconditionally and globally overwrites the CMAKE_CXX_STANDARD variable to the value 14. As a result, consumers who wish to use a different C++ standard are forced to manually (re)set this variable after including Ceres.

This behavior appears to have been introduced here, as part of #12785.

Proposed solution

Modify the Ceres portfile to stop injecting these lines. vcpkg should not modify the user's CMAKE_CXX_STANDARD variable. Provided there is agreement, I am happy to submit such a PR.

Note that the upstream Ceres project already enforces a minimum C++ standard using a more idiomatic and targeted CMake mechanism.

Describe alternatives you've considered

I am currently able to work around this issue by manually resetting CMAKE_CXX_STANDARD after including Ceres:

set(orig_cxx_std ${CMAKE_CXX_STANDARD})
find_package(Ceres REQUIRED)
set(CMAKE_CXX_STANDARD ${orig_cxx_std})

Additional context

This behavior appears to have been introduced by #12785. cc @cenit, @JackBoosY, @BillyONeal

@BillyONeal BillyONeal changed the title [ceres] Do not set CMAKE_CXX_STANDARD [ceres] Should not set CMAKE_CXX_STANDARD Feb 7, 2022
@BillyONeal
Copy link
Member

Timeline:

This means that whatever problem #14719 was trying to fix seems to not be able to use that for some reason.

@cenit Can you comment as to why that specific change was made?

@BillyONeal BillyONeal added the category:port-bug The issue is with a library, which is something the port should already support label Feb 7, 2022
@BillyONeal
Copy link
Member

I observe that the target_compile_options upstream is trying to do does not make it into the exported targets because they set cmake_minimum_required(VERSION 3.5) but the ability to do that seems to have been added in 3.8 or 3.11: https://cmake.org/cmake/help/latest/command/target_compile_options.html#arguments

@BillyONeal
Copy link
Member

Hmmm nvm I missed that I guess. The target_compile_features is present in both.

BillyONeal added a commit to BillyONeal/vcpkg that referenced this issue Feb 7, 2022
Resolves microsoft#22980

microsoft#12785 Added a set of `CMAKE_CXX_STANDARD` in this CMake config which accidentally overwrites higher versions. Upstream already has a `target_compile_options` to do this the Right Way.

This change:
* Deletes the offending attempt to set CMAKE_CXX_STANDARD. Downstream users that don't listen to the INTERFACE_COMPILE_FEATURES need to be patched locally.
* Modernizes to use vcpkg_cmake_Xxx.
* Removes attempt to fix up paths that already appears handled by `vcpkg_cmake_config_fixup`.
* Adds quotes.
@cenit
Copy link
Contributor

cenit commented Feb 7, 2022

Timeline:

This means that whatever problem #14719 was trying to fix seems to not be able to use that for some reason.

@cenit Can you comment as to why that specific change was made?

i think that the target_compile_options were not present at the time of my #12785 PR in that current ceres version, and upgrade to v2 in #14719 didn't remove the "quick and dirty" patch that was forcing c++14 on a downstream project

@BillyONeal
Copy link
Member

i think that the target_compile_options were not present at the time of my #12785 PR in that current ceres version, and upgrade to v2 in #14719 didn't remove the "quick and dirty" patch that was forcing c++14 on a downstream project

I think it was in there, because it was done in 2018 but the version we were using when your changes were made was in 2020. I'm worried about accidentally dropping what you were doing on the floor. If you don't remember what it was and CI is passing, I'm just going to go with it?

@cenit
Copy link
Contributor

cenit commented Feb 7, 2022

d66679f

the commit i did says that it was necessary for downstream ports using classic target and not the namespaced one. And that is coherent with the position inside the config file.

With vcpkg-ci-opencv not tested in many configs, the vtk module that required it is not tested too, and so i am not sure about the ci green reliability...

@BillyONeal
Copy link
Member

the commit i did says that it was necessary for downstream ports using classic target and not the namespaced one. And that is coherent with the position inside the config file.

That's strange to me since the classic target should be doing target_link_libraries (effectively) to the namespaced one.

@BillyONeal
Copy link
Member

With vcpkg-ci-opencv not tested in many configs, the vtk module that required it is not tested too, and so i am not sure about the ci green reliability...

What ports would you like me to manually test?

I know of:

  • vcpkg-ci-opencv:ALL_THE_TRIPLETS ?

@JackBoosY JackBoosY self-assigned this Feb 8, 2022
BillyONeal added a commit that referenced this issue Feb 9, 2022
Resolves #22980

#12785 Added a set of `CMAKE_CXX_STANDARD` in this CMake config which accidentally overwrites higher versions. Upstream already has a `target_compile_options` to do this the Right Way.

This change:
* Deletes the offending attempt to set CMAKE_CXX_STANDARD. Downstream users that don't listen to the INTERFACE_COMPILE_FEATURES need to be patched locally.
* Modernizes to use vcpkg_cmake_Xxx.
* Removes attempt to fix up paths that already appears handled by `vcpkg_cmake_config_fixup`.
* Adds quotes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants