Skip to content

Commit

Permalink
[docs] Document acceptable uses for features. (#15171)
Browse files Browse the repository at this point in the history
  • Loading branch information
BillyONeal authored Dec 17, 2020
1 parent 8bd3481 commit 5d41154
Showing 1 changed file with 68 additions and 19 deletions.
87 changes: 68 additions & 19 deletions docs/maintainers/maintainer-guide.md
Original file line number Diff line number Diff line change
@@ -1,36 +1,36 @@
# Maintainer Guidelines and Policies

This document lists a set of policies that you should apply when adding or updating a port recipe.
It is intended to serve the role of
[Debian's Policy Manual](https://www.debian.org/doc/debian-policy/),
[Homebrew's Maintainer Guidelines](https://docs.brew.sh/Maintainer-Guidelines), and
This document lists a set of policies that you should apply when adding or updating a port recipe.
It is intended to serve the role of
[Debian's Policy Manual](https://www.debian.org/doc/debian-policy/),
[Homebrew's Maintainer Guidelines](https://docs.brew.sh/Maintainer-Guidelines), and
[Homebrew's Formula Cookbook](https://docs.brew.sh/Formula-Cookbook).

## PR Structure

### Make separate Pull Requests per port

Whenever possible, separate changes into multiple PRs.
Whenever possible, separate changes into multiple PRs.
This makes them significantly easier to review and prevents issues with one set of changes from holding up every other change.

### Avoid trivial changes in untouched files

For example, avoid reformatting or renaming variables in portfiles that otherwise have no reason to be modified for the issue at hand.
However, if you need to modify the file for the primary purpose of the PR (updating the library),
For example, avoid reformatting or renaming variables in portfiles that otherwise have no reason to be modified for the issue at hand.
However, if you need to modify the file for the primary purpose of the PR (updating the library),
then obviously beneficial changes like fixing typos are appreciated!

### Check names against other repositories

A good service to check many at once is [Repology](https://repology.org/).
If the library you are adding could be confused with another one,
A good service to check many at once is [Repology](https://repology.org/).
If the library you are adding could be confused with another one,
consider renaming to make it clear.

### Use GitHub Draft PRs

GitHub Draft PRs are a great way to get CI or human feedback on work that isn't yet ready to merge.
GitHub Draft PRs are a great way to get CI or human feedback on work that isn't yet ready to merge.
Most new PRs should be opened as drafts and converted to normal PRs once the CI passes.

More information about GitHub Draft PRs:
More information about GitHub Draft PRs:
https://github.blog/2019-02-14-introducing-draft-pull-requests/

## Portfiles
Expand All @@ -46,26 +46,75 @@ At this time, the following helpers are deprecated:

### Avoid excessive comments in portfiles

Ideally, portfiles should be short, simple, and as declarative as possible.
Ideally, portfiles should be short, simple, and as declarative as possible.
Remove any boiler plate comments introduced by the `create` command before submitting a PR.

## Features

### Do not use features to implement alternatives

Features must be treated as additive functionality. If port[featureA] installs and port[featureB] installs, then port[featureA,featureB] must install. Moreover, if a second port depends on [featureA] and a third port depends on [featureB], installing both the second and third ports should have their dependencies satisfied.

Libraries in this situation must choose one of the available options as expressed in vcpkg, and users who want a different setting must use overlay ports at this time.

Existing examples we would not accept today retained for backwards compatibility:
* `libgit2`, `libzip`, `open62541` all have features for selecting a TLS or crypto backend. Note that `curl` has different crypto backend options but allows selecting between them at runtime, meaning the above tenet is maintained.
* `darknet` has `opencv2`, `opencv3`, features to control which version of opencv to use for its dependencies.

### A feature may engage preview or beta functionality

Notwithstanding the above, if there is a preview branch or similar where the preview functionality has a high probability of not disrupting the non-preview functionality (for example, no API removals), a feature is acceptable to model this setting.

Examples:
* The Azure SDKs (of the form `azure-Xxx`) have a `public-preview` feature.
* `imgui` has an `experimental-docking` feature which engages their preview docking branch which uses a merge commit attached to each of their public numbered releases.

### Do not use features to control alternatives in published interfaces

If a consumer of a port depends on only the core functionality of that port, with high probability they must not be broken by turning on the feature. This is even more important when the alternative is not directly controlled by the consumer, but by compiler settings like `/std:c++17` / `-std=c++17`.

Existing examples we would not accept today retained for backwards compatibility:
* `redis-plus-plus[cxx17]` controls a polyfill but does not bake the setting into the installed tree.
* `ace[wchar]` changes all APIs to accept `const wchar_t*` rather than `const char*`.

### A feature may replace polyfills with aliases provided that replacement is baked into the installed tree

Notwithstanding the above, ports may remove polyfills with a feature, as long as:
1. Turning on the feature changes the polyfills to aliases of the polyfilled entity
2. The state of the polyfill is baked into the installed headers, such that ABI mismatch "impossible" runtime errors are unlikely
3. It is possible for a consumer of the port to write code which works in both modes, for example by using a typedef which is either polyfilled or not

Example:
* `abseil[cxx17]` changes `absl::string_view` to a replacement or `std::string_view`; the patch
https://github.com/microsoft/vcpkg/blob/981e65ce0ac1f6c86e5a5ded7824db8780173c76/ports/abseil/fix-cxx-standard.patch implements the baking requirement

### Recommended solutions

If it's critical to expose the underlying alternatives, we recommend providing messages at build time to instruct the user on how to copy the port into a private overlay:
```cmake
set(USING_DOG 0)
message(STATUS "This version of LibContosoFrobnicate uses the Kittens backend. To use the Dog backend instead, create an overlay port of this with USING_DOG set to 1 and the `kittens` dependency replaced with `dog`.")
message(STATUS "This recipe is at ${CMAKE_CURRENT_LIST_DIR}")
message(STATUS "See the overlay ports documentation at https://github.com/microsoft/vcpkg/blob/master/docs/specifications/ports-overlay.md")
```

## Build Techniques

### Do not use vendored dependencies

Do not use embedded copies of libraries.
Do not use embedded copies of libraries.
All dependencies should be split out and packaged separately so they can be updated and maintained.

### Prefer using CMake

When multiple buildsystems are available, prefer using CMake.
When multiple buildsystems are available, prefer using CMake.
Additionally, when appropriate, it can be easier and more maintainable to rewrite alternative buildsystems into CMake using `file(GLOB)` directives.

Examples: [abseil](../../ports/abseil/portfile.cmake)

### Choose either static or shared binaries

By default, `vcpkg_configure_cmake()` will pass in the appropriate setting for `BUILD_SHARED_LIBS`,
By default, `vcpkg_configure_cmake()` 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`:

```cmake
Expand All @@ -83,8 +132,8 @@ vcpkg_configure_cmake(

### 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.
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.

```cmake
if ("zlib" IN_LIST FEATURES)
Expand Down Expand Up @@ -113,7 +162,7 @@ vcpkg_configure_cmake(
SOURCE_PATH ${SOURCE_PATH}
PREFER_NINJA
OPTIONS
${FEATURE_OPTIONS}
${FEATURE_OPTIONS}
)
```

Expand Down Expand Up @@ -168,7 +217,7 @@ Common options that allow avoiding patching:

### Prefer patching over overriding `VCPKG_<VARIABLE>` values

Some variables prefixed with `VCPKG_<VARIABLE>` have an equivalent `CMAKE_<VARIABLE>`.
Some variables prefixed with `VCPKG_<VARIABLE>` have an equivalent `CMAKE_<VARIABLE>`.
However, not all of them are passed to the internal package build [(see implementation: Windows toolchain)](../../scripts/toolchains/windows.cmake).

Consider the following example:
Expand Down

0 comments on commit 5d41154

Please sign in to comment.