Skip to content

[boost] stop renaming libs and add has_synchro for boost-atomic#27694

Closed
Neumann-A wants to merge 76 commits intomicrosoft:masterfrom
Neumann-A:fix_boost_clang_cl
Closed

[boost] stop renaming libs and add has_synchro for boost-atomic#27694
Neumann-A wants to merge 76 commits intomicrosoft:masterfrom
Neumann-A:fix_boost_clang_cl

Conversation

@Neumann-A
Copy link
Copy Markdown
Contributor

@Neumann-A Neumann-A commented Nov 7, 2022

The current behavior does:
a) leads to irritation about the lib name
b) doesn't work with clang-cl
c) Is wrong in that regard that VS2015 could use boost build with VS2022 which certainly will end in linker errors.....

supersedes #27693, #26337

(somebody using mingw might want to look into the mingw naming but in this PR i didn't touch that.)

github-actions[bot]
github-actions bot previously approved these changes Nov 7, 2022
@ras0219-msft
Copy link
Copy Markdown
Contributor

ras0219-msft commented Nov 7, 2022

Without opening a question of dropping support for older CMake versions, additional requirements apply for the DLL naming:

All of the above must work out of the box without any user customization

@Neumann-A
Copy link
Copy Markdown
Contributor Author

Users of newer MSVC toolsets (e.g. v143) MUST be able to find and link against Boost binaries built with older MSVC (e.g. v140)

Do you now which CMake version VS shipped with for VS 2022; 2019 and 2017 ?

@Neumann-A
Copy link
Copy Markdown
Contributor Author

apply for the DLL naming:

you mean LIB naming. vcpkg doesn't touch the dlls.

Users can use the built-in FindBoost.cmake module in CMake Version 3.10:
Users can use the built-in FindBoost.cmake module in CMake Version 3.11:

if (NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 19.10)
      set(_boost_COMPILER "-vc141;-vc140")
    elseif (NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 19)
      set(_boost_COMPILER "-vc140")

vcpkg shouldn't support FindBoost with VS 2022 and e.g. CMake 3.10. (VS 2022 was only added in 3.21). This means max VS 2017 which is full filled by FindBoost . But the upstream model only supports boost up to 1.71 in 3.11 so maybe drop that requirement since it is unrealistic considering boost in vcpkg is at 1.80 and would logically require a newer CMake anyway (since vcpkg removes the version from the libs files it will probably find it regardless.).

Users can use the built-in FindBoost.cmake module in CMake Version 3.24.3 (latest):

that is nearly tested by CI using it. (Increase CI CMake to match latest CMake? Or is that good enough?)

Users of newer MSVC toolsets (e.g. v143) MUST be able to find and link against Boost binaries built with older MSVC (e.g. v140)

that should be handled by https://github.com/Kitware/CMake/blob/c974557598645360fbabac71352b083117e3cc17/Modules/FindBoost.cmake#L879-L881

Users must find Release and Debug files correctly for multi-config generators

only relevant change considering that is:
set(BOOST_LIB_DEBUG_SUFFIX .lib) # Note: FindBoost.cmake will not look for d suffixed libraries
I can make that case an error instead since that case should never happen anyway.
All other code paths don't change the behavior.

Users must find static or dynamic libraries based on the vcpkg build settings

there is no change in this PR which would change that

@ras0219-msft
Copy link
Copy Markdown
Contributor

First I want to clarify that I was not attempting to only list items I believed this PR would break. This (renaming boost libs) has often been a topic of discussion before (as you listed -- "leads to irritation about the lib name") and so I wanted to provide a more complete picture of what our requirements are.

Do you now which CMake version VS shipped with for VS 2022; 2019 and 2017 ?

The last VS2017 ships with 3.12 (thanks @BillyONeal for installing it in a VM and checking!), however the existing comments[1] clearly indicate an intent to work with 3.10 or below. That's why I started with "Without opening a question of dropping support for older CMake versions" -- the current implementation claims support for at "least" 3.10, so dropping that would clearly be a changed support intent.

But the upstream model only supports boost up to 1.71...

While CMake's FindBoost module does emit warnings for newer Boost versions, I have personally been extremely successful in the past when using a newer boost than what the CMake version explicitly checked. I believe the two potential issues are 1) new components won't be listable and 2) inter-dependencies with compiled modules may not be accurate. New components are not widespread by definition (they're new) and the inter-dependencies between compiled modules do not change very much from version to version.

that is nearly tested by CI using it. (Increase CI CMake to match latest CMake? Or is that good enough?)

Yep, I think that's good enough. I just wanted to be thorough in my list of "requirements".

that should be handled by https://github.com/Kitware/CMake/blob/c974557598645360fbabac71352b083117e3cc17/Modules/FindBoost.cmake#L879-L881

Those lines were added in CMake 3.15 and so relying on them breaks 3.11 3.12 / 3.10 requirement.
Kitware/CMake@717e854

[1]: string(REPLACE "-x32-" "-" NEW_FILENAME ${NEW_FILENAME}) # To enable CMake 3.10 and earlier to locate the binaries
[2]: https://github.com/Kitware/CMake/blob/v3.10.0/Modules/FindBoost.cmake#L1674

@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label Nov 8, 2022
@Neumann-A
Copy link
Copy Markdown
Contributor Author

the current implementation claims support for at "least" 3.10, so dropping that would clearly be a changed support intent.

You cannot use vs142/3 with CMake 3.10 so there is no need to support that "use case" since it doesn't exist. Support for 3.10 is only given for VS2015/17.

Those lines were added in CMake 3.15 and so relying on them breaks 3.11 3.12 / 3.10 requirement.

being a vs143 user already requires to be a 3.21 user. (https://cmake.org/cmake/help/latest/generator/Visual%20Studio%2017%202022.html)

being a vs142 user requires to be at least a 3.14 user and that was based on a experimental VS release according to the release notes; so I am more in less in line to consider 3.15 the official supported version for vs142 ;). Especially since upstream CMake doesn't touch Find Modules unless stuff has been released (which I learned from trying to update FindMATLAB with a newer version before it was released.)
(https://cmake.org/cmake/help/latest/generator/Visual%20Studio%2016%202019.html)

So the real question here is if VS2019 really shipped with CMake 3.14 on release with an obviously broken FindBoost.cmake for that version.

github-actions[bot]
github-actions bot previously approved these changes Nov 8, 2022
github-actions[bot]
github-actions bot previously approved these changes Nov 10, 2022
@Neumann-A
Copy link
Copy Markdown
Contributor Author

So I think i solved the CMake FindBoost issue by simply downloading the 3.24.3 version and forcing it via CMAKE_MODULE_PATH.

JonLiu1993
JonLiu1993 previously approved these changes May 17, 2023
@JonLiu1993 JonLiu1993 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label May 17, 2023
@JavierMatosD
Copy link
Copy Markdown
Contributor

Hi @Neumann-A, I've taken the liberty of splitting out the has_snchro bits -> #30836. I'll place this PR on draft status until the other PR lands then readdress the lib renaming stuff.

I do apologies for the slow turnaround on this PR.

@JavierMatosD JavierMatosD removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label May 18, 2023
@JavierMatosD JavierMatosD marked this pull request as draft May 18, 2023 21:28
@JonLiu1993 JonLiu1993 removed the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jun 14, 2023
JavierMatosD added a commit that referenced this pull request Jun 16, 2023
…pkg/pull… (#30836)

* pulling has_synchro bits from #27694

* bump port version

* version

* fix clang

* bump version

* version

* oops

* bump dependency version for boost-atomic's boost-module-build-helper dependency

* version

* revert renaming stuff -.-

* version

* remove detection stuff for lib prefix/suffix

* version

* fix indentation

* version

* forgot one

* version

* one more!

* version

* revert changes to cmake-get-vars

* version

* generate ports

* version db

* generate ports

* version db

---------

Co-authored-by: Alexander Neumann <30894796+Neumann-A@users.noreply.github.com>
@JavierMatosD JavierMatosD dismissed JonLiu1993’s stale review June 16, 2023 23:30

The merge-base changed after approval.

JavierMatosD and others added 9 commits June 16, 2023 16:40
# Conflicts:
#	ports/boost-atomic/vcpkg.json
#	ports/boost-modular-build-helper/boost-modular-build.cmake
#	ports/boost-modular-build-helper/vcpkg.json
#	ports/boost/vcpkg.json
#	scripts/boost/generate-ports.ps1
#	scripts/test_ports/cmake-user/vcpkg.json
#	scripts/test_ports/vcpkg-ci-boost/vcpkg.json
#	versions/b-/boost-atomic.json
#	versions/b-/boost-modular-build-helper.json
#	versions/b-/boost-uninstall.json
#	versions/b-/boost.json
#	versions/baseline.json
#	versions/v-/vcpkg-cmake-get-vars.json
@Neumann-A
Copy link
Copy Markdown
Contributor Author

Discussion is currently in https://github.com/microsoft/vcpkg/pull/27694/files#r1238076280 which isn't directly visible from the main page

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 this pull request may close these issues.

7 participants