vcpkg icon indicating copy to clipboard operation
vcpkg copied to clipboard

[boost-modular-build-helper] use system layout

Open vitalyster opened this issue 4 years ago • 21 comments

Describe the pull request Boost layout switched to system

  • What does your PR fix?

    This will allow linking boost libraries on Windows without guessing compiler and other suffixes, just like on any other platform

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

vitalyster avatar Feb 08 '22 22:02 vitalyster

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

:x: vitalyster sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

The reason we do not use the system layout on Windows is because it is incompatible with Multi-Configuration CMake Generators like MSBuild while using the built-in FindBoost.cmake module.

The FindBoost module needs to be able to find both the release and debug library files for Multi-Config Generators to work, but the system layout does not have any naming difference between them. This is why our current approach is to "pin" all the relevant values (compiler, etc) such that FindBoost is able to find them while also being able to distinguish between Release and Debug.

Can you please go into detail about the precise problem this PR resolves including repro steps? Thanks for the PR!

ras0219-msft avatar Feb 09 '22 21:02 ras0219-msft

find both the release and debug library files for Multi-Config Generators

Everything works just fine, tested with minimal CMake project: Release directory contains release version of boost dlls and Debug directory contains Debug dlls, no need to have these creepy suffixes.

precise problem this PR resolves

This PR resolves problem with other build systems: without system layout each build system should implement giant FindBoost.cmake analog for no reason.

vitalyster avatar Feb 09 '22 21:02 vitalyster

  1. Most of ports worked perfectly without suffixes, some ports like Boost have that only for legacy reasons which are not actual for vcpkg in my opinion.

vitalyster avatar Feb 09 '22 21:02 vitalyster

@vitalyster You don't understand the problem. Multi Config cmake generators need the suffix to be able to correctly link against the libraries and generate correctly setup <package>-target.cmake files. If it is the same name CMake can only find one version of the libraries on its own.

Neumann-A avatar Feb 09 '22 21:02 Neumann-A

@Neumann-A why it works for me and for thousands of other ports?

vitalyster avatar Feb 09 '22 22:02 vitalyster

why it works for me and for thousands of other ports?

Because you don't know where the issues are and don't understand the multi config issues / <package>-target.cmake generation issue. If you only ever use single configuration stuff and consumers link against the boost target instead of the _LIBRARY vars you probably never see these issue. But those issues exists and are annoying. At least the d.lib must stay for cmake to do the right thing. (I really don't care about the rest of the suffix)

Neumann-A avatar Feb 09 '22 22:02 Neumann-A

@Neumann-A I tested a multi-config CMake build and it works. If you know some issues that can happen - please provide an example.

vitalyster avatar Feb 09 '22 22:02 vitalyster

At least the d.lib

I see the only reason to have a suffix is when all libraries are stored in a single directory. But in the vcpkg world debug libraries are stored in debug directory and it is enough to distinct debug and release binaries

vitalyster avatar Feb 09 '22 22:02 vitalyster

I tested a multi-config CMake build and it works.

probably only tested debug config.....

I see the only reason to have a suffix is when all libraries are stored in a single directory.

wrong.. not the only reason. The reason to have d suffix is so that cmake can distinguish _LIBRARY_DEBUG from _LIBRARY_RELEASE and actually find the correct DEBUG and RELEASE version to setup the targets. Otherwise a vcpkg-cmake-wrapper.cmake is necessary to fix the targets/variables

If you know some issues that can happen - please provide an example.

feel free to read #6045 and all associated issues/pr. This should keep you busy for at least a week :P

Neumann-A avatar Feb 09 '22 22:02 Neumann-A

cmake can distinguish _LIBRARY_DEBUG from _LIBRARY_RELEASE

They are stored in totally different directories: <triplet>/lib/library_name.dll and <triplet>/debug/lib/library_name.dll. All information previously encoded in suffixes are now in the file path.

https://github.com/microsoft/vcpkg/issues/6045 and all associated issues/pr.

The reason of postfix issues is postfix presence at all, it is not needed. Well, if I see the port which does not have debug suffixes - is it a bug?

vitalyster avatar Feb 09 '22 22:02 vitalyster

They are stored in totally different directories:

You are missing the point. CMake does not have a default concept of "different directories" for libraries. So what will find_library(somelib_RELEASE NAMES somelib) and find_library(somelib_DEBUG NAMES somelib) find? If you don't get it after that please look into your cmake module directory and look at some Find<Modules>.cmake. It doesn't matter if the information is encoded in the directory for cmake since cmake knows nothing about that and there is no way to tell it to cmake in a general configuration dependent way.

Well, if I see the port which does not have debug suffixes - is it a bug?

no but it either has a vcpkg-cmake-wrapper.cmake if it has a cmake Find<Module>.cmake or it just has pkgconfig files which are ok but don't work correctly either in cmake multi config.

Neumann-A avatar Feb 09 '22 22:02 Neumann-A

find_library(somelib_RELEASE NAMES somelib) and find_library(somelib_DEBUG NAMES somelib) find?

Why I should do these strange things? find_library(Boost COMPONENTS ...) with my patch just works. And Visual Studio builds successfully both Release and Debug targets.

it either has a vcpkg-cmake-wrapper.cmake

Great! So vcpkg-cmake-wrapper do the right thing and we don't need to have suffixes.

pkgconfig files which are ok but don't work correctly either in cmake multi config

Yes, and that is the job of vcpkg to provide a wrapper for multi config cmake and take the information from library directory or suffix. You are missing the point that these suffixes makes vcpkg builds unusable from other build systems. While I can provide a triplet directory root (either release or debug one) with clean names for any build system and it will find libraries without any specific changes. Yes it will not work for "multi config" builds, but it is not needed because there is no such concept in other build systems.

vitalyster avatar Feb 09 '22 22:02 vitalyster

Let's get back to the core problem seeking a solution.

This PR resolves problem with other build systems: without system layout each build system should implement giant FindBoost.cmake analog for no reason.

Which buildsystem are you specifically having an issue with? I'm not sure if it has changed in recent versions, but the last time I checked the prebuilt binaries provided by Boost for Windows were built using the default layout and not the system layout. I assume that all C++ buildsystems must have a solution for using those well known binaries?

ras0219-msft avatar Feb 10 '22 02:02 ras0219-msft

@ras0219-msft prebuilt Boost binaries are using auto linking by default and that is assumed by a lot of custom build tools. These suffixes were made for Boost auto linking feature. Quoting your answer in https://github.com/microsoft/vcpkg/issues/694#issuecomment-281235155

we want to embrace the common, cross-platform core that CMake defines. Therefore, when using CMake, we model the same experience (i.e. no-auto-linking) as on other platforms so you can minimize per-platform differences

Without suffixes I can use vcpkg Boost binaries in a custom build environment without platform-specific code, and there is no any reason to keep suffixes when auto linking is disabled.

vitalyster avatar Feb 10 '22 02:02 vitalyster

@ras0219-msft ,Could you please help review this pr?

JonLiu1993 avatar Feb 16 '22 09:02 JonLiu1993

Sorry for taking a long time to get back to this issue, but we've thought more about the problem and come to the following conclusions:

  1. Relying on the publicly documented boost feature of autolinking (which is supported by the official boost distribution) makes sense and is something that vcpkg should support. If that will resolve your case, then I move that we retitle this issue to be "Re-enable boost autolinking with MSVC".
  2. While it would be convenient for boost to officially have the same binary names on all platforms, the official prebuilt windows binaries do not use the system layout, so it doesn't seem that us providing the system layout will overlap with existing users. Therefore, we do not see moving to the system layout as worth the costs to CMake's Multi-config generators. (MSBuild users are fine either way via our MSBuild integration).

ras0219-msft avatar Apr 14 '22 23:04 ras0219-msft

  1. Yes, I'm ok with autolinking support, but that may require a lot more work
  2. Still not clear why we need such concept as "multi-config build", because everything just works with system layout

vitalyster avatar Apr 19 '22 08:04 vitalyster

/azp run

JonLiu1993 avatar Jun 13 '22 06:06 JonLiu1993

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Jun 13 '22 06:06 azure-pipelines[bot]

@vitalyster, please run command ./vcpkg x-add-version boost-modular-build-helper --overwrite-versionand commit again

JonLiu1993 avatar Aug 05 '22 07:08 JonLiu1993

@vitalyster, Can you please resolve the conflicts against master?

JonLiu1993 avatar Sep 09 '22 07:09 JonLiu1993

@vitalyster, Can you please resolve the conflicts against master?

JonLiu1993 avatar Oct 28 '22 08:10 JonLiu1993

FTR, there is a draft PR to get a little bit of multi-config usage covered, #27408. It doesn't include boost. In any case, multi-config support shouldn't degrade with new PRs. After all, that's the whole point why vcpkg builds release and debug by default.

dg0yt avatar Mar 03 '23 07:03 dg0yt

I will be converting your PR to draft status. The above suggested changes are only recommendations. If you are willing to adopt them, please click "ready for review" after making the modifications. If you do not wish to make any changes, please click "ready for review" directly. That way, I can be aware that you've responded since you can't modify the tags.

JonLiu1993 avatar May 06 '23 10:05 JonLiu1993

@vitalyster, Could you pleasae fix the conflicting files?

JonLiu1993 avatar May 26 '23 08:05 JonLiu1993

@vitalyster, please change the port-version of both boost-uninstall/vcpkg.json and boost-modular-build-helper/vcpkg.json files, then run command ./vcpkg x-add-version --all --overwrite-version and commit again.

JonLiu1993 avatar May 30 '23 01:05 JonLiu1993