-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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.
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
[docs] Add maintainer guidelines #6871
Changes from all commits
03332d0
3ae4b97
b486586
d3d3bce
50910d3
d8cd438
da71c3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,178 @@ | ||
# 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 [Homebrew's Formula Cookbook](https://docs.brew.sh/Formula-Cookbook). | ||
|
||
## PR Structure | ||
|
||
### Make separate Pull Requests per port | ||
|
||
Whenever possible, separate changes into multiple PR's. 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), 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, 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. Most new PRs should be opened as drafts and converted to normal PRs once the CI passes. | ||
|
||
More information about GitHub Draft PRs: https://github.blog/2019-02-14-introducing-draft-pull-requests/ | ||
|
||
## Portfiles | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you perhaps add notes on the recommended way to do OS checks? In my past few submitted PRs, I have received suggestions to use:
Some formal guidelines on the matter would be great There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that this is required. You simply have to learn to differentiate between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a great explanation and I think it would also be good to include this in the document for future maintainers! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added an ascii art diagram to illustrate the difference. Let me know if it helps! |
||
|
||
### Avoid deprecated helper functions | ||
|
||
At this time, the following helpers are deprecated: | ||
|
||
1. `vcpkg_extract_archive()` should be replaced by `vcpkg_extract_archive_ex()` | ||
2. `vcpkg_apply_patches()` should be replaced by the `PATCHES` arguments to the "extract" helpers (e.g. `vcpkg_from_github()`) | ||
3. `vcpkg_build_msbuild()` should be replaced by `vcpkg_install_msbuild()` | ||
|
||
### Avoid excessive comments in portfiles | ||
|
||
Ideally, portfiles should be short, simple, and as declarative as possible. Remove any helper comments introduced by the `create` command before submitting a PR. | ||
|
||
## Build Techniques | ||
|
||
### Do not use vendored dependencies | ||
|
||
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. 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`, however for libraries that don't respect that variable, you can switch on `VCPKG_LIBRARY_LINKAGE`: | ||
|
||
```cmake | ||
string(COMPARE EQUAL "${VCPKG_LIBRARY_LINKAGE}" "static" KEYSTONE_BUILD_STATIC) | ||
string(COMPARE EQUAL "${VCPKG_LIBRARY_LINKAGE}" "dynamic" KEYSTONE_BUILD_SHARED) | ||
|
||
vcpkg_configure_cmake( | ||
SOURCE_PATH ${SOURCE_PATH} | ||
PREFER_NINJA | ||
OPTIONS | ||
-DKEYSTONE_BUILD_STATIC=${KEYSTONE_BUILD_STATIC} | ||
-DKEYSTONE_BUILD_SHARED=${KEYSTONE_BUILD_SHARED} | ||
) | ||
``` | ||
|
||
### 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. For example: | ||
|
||
```cmake | ||
set(CMAKE_DISABLE_FIND_PACKAGE_ZLIB ON) | ||
if("zlib" IN_LIST FEATURES) | ||
set(CMAKE_DISABLE_FIND_PACKAGE_ZLIB OFF) | ||
endif() | ||
|
||
vcpkg_configure_cmake( | ||
SOURCE_PATH ${SOURCE_PATH} | ||
PREFER_NINJA | ||
OPTIONS | ||
-DCMAKE_DISABLE_FIND_PACKAGE_ZLIB=${CMAKE_DISABLE_FIND_PACKAGE_ZLIB} | ||
) | ||
``` | ||
|
||
Note that `ZLIB` in the above is case-sensitive. See the [cmake documentation](https://cmake.org/cmake/help/v3.15/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html) for more details. | ||
|
||
## Versioning | ||
|
||
### Follow common conventions for the `Version:` field | ||
|
||
See our [CONTROL files document](control-files.md#version) for a full explanation of our conventions. | ||
|
||
### Update the `Version:` field in the `CONTROL` file of any modified ports | ||
|
||
Vcpkg uses this field to determine whether a given port is out-of-date and should be changed whenever the port's behavior changes. | ||
|
||
Our convention for this field is to append a `-N` to the upstream version when changes need to be made. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is interesting, however:
If we are using SemVer as the driving design, this would imply that Based on my experience will all the current libraries, the number of cases where a trailing |
||
|
||
For Example: | ||
|
||
- Zlib's package version is currently `1.2.1`. | ||
- You've discovered that the wrong copyright file has been deployed, and fixed that in the portfile. | ||
- You should update the `Version:` field in the control file to `1.2.1-1`. | ||
|
||
See our [CONTROL files document](control-files.md#version) for a full explanation of our conventions. | ||
|
||
## Patching | ||
|
||
### Prefer options over patching | ||
|
||
It is preferable to set options in a call to `vcpkg_configure_xyz()` over patching the settings directly. | ||
|
||
Common options that allow avoiding patching: | ||
1. [MSBUILD] `<PropertyGroup>` settings inside the project file can be overridden via `/p:` parameters | ||
2. [CMAKE] Calls to `find_package(XYz)` in CMake scripts can be disabled via [`-DCMAKE_DISABLE_FIND_PACKAGE_XYz=ON`](https://cmake.org/cmake/help/v3.15/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html) | ||
3. [CMAKE] Cache variables (declared as `set(VAR "value" CACHE STRING "Documentation")` or `option(VAR "Documentation" "Default Value")`) can be overriden by just passing them in on the command line as `-DVAR:STRING=Foo`. One notable exception is if the `FORCE` parameter is passed to `set()`. See also the [CMake `set` documentation](https://cmake.org/cmake/help/v3.15/command/set.html) | ||
|
||
### 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. | ||
|
||
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. | ||
|
||
### Do not implement features in patches | ||
|
||
The purpose of patching in vcpkg is to enable compatibility with compilers, libraries, and platforms. It is not to implement new features in lieu of following proper Open Source procedure (submitting an Issue/PR/etc). | ||
|
||
## Do not build tests/docs/examples by default | ||
|
||
When submitting a new port, check for any options like `BUILD_TESTS` or `WITH_TESTS` or `POCO_ENABLE_SAMPLES` and ensure the additional binaries are disabled. This minimizes build times and dependencies for the average user. | ||
|
||
Optionally, you can add a `test` feature which enables building the tests, however this should not be in the `Default-Features` list. | ||
|
||
## Enable existing users of the library to switch to vcpkg | ||
|
||
### Do not add `CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS` | ||
|
||
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)`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it Ok to do this for C libraries? |
||
|
||
### 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that in general, we should not be doing this. The reasoning is twofold:
When using single configuration generators with CMake, this should be a non-issue -- However, we do need to handle the case of Multi-configuration generators. I believe the right way to proceed for this case will be to add This appears (to me) to be the ONLY way to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is in general a correct stance with one important exception: If the project uses CMake the maintainers of the project implicitly agree to renaming the library because passing
Does VCPKG have that authority? e.g.: #6698
I think your arguments is based on a wrong assumption here. Do package managers and system even supply debug libraries? Do Makefiles care about external debug libraries? Did the writer of the Makefile even consider external debug libraries? I think the answer to those questions are probably all "No" because I read somewhere that on linux debug/release are not so well defined as on Windows due to the iterator debug levels. Due to that VCPKG should not care about them because we still deliver the release libraries and it should still work for external users. Your main concern here seems to be the ports within VCPKG which use Makefiles instead of CMake. To patch these Makefiles is the work VCPKG has to do internally.
From my experience with PR 5543. TL;DR: My opinion
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First, given that a library "officially" uses the same name in release and debug, we can expect that FindXYZ scripts will be written with a single As I see it, our options are:
Given this, it seems like only adding a vcpkg-cmake-wrapper is the obviously correct thing to do. I'll also note that Multi-config generators, while we do care about making them work, are going to become less and less common since VS now has built-in cmake support (which runs in Single Configuration mode :)). I'll also note that within Vcpkg we always build in single-configuration style (even if using MSBuild!), so as long as we just don't add a suffix, everything will tend to work fine (including existing FindXYZ scripts embedded in other libraries). In this case, we actually tend to hit the opposite problem where libraries might add a suffix in their official binaries but FindXYZ scripts aren't written to handle them correctly.
I have not seen it to be conventional to pass this value in with other package managers (apt, brew, etc). If you have evidence to the contrary, I'd love to see it! Just because CMake has a crazy option which can mess with your library, doesn't mean that you've implicitly approved of it.
At the end of the day, we do have to ship something, and we have the power to patch buildsystems to decide what the binaries are called. My primary point in the guidelines is that we should seek to exercise this power as little as possible while achieving compatibility. As much as possible, we should use the names that the upstream buildsystem(s) use. The one common exception is where we do choose to exercise this is static vs dynamic -- I'll omit talking more about that case for brevity.
Boost specifically has an enumeration of valid library naming schemes -- we use one of them (with asterisks).
You're right that Makefiles on Linux may not commonly handle this, but other cases will. Qt/QMake does, MSBuild does, and NMake Makefiles on Windows will (for exactly the reason you stated).
No, I'm actually primarily concerned with users outside vcpkg, using buildsystems that we can't know or fix. Within vcpkg's curated set, I'm certain that we can patch our way to victory no matter what strategy we choose here -- however, we can't patch external software like firefox (for example). Therefore, we should choose names that are maximally likely to be compatible with existing practice. There's an obvious choice for that: it's whatever the current buildsystems produce by default with no interference/non-default options.
Links? Vcpkg must be able to handle libraries that use the same name for release and debug (in the general case). Therefore, we always use separate folders.
That only applies if you are using
This is the outcome for FindXYZ.cmake scripts that do handle both libraries at once; if that doesn't work, we have bigger problems!
Not a bad idea, I agree this would be clearer. Though I think the primary problem with |
||
|
||
Important caveat: | ||
- Static and shared variants often should be renamed to a common scheme. This enables consumers to use a common name and be ignorant of the downstream linkage. This is safe because we only make one at a time available. | ||
|
||
Note that if a library generates CMake integration files (`foo-config.cmake`), renaming must be done through patching the CMake build itself instead of simply calling `file(RENAME)` on the output archives/LIBs. | ||
|
||
Finally, DLL files on Windows should never be renamed post-build because it breaks the generated LIBs. | ||
|
||
## Useful implementation notes | ||
|
||
### Portfiles are run in Script Mode | ||
|
||
While `portfile.cmake`'s and `CMakeLists.txt`'s share a common syntax and core CMake language constructs, portfiles run in "Script Mode", whereas `CMakeLists.txt` files run in "Build Mode" (unofficial term). The most important difference between these two modes is that "Script Mode" does not have a concept of "Target" -- any behaviors that depend on the "target" machine (`CMAKE_CXX_COMPILER`, `CMAKE_EXECUTABLE_SUFFIX`, `CMAKE_SYSTEM_NAME`, etc) will not be correct. | ||
|
||
Portfiles have direct access to variables set in the triplet file, but `CMakeLists.txt`s do not (though there is often a translation that happens -- `VCPKG_LIBRARY_LINKAGE` versus `BUILD_SHARED_LIBS`). | ||
|
||
Finally, portfiles and CMake builds invoked by portfiles are run in different processes. Conceptually: | ||
|
||
```no-highlight | ||
+----------------------------+ +------------------------------------+ | ||
| CMake.exe | | CMake.exe | | ||
+----------------------------+ +------------------------------------+ | ||
| Triplet file | ====> | Toolchain file | | ||
| (x64-windows.cmake) | | (scripts/buildsystems/vcpkg.cmake) | | ||
+----------------------------+ +------------------------------------+ | ||
| Portfile | ====> | CMakeLists.txt | | ||
| (ports/foo/portfile.cmake) | | (buildtrees/../CMakeLists.txt) | | ||
+----------------------------+ +------------------------------------+ | ||
``` | ||
|
||
To determine the host in a portfile, the standard CMake variables are fine (`CMAKE_HOST_WIN32`). | ||
|
||
To determine the target in a portfile, the vcpkg triplet variables should be used (`VCPKG_CMAKE_SYSTEM_NAME`). | ||
|
||
See also our [triplet documentation](../users/triplets.md) for a full enumeration of possible settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A link to the control-file.md documentation that already talks about this would be useful.