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.

Sign up for GitHub

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

Add maintainer guide entry on CMake variants and expand on "only-one-variant" #176

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add maintainer guide entry on CMake variants and expand on "only-one-variant" #176

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 139 additions & 6 deletions vcpkg/contributing/maintainer-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,12 +216,15 @@ Additionally, when appropriate, it can be easier and more maintainable to rewrit

Examples: [abseil](https://github.com/Microsoft/vcpkg/tree/master/ports/abseil/portfile.cmake)

### Choose either static or shared binaries
### <a name="only-static-or-shared"></a>Choose either static or shared binaries

By default, `vcpkg_cmake_configure()` will pass in the appropriate setting for `BUILD_SHARED_LIBS`,
however for libraries that don't respect that variable, you can switch on `VCPKG_LIBRARY_LINKAGE`:
When building CMake libraries, [`vcpkg_cmake_configure()`](../maintainers/functions/vcpkg_cmake_configure.md) will pass in the correct value for `BUILD_SHARED_LIBS` based on the user's requested variant.

You can calculate alternative configure parameters by using `string(COMPARE EQUAL "${VCPKG_LIBRARY_LINKAGE}" ...)`.

```cmake
# portfile.cmake

string(COMPARE EQUAL "${VCPKG_LIBRARY_LINKAGE}" "static" KEYSTONE_BUILD_STATIC)
string(COMPARE EQUAL "${VCPKG_LIBRARY_LINKAGE}" "dynamic" KEYSTONE_BUILD_SHARED)

Expand All @@ -233,6 +236,70 @@ vcpkg_cmake_configure(
)
```

If a library does not offer configure options to select the build variant, the build must be patched. When patching a build, you should always attempt to maximize the future maintainability of the port. Typically this means minimizing the number of lines that need to be touched to fix the issue at hand.

After all unwanted variants have been disabled, the produced CMake config or pkg-config files should be modified to [provide all variant names](#provide-cmake-variants) to maximize compatibility with downstream builds which may have hardcoded a variant name.

#### Example: Patching a CMake library to avoid building unwanted variants

For example, when patching a CMake-based library, it may be sufficient to add [`EXCLUDE_FROM_ALL`](https://cmake.org/cmake/help/latest/prop_tgt/EXCLUDE_FROM_ALL.html) to unwanted targets and wrap the `install(TARGETS ...)` call in an `if(BUILD_SHARED_LIBS)`. This will be shorter than wrapping or deleting every line that mentions the unwanted variant.

For a project `CMakeLists.txt` with the following contents:
```cmake
add_library(contoso SHARED contoso.c)
add_library(contoso_static STATIC contoso.c)

install(TARGETS contoso contoso_static EXPORT ContosoTargets)

install(EXPORT ContosoTargets
FILE ContosoTargets
NAMESPACE contoso::
DESTINATION share/contoso)
```

Only the `install(TARGETS)` line needs to be patched.
```cmake
add_library(contoso SHARED contoso.c)
add_library(contoso_static STATIC contoso.c)

if(BUILD_SHARED_LIBS)
set_target_properties(contoso_static PROPERTIES EXCLUDE_FROM_ALL 1)
install(TARGETS contoso EXPORT ContosoTargets)
else()
set_target_properties(contoso PROPERTIES EXCLUDE_FROM_ALL 1)
install(TARGETS contoso_static EXPORT ContosoTargets)
endif()

install(EXPORT ContosoTargets
FILE ContosoTargets
NAMESPACE contoso::
DESTINATION share/contoso)
```

If a buildsystem patch is required for [Choose either static or shared binaries](#only-static-or-shared), you can also resolve [Provide all CMake variant targets](#provide-cmake-variants) by introducing stubs that have the original variant's [`EXPORT_NAME`](https://cmake.org/cmake/help/latest/prop_tgt/EXPORT_NAME.html). These stubs can be introduced as part of the same patch hunk.

```cmake
add_library(contoso SHARED contoso.c)
add_library(contoso_static STATIC contoso.c)

if(BUILD_SHARED_LIBS)
set_target_properties(contoso_static PROPERTIES EXCLUDE_FROM_ALL 1)
add_library(contoso_static_stub INTERFACE)
set_target_properties(contoso_static_stub PROPERTIES EXPORT_NAME contoso_static INTERFACE_LINK_LIBRARIES contoso)
install(TARGETS contoso contoso_static_stub EXPORT ContosoTargets)
else()
set_target_properties(contoso PROPERTIES EXCLUDE_FROM_ALL 1)
add_library(contoso_stub INTERFACE)
set_target_properties(contoso_stub PROPERTIES EXPORT_NAME contoso INTERFACE_LINK_LIBRARIES contoso_static)
install(TARGETS contoso_stub contoso_static EXPORT ContosoTargets)
endif()

Comment on lines +279 to +296
Copy link
Contributor

Choose a reason for hiding this comment

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

I am against providing stubs. It up to upstream if they want to provide a library type agnostic target or not. So fix it upstream and not in vcpkg.
There is also an argument where this can lead to logical errors downstream. Where a consumer might check

if(TARGET EXISTS dep_static)
*** do something affecting my project which is wrong for dynamic dep ***
endif()

or the other way around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is also an argument where this can lead to logical errors downstream.

I would be very interested in any concrete project you can point to where this causes problematic behavior. I don't think it's realistic for something like this to key off of the existence of the target, since the target's existence is wildly uncontrolled on different systems. For example, based on whether the "upstream" project was built with both or even find_package() order.

I just don't think it's realistic that this will cause problems -- which makes me very curious if you have concrete examples!

I am against providing stubs. It up to upstream if they want to provide a library type agnostic target or not. So fix it upstream and not in vcpkg.

In a vacuum I agree, but the alternative here simply causes too much of a maintenance burden. Too many downstream projects assume one thing or the other and will break if only the static variant is available, or perhaps if only the shared variant is available for the dependency and we're asking that library to build statically.

Furthermore, this is a huge drop in adoption friction for existing applications that we don't control and can't fix. It's the difference between vcpkg being a drop-in replacement versus "The build breaks and I need to do an unknown amount of work to investigate".

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm mildly in support of @Neumann-A's position. The proposal decorates unofficial targets with official names, and it promotes lock-in. It doesn't only break (rare) target tests, but also looking up target properties (and such uses do exist).

Most adoption friction won't go away because the packages simply look for the dependency in a different way. Projects which only look for one variant indicate that the other variant isn't supported/tested. And vcpkg doesn't run the packages's test suites either.

And the usage heuristics will make uninformed proposals in both cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just don't think it's realistic that this will cause problems -- which makes me very curious if you have concrete examples!

I think netcdf-c once added H5_BUILD_DYNAMICALLY (or similar) if it detected the hdf5 shared target leading to linker errors if it was a static build. Has been a long time since then.

Furthermore, this is a huge drop in adoption friction for existing applications that we don't control and can't fix

Don't fight battle you don't have to fight. Simply, this is not a problem for vcpkg to solve. vcpkg is a collection of build recipes somehow glued together and made to work. If upstream doesn't provide easy usage let them not provide easy usage and let people complain about it. If people complain about vcpkg not providing easy usage for X just delegate to upstream and close the issue. Otherwise upstream is never going to learn how to provide proper usage. There needs to be a learning experience for upstream since otherwise vcpkg will have to deal with build scripts which will silently get worse over time since vcpkg magically "fixes all the problems". It doesn't hurt adoption of vcpkg if it is clearly communicated and argued that X is to blame and vcpkg doesn't have any stakes in fixing X usage problems. If it hurts someones adoption it should hurts X adoption rate and vcpkg should amplify that instead of trying to fix it so that upstream has a stronger incentive to fix it.
I mean vcpkg once tried to supply vendored CMakeLists.txt for everything and how well did that work in the end? The less vcpkg has to maintain the better.

A patch to fix static/shared targets in a downstream user is in general more transparent for an experienced user than hacking the targets upstream. It also fits better in the patching policy of vcpkg. Since hacking shared/static targets will never be merged upstream.

but also looking up target properties

That is another nail in the casket of this idea. shared targets on windows are expected to have IMPORTED_LOCATION and IMPORTED_IMPLIB_LOCATION set and at least VTK is checking one of those.

It's the difference between vcpkg being a drop-in replacement versus "The build breaks and I need to do an unknown amount of work to investigate".

This implicitly assumes that the user didn't do its due diligent and hacked together their build scripts with implicit wrong assumptions. This is really not the case vcpkg should consider..... vcpkg generally works if users use cmake correctly and take care of the imported targets. Otherwise, there needs to be some kind of learning experience which needs to be paid in time.

Also about how many ports are we actually speaking about here? HDF5, zstd, another port I forgot?

install(EXPORT ContosoTargets
FILE ContosoTargets
NAMESPACE contoso::
DESTINATION share/contoso)
```

### When defining features, explicitly control dependencies

When defining a feature that captures an optional dependency, ensure that the dependency will not be used accidentally when the feature is not explicitly enabled.
Expand Down Expand Up @@ -392,11 +459,11 @@ Using `vcpkg`'s built-in toolchains this works, because the value of `VCPKG_<LAN

Because of this, it is preferable to patch the buildsystem directly when setting `CMAKE_<LANG>_FLAGS`.

### Minimize patches
### <a name="minimize-patches"></a>Minimize patches

When making changes to a library, strive to minimize the final diff. This means you should _not_ reformat the upstream source code when making changes that affect a region. Also, when disabling a conditional, it is better to add a `AND FALSE` or `&& 0` to the condition than to delete every line of the conditional.
When making changes to a library, strive to minimize the final diff. This means you should not reformat the upstream source code when making changes that affect a region. When disabling a conditional, it is better to add an `AND FALSE` or `&& 0` to the condition than to delete every line of the conditional. If a large region needs to be disabled, it is shorter to add an `if(0)` or `#if 0` around the region instead of deleting every line in the patch.

Don't add patches if the port is outdated and updating the port to a newer released version would solve the same issue. vcpkg prefers updating ports over patching outdated versions unless the version bump breaks a considerable amount of dependent ports.
Don't add patches if the port is outdated and updating the port to a newer released version would solve the same issue. vcpkg prefers updating ports over patching outdated versions.

This helps to keep the size of the vcpkg repository down as well as improves the likelihood that the patch will apply to future code versions.

Expand All @@ -416,6 +483,72 @@ Optionally, you can add a `test` feature which enables building the tests, howev

Unless the author of the library is already using it, we should not use this CMake functionality because it interacts poorly with C++ templates and breaks certain compiler features. Libraries that don't provide a .def file and do not use __declspec() declarations simply do not support shared builds for Windows and should be marked as such with `vcpkg_check_linkage(ONLY_STATIC_LIBRARY)`.

### <a name="provide-cmake-variants"></a>Provide all CMake variant targets

Some CMake-based projects offer the ability to build both static and shared variants of a library in the same build. For example:
```cmake
# contoso/CMakeLists.txt
add_library(contoso SHARED contoso.c)
add_library(contoso_static STATIC contoso.c)
```
This violates [Choose either static or shared binaries](#only-static-or-shared) and the portfile should arrange for only one of the variants to be installed.

After the build has been configured to only provide one set of binaries, many downstream customers are still likely to be broken. For example, if a consumer expects `contoso` to always be available, they may fail to build when static libraries are selected.
```cmake
# myapp/CMakeLists.txt
add_executable(consumer main.c)

find_package(Contoso CONFIG REQUIRED)
target_link_libraries(consumer PRIVATE contoso)
```
If the target is namespaced as `contoso::contoso`, the consuming project will fail at configure time.
```console
CMake Error at CMakeLists.txt:6 (target_link_libraries):
Target "consumer" links to:

contoso::contoso

but the target was not found. Possible reasons include:

* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.
```

Otherwise, it will fail at build time.
```console
LINK : fatal error LNK1104: cannot open file 'contoso.lib'
```

To resolve this issue, the installed CMake config files should be modified to provide all upstream variant names. This improves the ecosystem maintainability because it solves the problem in one place for all consumers, instead of needing to patch every consumer of the library. This also improves compatibility with existing code outside the ecosystem, such as applications that may have hardcoded dependencies on one or the other target names.

To provide all upstream variant names, the CMake config file should be extended with additional `add_library(variant IMPORTED)` calls.
```cmake
# Inside ContosoConfig.cmake
if(TARGET contoso AND NOT TARGET contoso_static)
add_library(contoso_static INTERFACE IMPORTED)
set_target_properties(contoso_static PROPERTIES INTERFACE_LINK_LIBRARIES contoso)
elseif(TARGET contoso_static AND NOT TARGET contoso)
add_library(contoso INTERFACE IMPORTED)
set_target_properties(contoso PROPERTIES INTERFACE_LINK_LIBRARIES contoso_static)
endif()
```
Usually, these lines can be added to the end of the installed CMake config file after [`vcpkg_cmake_config_fixup()`](../maintainers/functions/vcpkg_cmake_config_fixup.md).
```cmake
# In portfile.cmake
vcpkg_cmake_config_fixup(PACKAGE_NAME Contoso)

file(APPEND "${CURRENT_PACKAGES_DIR}/share/ContosoConfig.cmake" "
if(TARGET contoso AND NOT TARGET contoso_static)
add_library(contoso_static INTERFACE IMPORTED)
set_target_properties(contoso_static PROPERTIES INTERFACE_LINK_LIBRARIES contoso)
elseif(TARGET contoso_static AND NOT TARGET contoso)
add_library(contoso INTERFACE IMPORTED)
set_target_properties(contoso PROPERTIES INTERFACE_LINK_LIBRARIES contoso_static)
endif()
")
```

### Do not rename binaries outside the names given by upstream

This means that if the upstream library has different names in release and debug (libx versus libxd), then the debug library should not be renamed to `libx`. Vice versa, if the upstream library has the same name in release and debug, we should not introduce a new name.
Expand Down