Dear @BillyONeal,
The requested change #45295 (comment) has been implemented in this PR.

Can we please get some traction on this PR ?

It seems the hold-up is removal of post-installation header patch for FMT_SHARED
and the choices are:

1. Cause build inconvenience to MSBuild build-system users which requires them to explicitly add -FMT_SHARED either in the including-header or via build system declaration (Should cause a build failure AFAIK)

2. Cause debugging inconvenience to all windows users (CMake and MSBuild alike) where the source files won't match leading to missing breakpoints unless the Visual Studio - "Require source files to exactly match" option is turned off (No indication of why it's happening)

Is there a reason we want 1. to take precedence.
Personally, I'm more sympathetic to the plight of (2) but I'm biased (having abandoned MSBuild as a native build-system long back in favor of CMake-generators)

Can we please take a final call as to why we'd like (1) to take precedence here and move on with this change ?

I'm sure a lot of folks are eagerly awaiting the fmt updates.

It is easy to patch the header on the condition of VCPKG_LIBRARY_LINKAGE if debugging fmt is the major concern. No reason to break msbuild.

Not sure how conditioning the patch on VCPKG_LIBRARY_LINKAGE would help, unless you're suggesting that it's a problem only for shared-libs ?. I could be wrong, but I think even static libs would be affected by the mismatch since the translation unit has already compiled and won't match the header anymore. It'd be nice if someone can confirm if the debugging issue is only for shared or static or both.

(Sorry for injecting my opinions below but just trying to get a discussion going to reach a conclusion sooner)

No reason to break msbuild.

The change can probably be considered a regression, but I think it could be argued that users of MSBuild shouldn't rely on header patches coming from dependency/package managers like vcpkg anyway.
Specifically in this instance where the alternative fix is trivial.
It'll definitely cause a surprise for someone updating vcpkg but in my experience even for cmake vcpkg releases usually come with plenty surprises, so this'll just be one for the MSBuild folks.

@BillyONeal mentioned on the other (obsolete) PR the following

Good support for downstream customers who don't speak CMake configs is a thing we like and ask for but is not generally a requirement. We wouldn't ask you to add it if it were not there, but given that it is already here, we don't want to remove it 'just because'. If this being here broke something about the fmt update then it would be fine to remove.

Which sounds like a fair policy

So it seems the question really is if pdb-load issues and fmt-debugger-step-in issues meets the bar of "something broken"
FWIW I dont usually debug fmt but once in a while I do find myself trying to step-in to the header

@yurybara

Can you share an example of usage in MSBuild and how this patch will help MSBuild users?

I tried to prepare a hello world example for you but it turns out I can't actually do that for Windows. The effect of FMT_SHARED on Windows is to add __declspec(dllimport) to declarations inside fmt. And without that setting, things still happen to work even with the hello world defaults that link with fmt.dll, they're just slower (unless you link with /GL which can figure this out and put dllimport back on under the covers...)

Where it will actually break is non-Windows, non-CMake, dynamic customers, which is admittedly a small set as all the non-Windows triplets default to building static.

Embedding settings customers can't change after the fact into the installed contents is common in vcpkg's registry, and we view it as an improvement over any scheme that relies on build system bindings like CMake configs, as embedded settings don't require downstream customers choose a matching build system to work. Here are some examples:

• abseil

  • vcpkg/ports/abseil/portfile.cmake

    Lines 17 to 20 in 316e9e2

    	set(ABSL_USE_CXX17_OPTION "")
    	if("cxx17" IN_LIST FEATURES)
    	set(ABSL_USE_CXX17_OPTION "-DCMAKE_CXX_STANDARD=17")
    	endif()

  • vcpkg/ports/abseil/portfile.cmake

    Lines 27 to 30 in 316e9e2

    	set(ABSL_STATIC_RUNTIME_OPTION "")
    	if(VCPKG_TARGET_IS_WINDOWS AND VCPKG_CRT_LINKAGE STREQUAL "static")
    	set(ABSL_STATIC_RUNTIME_OPTION "-DABSL_MSVC_STATIC_RUNTIME=ON")
    	endif()

  • vcpkg/ports/abseil/portfile.cmake

    Lines 61 to 64 in 316e9e2

    	if(VCPKG_TARGET_IS_WINDOWS AND VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic")
    	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/absl/base/config.h" "defined(ABSL_CONSUME_DLL)" "1")
    	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/absl/base/internal/thread_identity.h" "defined(ABSL_CONSUME_DLL)" "1")
    	endif()

• apr-util

  vcpkg/ports/apr-util/portfile.cmake

  Lines 54 to 58 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL dynamic)
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/apu.h" "defined(APU_DECLARE_STATIC)" "0")
  	else()
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/apu.h" "defined(APU_DECLARE_STATIC)" "1")
  	endif()

• argon2

  vcpkg/ports/argon2/portfile.cmake

  Lines 41 to 43 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/argon2.h" "defined(USING_ARGON2_DLL)" "1")
  	endif()

• asmjit

  vcpkg/ports/asmjit/portfile.cmake

  Lines 23 to 28 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/asmjit/core/api-config.h"
  	"#if !defined(ASMJIT_STATIC)"
  	"#if 0"
  	)
  	endif()

• asmtk

  vcpkg/ports/asmtk/portfile.cmake

  Lines 28 to 30 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/asmtk/globals.h" "!defined(ASMTK_STATIC)" "0")
  	endif()

• atk

  vcpkg/ports/atk/portfile.cmake

  Lines 35 to 37 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/atk-1.0/atk/atkmisc.h" "ifdef ATK_STATIC_COMPILATION" "if 1")
  	endif()

• blend2d

  vcpkg/ports/blend2d/portfile.cmake

  Lines 32 to 41 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/blend2d/api.h"
  	"#if !defined(BL_STATIC)"
  	"#if 0"
  	)
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/blend2d-debug.h"
  	"#if defined(BL_STATIC)"
  	"#if 1"
  	)
  	endif()

• box2d

  vcpkg/ports/box2d/portfile.cmake

  Lines 28 to 30 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/box2d/base.h" "defined( BOX2D_DLL )" "1")
  	endif()

• c-ares

  vcpkg/ports/c-ares/portfile.cmake

  Lines 29 to 34 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  	vcpkg_replace_string(
  	"${CURRENT_PACKAGES_DIR}/include/ares.h"
  	"# ifdef CARES_STATICLIB" "#if 1"
  	)
  	endif()

• cairo

  vcpkg/ports/cairo/portfile.cmake

  Lines 65 to 67 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/cairo/cairo.h" "defined(CAIRO_WIN32_STATIC_BUILD)" "1")
  	endif()

• cariomm

  vcpkg/ports/cairomm/portfile.cmake

  Lines 29 to 31 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/cairommconfig.h" "# define CAIROMM_DLL 1" "# undef CAIROMM_DLL\n# define CAIROMM_STATIC_LIB 1")
  	endif()

• celero

  vcpkg/ports/celero/portfile.cmake

  Lines 29 to 31 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL static)
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/celero/Export.h" "#ifdef CELERO_STATIC" "#define CELERO_STATIC\n#ifdef CELERO_STATIC")
  	endif()

• cgns

  vcpkg/ports/cgns/portfile.cmake

  Lines 65 to 67 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/cgnslib.h" "defined(USE_DLL)" "1")
  	endif()

• check

  vcpkg/ports/check/portfile.cmake

  Lines 28 to 30 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic" AND VCPKG_TARGET_IS_WINDOWS)
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/check.h" "#define CK_DLL_EXP" "#define CK_DLL_EXP __declspec(dllimport)")
  	endif()

• chromaprint

  vcpkg/ports/chromaprint/portfile.cmake

  Lines 21 to 23 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/chromaprint.h" "ifdef CHROMAPRINT_NODLL" "if 1")
  	endif()

• civetweb

  vcpkg/ports/civetweb/portfile.cmake

  Lines 44 to 47 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/civetweb.h" "defined(CIVETWEB_DLL_IMPORTS)" 1)
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/CivetServer.h" "defined(CIVETWEB_CXX_DLL_IMPORTS)" 1)
  	endif()

• cminpack

  vcpkg/ports/cminpack/portfile.cmake

  Lines 32 to 34 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/cminpack-1/cminpack.h" [[!defined(CMINPACK_NO_DLL)]] 0)
  	endif()

• coolprop

  vcpkg/ports/coolprop/portfile.cmake

  Lines 50 to 54 in 316e9e2

  	if (VCPKG_TARGET_IS_WINDOWS AND COOLPROP_SHARED_LIBRARY)
  	vcpkg_replace_string(${CURRENT_PACKAGES_DIR}/include/CoolPropLib.h
  	"#if defined(COOLPROP_LIB)" "#if 1"
  	)
  	endif()

• cpp-ipc

  vcpkg/ports/cpp-ipc/portfile.cmake

  Lines 29 to 31 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/libipc/export.h" "#elif defined(LIBIPC_LIBRARY_SHARED_USING__)" "#elif 1")
  	endif()

• curl

  vcpkg/ports/curl/portfile.cmake

  Lines 122 to 127 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  	vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/curl/curl.h"
  	"#ifdef CURL_STATICLIB"
  	"#if 1"
  	)
  	endif()

• czmq

  vcpkg/ports/czmq/portfile.cmake

  Lines 76 to 81 in 316e9e2

  	if(VCPKG_LIBRARY_LINKAGE STREQUAL static)
  	vcpkg_replace_string(${CURRENT_PACKAGES_DIR}/include/czmq_library.h
  	"if defined CZMQ_STATIC"
  	"if 1 //if defined CZMQ_STATIC"
  	)
  	endif()

It makes no difference whether the setting is published in these projects' CMake bindings, if any, because non-CMake customers exist.

• The removed code was actually a patch of the source. It was added for {fmt} version 3.0.1 in this commit. At the time, the issue hadn’t yet been resolved in the upstream library.
• The issue was later resolved in the library itself in version 5.0.0 via CMake.
• When the port was updated, the patch remained and hadn’t been removed until now.
• I’ve now removed the outdated patch.

I think I see the confusion now. Before 5.x, fmt didn't correctly publish this setting in its CMake bindings, so embedding the value of the setting helped everyone. fmt 5.x now publishes this setting in its CMake bindings, so if you're a CMake customer, this looks like a patch that is no longer necessary.

When the vcpkg team overall reviewed this before, we did not understand that fmt didn't publish the setting in CMake bindings before 3.x. We viewed this entirely as 'elevating the experience of non-CMake customers to that of CMake customers', and thus saw it as something we should keep.

(I'm using past tense here because the vcpkg maintainers together haven't discussed this issue with the understanding 'it still happens to work')

@ankurvdev

Cause build inconvenience to MSBuild build-system users which requires them to explicitly add -FMT_SHARED either in the including-header or via build system declaration (Should cause a build failure AFAIK)
[...]
I think it could be argued that users of MSBuild shouldn't rely on header patches coming from dependency/package managers like vcpkg anyway.

We explicitly disagree. We don't have a maintainer guide entry requiring all settings to be embedded like this because it's a lot closer to 'nice to have' and we don't have great ways to verify it. We do care about non-CMake customers.

Cause debugging inconvenience to all windows users (CMake and MSBuild alike) where the source files won't match leading to missing breakpoints unless the Visual Studio - "Require source files to exactly match" option is turned off (No indication of why it's happening)

In most cases it won't actually do that, because the change is only in an installed header, and downstream customers are doing their builds with that installed header, so things match. It could happen if you debug into fmt.dll, and are looking at the version of the header as it existed in vcpkg's buildtrees, but we normally don't even preserve buildtrees and it isn't put back at all on binary cache hits or similar.

@BillyONeal Thank you for the detailed analysis.

While reviewing the provided examples, I’d like to point out that in the case of abseil, some build parameters were defined prior to compilation, which I consider correct and appropriate. The library supports such options and correctly generates the required files. In other cases, modifications are performed in VCPKG post-build, which causes the resulting files to differ from those used during compilation.

Many libraries support installing additional configuration files alongside the sources (e.g. <package>.pc, <package>-config.cmake). If the final project opts out of using any existing build system, many settings will need to be defined manually anyway — directory paths (include, lib, bin, etc.), compiler options like /std:c++17, and preprocessor defines such as FMT_STATIC. That seems like a reasonable trade-off for not relying on a build system.

I’m not very familiar with MSBuild, but if VCPKG intends to support MSBuild directly, perhaps it would make sense to generate a <package>.props file to streamline integration.

From an optimization standpoint, it would be more convenient for users to have a unified include folder across various triplets and configurations. Currently, VCPKG allows this for build configurations (debug/release), but not for triplets.

To summarize the above, I believe that the presence of such patches does not correctly and completely solve the problem of connecting third-party libraries to MSBuild projects. Such patches do more harm than good.

/azp run

Looks like a baseline issue on arm64-osx, I'm investigating....

@vicroms and I discussed today. We still would prefer leaving the imbuing in but given that MSBuild customers are no longer broken it's no longer an over our dead body issue and we're happy to make the contributor(s) happier by just merging this as is.

I'm going to merge through the clear vtk baseline issue.