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.

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

[scripts-audit] add guidelines for cmake #18319

Closed
wants to merge 12 commits into from
1 change: 1 addition & 0 deletions docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Vcpkg helps you manage C and C++ libraries on Windows, Linux and MacOS. This too
- [Common CMake definitions](maintainers/vcpkg_common_definitions.md)
- [Maintainer Guidelines](maintainers/maintainer-guide.md)
- [Creating Registries](maintainers/registries.md)
- [CMake Guidelines](maintainers/cmake-guidelines.md)

### [Vcpkg-Tool](https://github.com/microsoft/vcpkg-tool) Maintainer Help

Expand Down
127 changes: 127 additions & 0 deletions docs/maintainers/cmake-guidelines.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
# CMake Guidelines

We expect that all CMake scripts that are either:

- In the `scripts/` directory, or
- In a `vcpkg-*` port

should follow the guidelines laid out in this document.
Existing scripts may not follow these guidelines yet;
it is expected that we will continue to update old scripts
to fall in line with these guidelines.

These guidelines are intended to create stability in our scripts.
We hope that they will make both forwards and backwards compatibility easier.

## The Guidelines

- Except for out-parameters, we always use `cmake_parse_arguments()`
rather than function parameters or referring to `${ARG<N>}`.
- This doesn't necessarily need to be followed for "script-local helper functions"
- In this case, positional parameters should be put in the function
declaration (rather than using `${ARG<N>}`),
and should be named according to local rules (i.e. `snake_case`).
- Exception: positional parameters that are optional should be
given a name via `set(argument_name "${ARG<N>}")`, after checking `ARGC`.
- Out-parameters should be the first parameter to a function. Example:
```cmake
function(format out_var)
cmake_parse_arguments(PARSE_ARGV 1 "arg" ...)
# ... set(buffer "output")
set("${out_var}" "${buffer}" PARENT_SCOPE)
endfunction()
```
- There are no unparsed or unused arguments.
Always check for `ARGN` or `arg_UNPARSED_ARGUMENTS`.
`FATAL_ERROR` when possible, `WARNING` if necessary for backwards compatibility.
- All `cmake_parse_arguments` must use `PARSE_ARGV`.
- All `foreach` loops must use `IN LISTS` and `IN ITEMS`.
- The variables `${ARGV}` and `${ARGN}` are unreferenced,
except in helpful messages to the user.
- (i.e., `message(FATAL_ERROR "blah was passed extra arguments: ${ARGN}")`)
- We always use functions, not macros or top level code.
- Exception: "script-local helper macros". It is sometimes helpful to define a small macro.
This should be done sparingly, and functions should be preferred.
- Exception: `vcpkg.cmake`'s `find_package`.
- Scripts in the scripts tree should not be expected to need observable changes
as part of normal operation.
- Example violation: `vcpkg_acquire_msys()` has hard-coded packages and versions that need updating over time due to the MSYS project dropping old packages.
- Example exception: `vcpkg_from_sourceforge()` has a list of mirrors which needs maintenance but does not have an observable behavior impact on the callers.
- All variable expansions are in quotes `""`,
except those which are intended to be passed as multiple arguments.
- Example:
```cmake
set(working_directory "")
if(DEFINED arg_WORKING_DIRECTORY)
set(working_directory "WORKING_DIRECTORY" "${arg_WORKING_DIRECTORY}")
endif()
# calls do_the_thing() if NOT DEFINED arg_WORKING_DIRECTORY,
# else calls do_the_thing(WORKING_DIRECTORY "${arg_WORKING_DIRECTORY}")
do_the_thing(${working_directory})
```
- There are no "pointer" or "in-out" parameters
(where a user passes a variable name rather than the contents),
except for simple out-parameters.
- Variables are not assumed to be empty.
If the variable is intended to be used locally,
it must be explicitly initialized to empty with `set(foo "")`.
- All variables expected to be inherited from the parent scope across an API boundary (i.e. not a file-local function) should be documented. Note that all variables mentioned in triplets.md are considered documented.
- Out parameters are only set in `PARENT_SCOPE` and are never read.
See also the helper `z_vcpkg_forward_output_variable()` to forward out parameters through a function scope.
- `CACHE` variables are used only for global variables which are shared internally among strongly coupled
functions and for internal state within a single function to avoid duplicating work.
These should be used extremely sparingly and should use the `Z_VCPKG_` prefix to avoid
colliding with any local variables that would be defined by any other code.
- Examples:
- `vcpkg_cmake_configure`'s `Z_VCPKG_CMAKE_GENERATOR`
- `z_vcpkg_get_cmake_vars`'s `Z_VCPKG_GET_CMAKE_VARS_FILE`
- `include()`s are only allowed in `ports.cmake` or `vcpkg-port-config.cmake`.
- `foreach(RANGE)`'s arguments _must always be_ natural numbers,
and `<start>` _must always be_ less than or equal to `<stop>`.
- This must be checked by something like:
```cmake
if(start LESS_EQUAL end)
foreach(RANGE start end)
...
endforeach()
endif()
```
- All port-based scripts must use `include_guard(GLOBAL)`
to avoid being included multiple times.
- `set(var)` should not be used. Use `unset(var)` to unset a variable,
and `set(var "")` to set it to the empty value. _Note: this works for use as a list and as a string_

### CMake Versions to Require

- All CMake scripts, except for `vcpkg.cmake`,
strega-nil-ms marked this conversation as resolved.
Show resolved Hide resolved
may assume the version of CMake that is present in the
`cmake_minimum_required` of `ports.cmake`.
strega-nil marked this conversation as resolved.
Show resolved Hide resolved
- This `cmake_minimum_required` should be bumped every time a new version
of CMake is added to `vcpkgTools.xml`, as should the
`cmake_minimum_required` in all of the helper `CMakeLists.txt` files.
- `vcpkg.cmake` must assume a version of CMake back to 3.1 in general
Copy link
Contributor

Choose a reason for hiding this comment

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

For vcpkg-cmake-wrapper.cmake scripts, version 3.1 even excludes if("value" IN_LIST FEATURES). This makes it more complicated to deal with port features or even with arguments to find_package (e.g. port tiff).

Copy link
Contributor

Choose a reason for hiding this comment

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

vcpkg.cmake must assume a version of CMake back to 3.1 in general

Is sentence is nonsense and should be ignored. I think the correct phrasing would be that vcpkg.cmake must assume that the toplevel CMakeLists.txt might not have a cmake_minimum_required or a very low cmake_minimum_required. As such, setting appropriate cmake policies are a requirement. vcpkg assume that it is at least executed with a cmake binary of <some cmake version|preferly the cmake version used by vcpkg>.
If there is somebody really arguing for a cmake binary of 3.1 I would like to have him/her invent vcpkg.old.cmake .....

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we really do support CMake version back to CMake 3.1 in vcpkg.cmake. There are extra features that require more recent versions, but we do support back to CMake 3.1 (or there's a bug.)

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason to keep it at 3.1?
What is the threshold for moving it to another version?
As I already wrote:

For vcpkg-cmake-wrapper.cmake scripts, version 3.1 even excludes if("value" IN_LIST FEATURES).

which is relevant for dealing with transitive dependencies.

Copy link
Contributor

@strega-nil-ms strega-nil-ms Jun 29, 2021

Choose a reason for hiding this comment

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

we do not consider ports as having to support any specific version of CMake; they can support whatever they want. All we need is that, if a port supports CMake 3.1, and the user uses 3.1 for their build, it works.

As to upgrading, not sure.

- Specific functions and options may assume a greater CMake version;
if they do, make sure to comment that function or option
with the required CMake version.


### Changing Existing Functions

- Never remove arguments in non-internal functions;
if they should no longer do anything, just take them as normal and warn on use.
- Never add a new mandatory argument.

strega-nil-ms marked this conversation as resolved.
Show resolved Hide resolved
### Naming Variables

- `cmake_parse_arguments`: set prefix to `"arg"`
strega-nil-ms marked this conversation as resolved.
Show resolved Hide resolved
- Local variables are named with `snake_case`
- Internal global variable names are prefixed with `Z_VCPKG_`.
- External experimental global variable names are prefixed with `X_VCPKG_`.

- Internal functions are prefixed with `z_vcpkg_`
- Functions which are internal to a single function (i.e., helper functions)
are named `[z_]<func>_<name>`, where `<func>` is the name of the function they are
a helper to, and `<name>` is what the helper function does.
- `z_` should be added to the front if `<func>` doesn't have a `z_`,
but don't name a helper function `z_z_foo_bar`.
- Public global variables are named `VCPKG_`.
4 changes: 3 additions & 1 deletion scripts/buildsystems/vcpkg.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ mark_as_advanced(VCPKG_VERBOSE)

option(VCPKG_APPLOCAL_DEPS "Automatically copy dependencies into the output directory for executables." ON)
option(X_VCPKG_APPLOCAL_DEPS_SERIALIZED "(experimental) Add USES_TERMINAL to VCPKG_APPLOCAL_DEPS to force serialization." OFF)
option(X_VCPKG_APPLOCAL_DEPS_INSTALL "(experimental) Automatically copy dependencies into the install target directory for executables." OFF)

option(X_VCPKG_APPLOCAL_DEPS_INSTALL "(experimental) Automatically copy dependencies into the install target directory for executables. Requires CMake 3.14." OFF)

option(VCPKG_PREFER_SYSTEM_LIBS "Appends the vcpkg paths to CMAKE_PREFIX_PATH, CMAKE_LIBRARY_PATH and CMAKE_FIND_ROOT_PATH so that vcpkg libraries/packages are found after toolchain/system libraries/packages." OFF)

# Manifest options and settings
Expand Down
2 changes: 1 addition & 1 deletion scripts/detect_compiler/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.10)
cmake_minimum_required(VERSION 3.20)
project(detect_compiler NONE)

if(CMAKE_GENERATOR STREQUAL "Ninja" AND CMAKE_SYSTEM_NAME STREQUAL "Windows")
Expand Down
2 changes: 1 addition & 1 deletion scripts/get_cmake_vars/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.17)
cmake_minimum_required(VERSION 3.20)

set(VCPKG_LANGUAGES "C;CXX" CACHE STRING "Languages to enables for this project")

Expand Down
2 changes: 1 addition & 1 deletion scripts/ports.cmake
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# rebuild: 1
cmake_minimum_required(VERSION 3.5)
cmake_minimum_required(VERSION 3.20)

set(SCRIPTS "${CMAKE_CURRENT_LIST_DIR}" CACHE PATH "Location to stored scripts")
include("${SCRIPTS}/cmake/z_vcpkg_function_arguments.cmake")
Expand Down
7 changes: 0 additions & 7 deletions scripts/vcpkgTools.xml
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,6 @@
<sha512>1e81c37d3b144cfb81478140e7921f314134845d2f0e0f941109ef57d510e7bd37dda6cc292ec00782472e7f1671349b857be9aac1c3f974423c8d1875a46302</sha512>
<archiveName>cmake-3.20.2-linux-x86_64.tar.gz</archiveName>
</tool>
<tool name="cmake" os="freebsd">
<version>3.12.4</version>
<exeRelativePath>cmake-3.12.4-FreeBSD-x86_64/bin/cmake</exeRelativePath>
<url>https://github.com/ivysnow/CMake/releases/download/v3.12.4/cmake-3.12.4-FreeBSD-x86_64.tar.gz</url>
<sha512>b5aeb2de36f3c29757c9404e33756da88580ddfa07f29079c7f275ae0d6d018fdfe3f55d54d1403f38e359865cf93436e084c6b1ea91f26c88bc01dde3793479</sha512>
<archiveName>cmake-3.12.4-FreeBSD-x86_64.tar.gz</archiveName>
</tool>
<tool name="git" os="windows">
<version>2.26.2-1</version>
<exeRelativePath>mingw32\bin\git.exe</exeRelativePath>
Expand Down