Skip to content

[vcpkg] Add mapping for the None config#35940

Merged
JavierMatosD merged 1 commit intomicrosoft:masterfrom
kiwixz:none_config
Jan 8, 2024
Merged

[vcpkg] Add mapping for the None config#35940
JavierMatosD merged 1 commit intomicrosoft:masterfrom
kiwixz:none_config

Conversation

@kiwixz
Copy link
Contributor

@kiwixz kiwixz commented Dec 31, 2023

vcpkg's toolchain currently breaks find_package of CMake libraries on Debian with MinSizeRel or RelWithDebInfo configs. For example I can't import the version of fmt installed by apt.

This is caused by the explicit config mapping which disables the automatic fallback1 to any other configuration and the fact that Debian explicitly sets CMAKE_BUILD_TYPE to None when building packages2.

Footnotes

  1. https://cmake.org/cmake/help/latest/prop_tgt/IMPORTED_LOCATION.html

  2. https://salsa.debian.org/debian/debhelper/-/blob/fedaeeb9a61d2622c134c028d2b02fdb286a8636/lib/Debian/Debhelper/Buildsystem/cmake.pm#L16

@Neumann-A
Copy link
Contributor

Did you provide an empty overlay for fmt?
Also None is not a default configuration of CMake as such I don't see how it would ever map outside of vcpkg. For me this seems more like you want to externally use the None build type and give it a hint to use the Release config if None cannot be found (so basically the other way around)

@kiwixz
Copy link
Contributor Author

kiwixz commented Dec 31, 2023

I did not provide an empty overlay for fmt, my goal here is to make it possible to import a system library not installed in vcpkg. The issue is that just using vcpkg's toolchain (without installing anything) is enough to break find_package for system libraries in those configurations.

None is not mentioned in the CMake documentation but is the de facto standard (especially used by Linux distributions) to try to conform as close as possible to specified CFLAGS and CXXFLAGS. I think adding it as a fallback here make sense, for the same reason we currently have an empty string fallback. It's better to get the None config than not to find anything!

@Neumann-A
Copy link
Contributor

I did not provide an empty overlay for fmt, my goal here is to make it possible to import a system library not installed in vcpkg.

Not providing an (empty) overlay for system deps like this already goes against the philosophy of vcpkg.

None is not mentioned in the CMake documentation but is the de facto standard (especially used by Linux distributions) to try to conform as close as possible to specified CFLAGS and CXXFLAGS.

Doesn't make sense. You could just leave the build type empty in these cases. None is a user defined custom configuration. As such if you want to use it: a) You use None as the config yourself and specify the mapping to vcpkg Release/Debug configs or b) manually specify the MAPPING as done in this PR but outside of vcpkg (e.g. by specifying -DCMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO "RelWithDebInfo;Release;None;" on the command line).

You are basically requesting something here which already fails outside of vcpkg with only CMake.

close as possible to specified CFLAGS and CXXFLAGS.

The custom configs just add CFLAGS_None as compiler flags which are probably left empty, however there are many ways to get proper control over compile flags in CMake without having to specify a custom configuration (e.g. toolchains). I think the configuration was added to avoid conflicting with other versions of user installed fmt libs.

It's better to get the None config than not to find anything!

I see you don't understand the mapping at all. The mapping is for targets which setup IMPORTED_LOCATION but not IMPORTED_LOCATION_<CONFIG> which is often the case in single config FindModules.cmake or even FindPkgConfig.cmake

@kiwixz
Copy link
Contributor Author

kiwixz commented Dec 31, 2023

Not providing an (empty) overlay for system deps like this already goes against the philosophy of vcpkg.

I want vcpkg to handle what it can and fallback to system libraries for others.

Doesn't make sense. You could just leave the build type empty in these cases.

There is ample documentation explaining why it is done, see here for Debian and here for Arch Linux. As I said, it allows to conform as close as possible to the CFLAGS and CXXFLAGS that the distributions try to enforce because it blocks the defaulting of the build type that quite a few libraries have.

You are basically requesting something here which already fails outside of vcpkg with only CMake.

This is wrong. As I mentioned in my first comment CMake will fallback to any available configuration for the imported target if the mapping variable is not explicitly defined. However vcpkg's toolchain will set this variable if not present and therefore break an otherwise working find_package on a default Debian installation.


I'm not trying to argue whether things should have been done differently, but this tiny patch will help compatibility with the most popular Linux distributions out there and without any drawbacks.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 31, 2023

IIRC None was chosen by Linux distributions because

  • the distributions already know which flags they want to use,
  • CMake, and many packages, manipulate flags for the common build types in undesired ways,
  • many packages choose one of the common build types if it is unset.

And Kitware seems to agree that None is appropriate for that purpose, e.g. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=730636#27

It is similar to the vcpkg situation, but only for a single configuration where vcpkg must support two.

@JonLiu1993 JonLiu1993 changed the title Add mapping for the None config [vcpkg] Add mapping for the None config Jan 2, 2024
@JonLiu1993 JonLiu1993 added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Jan 2, 2024
@JonLiu1993 JonLiu1993 requested a review from BillyONeal January 4, 2024 10:02
@JonLiu1993
Copy link
Contributor

@BillyONeal, could you please help take a review? thanks.

@BillyONeal BillyONeal added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 4, 2024
@BillyONeal
Copy link
Member

Regarding:

For example I can't import the version of fmt installed by apt.

@ras0219-msft "If without vcpkg's edits, it would have been found, then it makes sense to add this so that they are still found."

The reason that we edit CMAKE_MAP_IMPORTED_CONFIG_* is to make sure that we get found, not to prevent others from being found.

@Neumann-A
Copy link
Contributor

@BillyONeal This will just add precedent case for all custom configuration following that argument. It would be better if upstream CMake would have a pattern rule for configuration mapping or an ANY keyword.

@kiwixz
Copy link
Contributor Author

kiwixz commented Jan 5, 2024

@BillyONeal This will just add precedent case for all custom configuration following that argument. It would be better if upstream CMake would have a pattern rule for configuration mapping or an ANY keyword.

This is a good idea. Please feel free to open an issue on CMake upstream, but until this is implemented in the CMake versions most commonly found vcpkg should still strive for full compatibility with all Debian (and Arch) based distributions.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM but I would like @ras0219-msft on record as also OKing it.

@BillyONeal BillyONeal added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Jan 5, 2024
@JavierMatosD JavierMatosD merged commit d64c5e8 into microsoft:master Jan 8, 2024
Osyotr pushed a commit to Osyotr/vcpkg that referenced this pull request Jan 23, 2024
TomKatom pushed a commit to TomKatom/vcpkg that referenced this pull request Feb 23, 2024
walbourn added a commit to walbourn/vcpkg that referenced this pull request Mar 19, 2024
walbourn added a commit to walbourn/vcpkg that referenced this pull request Mar 19, 2024
@kiwixz kiwixz deleted the none_config branch January 22, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants