From e21bdba8e6950da3547cf5fa38e9324115066d51 Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Mon, 7 Jun 2021 12:35:43 -0700 Subject: [PATCH 01/11] [scripts-audit] add guidelines for cmake --- docs/README.md | 1 + docs/maintainers/cmake-guidelines.md | 55 ++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 docs/maintainers/cmake-guidelines.md diff --git a/docs/README.md b/docs/README.md index 04d9557112125b..7ef5c0d0659269 100644 --- a/docs/README.md +++ b/docs/README.md @@ -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 diff --git a/docs/maintainers/cmake-guidelines.md b/docs/maintainers/cmake-guidelines.md new file mode 100644 index 00000000000000..966269a72f8b75 --- /dev/null +++ b/docs/maintainers/cmake-guidelines.md @@ -0,0 +1,55 @@ +# 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 + +- We always use `cmake_parse_arguments` rather than function parameters, or referring to `${ARG}`. + - This doesn't need to be followed for "script-local helper functions" + - Exception: exclusively positional parameters, like out variables. + - In this case, positional parameters should be put in the function declaration + (rather than using `${ARG}`), 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}") after checking `${ARGC}`. +- There are no unparsed or unused arguments. Always check for `ARGN` or `arg_UNPARSED_ARGUMENTS`, + and either `FATAL_ERROR`, or `WARN` if necessary for backwards compatibility. +- All `cmake_parse_arguments` use `PARSE_ARGV` for resistance to embedded semicolons. +- All `foreach` loops use `IN LISTS` for resistance to embedded semicolons. +- 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: `vcpkg.cmake`'s `find_package` +- Scripts in the scripts tree should not be expected to need changes as part of normal operation. (For example, `vcpkg_acquire_msys` has hard coded specific packages and versions thereof used which we believe is unacceptable) +- All non-splat variable expansions are in quotes "". +- There are no "pointer" parameters (where a user passes a variable name rather than the contents) except for out parameters. +- Undefined names are not referenced. +- Out parameters only set `PARENT_SCOPE`. +- `CACHE` variables are not used, except for internal global variables which are used to avoid duplicating work. +- `include()`s are removed and fixes to `port.cmake` et al. are made as necessary to avoid this. +- `foreach(RANGE)`'s arguments _must always be_ natural numbers, and `` _must always be_ less than or equal to ``. + - This should be checked. + +### Naming Variables + +- `cmake_parse_arguments`: set prefix to `"arg"` +- local variables are named `snake_case` +- Internal global variable names are named `Z_VCPKG_`. +- External experimental global variable names are named `X_VCPKG_`. +- Internal functions are named `z_vcpkg_*` + - Functions which are internal to a single function (i.e., helper functions) + are named `[z_]_`, where `` is the name of the function they are + a helper to, and `` is what the helper function does. + - `z_` should be added to the front if `` doesn't have a `z_`, + but don't name a helper function `z_z_foo_bar`. +- Public global variables are named `VCPKG_`. From 0f35779d8fca73bc74b0ad84c1934932dc34945e Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Mon, 7 Jun 2021 14:00:50 -0700 Subject: [PATCH 02/11] cr changes --- docs/maintainers/cmake-guidelines.md | 62 +++++++++++++++++++--------- 1 file changed, 42 insertions(+), 20 deletions(-) diff --git a/docs/maintainers/cmake-guidelines.md b/docs/maintainers/cmake-guidelines.md index 966269a72f8b75..658b20f77f69f0 100644 --- a/docs/maintainers/cmake-guidelines.md +++ b/docs/maintainers/cmake-guidelines.md @@ -7,38 +7,60 @@ We expect that all CMake scripts that are either: 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. +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 -- We always use `cmake_parse_arguments` rather than function parameters, or referring to `${ARG}`. +- We always use `cmake_parse_arguments` rather than function parameters, + or referring to `${ARG}`. - This doesn't need to be followed for "script-local helper functions" - Exception: exclusively positional parameters, like out variables. - - In this case, positional parameters should be put in the function declaration - (rather than using `${ARG}`), 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}") after checking `${ARGC}`. -- There are no unparsed or unused arguments. Always check for `ARGN` or `arg_UNPARSED_ARGUMENTS`, - and either `FATAL_ERROR`, or `WARN` if necessary for backwards compatibility. -- All `cmake_parse_arguments` use `PARSE_ARGV` for resistance to embedded semicolons. -- All `foreach` loops use `IN LISTS` for resistance to embedded semicolons. -- The variables `${ARGV}` and `${ARGN}` are unreferenced, except in helpful messages to the user. + - In this case, positional parameters should be put in the function + declaration (rather than using `${ARG}`), + 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}")`, after checking `ARGC`. +- 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: `vcpkg.cmake`'s `find_package` -- Scripts in the scripts tree should not be expected to need changes as part of normal operation. (For example, `vcpkg_acquire_msys` has hard coded specific packages and versions thereof used which we believe is unacceptable) -- All non-splat variable expansions are in quotes "". -- There are no "pointer" parameters (where a user passes a variable name rather than the contents) except for out parameters. +- Scripts in the scripts tree should not be expected to need changes + as part of normal operation. + - Example: `vcpkg_acquire_msys` has hard-coded packages and versions. + We believe that this is unacceptable. +- 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" parameters + (where a user passes a variable name rather than the contents) + except for out parameters. - Undefined names are not referenced. -- Out parameters only set `PARENT_SCOPE`. -- `CACHE` variables are not used, except for internal global variables which are used to avoid duplicating work. -- `include()`s are removed and fixes to `port.cmake` et al. are made as necessary to avoid this. -- `foreach(RANGE)`'s arguments _must always be_ natural numbers, and `` _must always be_ less than or equal to ``. - - This should be checked. +- Out parameters are only set in `PARENT_SCOPE`. +- `CACHE` variables are not used. + - Exception: internal global variables to avoid duplicating work. +- `include()`s are only allowed in `ports.cmake` or `vcpkg-port-config.cmake`. +- `foreach(RANGE)`'s arguments _must always be_ natural numbers, + and `` _must always be_ less than or equal to ``. + - This must be checked if necessary. ### Naming Variables From 731ffec79de08ed778f290a55b597c96e0be2e00 Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Mon, 7 Jun 2021 14:10:57 -0700 Subject: [PATCH 03/11] Neumann-A CRs --- docs/maintainers/cmake-guidelines.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/maintainers/cmake-guidelines.md b/docs/maintainers/cmake-guidelines.md index 658b20f77f69f0..c78a2c5eb7e403 100644 --- a/docs/maintainers/cmake-guidelines.md +++ b/docs/maintainers/cmake-guidelines.md @@ -33,7 +33,7 @@ We hope that they will make both forwards and backwards compatibility easier. 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: `vcpkg.cmake`'s `find_package` + - Exception: `vcpkg.cmake`'s `find_package`. - Scripts in the scripts tree should not be expected to need changes as part of normal operation. - Example: `vcpkg_acquire_msys` has hard-coded packages and versions. @@ -61,6 +61,8 @@ We hope that they will make both forwards and backwards compatibility easier. - `foreach(RANGE)`'s arguments _must always be_ natural numbers, and `` _must always be_ less than or equal to ``. - This must be checked if necessary. +- All port-based scripts must use `include_guard(GLOBAL)` + to avoid being included multiple times. ### Naming Variables From e3bd0c57db5772c7800ce2c2299ff05dbc4b7922 Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Mon, 7 Jun 2021 14:35:39 -0700 Subject: [PATCH 04/11] add guidelines on minimum CMake version get_cmake_vars requires version 3.17, thus all scripts should require 3.17 update cmake_minimum_required in scripts/ports.cmake --- docs/maintainers/cmake-guidelines.md | 17 +++++++++++++++++ scripts/buildsystems/vcpkg.cmake | 2 ++ scripts/detect_compiler/CMakeLists.txt | 2 +- scripts/ports.cmake | 2 +- 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/docs/maintainers/cmake-guidelines.md b/docs/maintainers/cmake-guidelines.md index c78a2c5eb7e403..23801841391242 100644 --- a/docs/maintainers/cmake-guidelines.md +++ b/docs/maintainers/cmake-guidelines.md @@ -64,6 +64,23 @@ We hope that they will make both forwards and backwards compatibility easier. - All port-based scripts must use `include_guard(GLOBAL)` to avoid being included multiple times. +### CMake Versions to Require + +- All CMake scripts, except for `vcpkg.cmake`, + may assume the version of CMake that is present in the + `cmake_minimum_required` of `ports.cmake`. +- `vcpkg.cmake` must assume a version of CMake back to 3.1 in general + - 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. + ### Naming Variables - `cmake_parse_arguments`: set prefix to `"arg"` diff --git a/scripts/buildsystems/vcpkg.cmake b/scripts/buildsystems/vcpkg.cmake index d9a7fb562b3185..98a7e1ebefe5d0 100644 --- a/scripts/buildsystems/vcpkg.cmake +++ b/scripts/buildsystems/vcpkg.cmake @@ -46,6 +46,8 @@ 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) + +# requires CMake 3.14 option(X_VCPKG_APPLOCAL_DEPS_INSTALL "(experimental) Automatically copy dependencies into the install target directory for executables." OFF) # Manifest options and settings diff --git a/scripts/detect_compiler/CMakeLists.txt b/scripts/detect_compiler/CMakeLists.txt index 5ae17c5b27f385..a86bd2a3c596dd 100644 --- a/scripts/detect_compiler/CMakeLists.txt +++ b/scripts/detect_compiler/CMakeLists.txt @@ -1,4 +1,4 @@ -cmake_minimum_required(VERSION 3.10) +cmake_minimum_required(VERSION 3.17) project(detect_compiler NONE) if(CMAKE_GENERATOR STREQUAL "Ninja" AND CMAKE_SYSTEM_NAME STREQUAL "Windows") diff --git a/scripts/ports.cmake b/scripts/ports.cmake index d91c81b0b284e7..b4e31b51135926 100644 --- a/scripts/ports.cmake +++ b/scripts/ports.cmake @@ -1,5 +1,5 @@ # rebuild: 1 -cmake_minimum_required(VERSION 3.5) +cmake_minimum_required(VERSION 3.17) set(SCRIPTS "${CMAKE_CURRENT_LIST_DIR}" CACHE PATH "Location to stored scripts") include("${SCRIPTS}/cmake/z_vcpkg_function_arguments.cmake") From e9c311dbc26c2f60854d111d0e9ec28b87901b7e Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Mon, 7 Jun 2021 15:00:22 -0700 Subject: [PATCH 05/11] require 3.20 --- docs/maintainers/cmake-guidelines.md | 3 +++ scripts/detect_compiler/CMakeLists.txt | 2 +- scripts/get_cmake_vars/CMakeLists.txt | 2 +- scripts/ports.cmake | 2 +- scripts/vcpkgTools.xml | 7 ------- 5 files changed, 6 insertions(+), 10 deletions(-) diff --git a/docs/maintainers/cmake-guidelines.md b/docs/maintainers/cmake-guidelines.md index 23801841391242..0f9582a4d6de07 100644 --- a/docs/maintainers/cmake-guidelines.md +++ b/docs/maintainers/cmake-guidelines.md @@ -69,6 +69,9 @@ We hope that they will make both forwards and backwards compatibility easier. - All CMake scripts, except for `vcpkg.cmake`, may assume the version of CMake that is present in the `cmake_minimum_required` of `ports.cmake`. + - 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 - Specific functions and options may assume a greater CMake version; if they do, make sure to comment that function or option diff --git a/scripts/detect_compiler/CMakeLists.txt b/scripts/detect_compiler/CMakeLists.txt index a86bd2a3c596dd..c55da9ebb2bdeb 100644 --- a/scripts/detect_compiler/CMakeLists.txt +++ b/scripts/detect_compiler/CMakeLists.txt @@ -1,4 +1,4 @@ -cmake_minimum_required(VERSION 3.17) +cmake_minimum_required(VERSION 3.20) project(detect_compiler NONE) if(CMAKE_GENERATOR STREQUAL "Ninja" AND CMAKE_SYSTEM_NAME STREQUAL "Windows") diff --git a/scripts/get_cmake_vars/CMakeLists.txt b/scripts/get_cmake_vars/CMakeLists.txt index 2b0bd671a51832..084042fb116ab2 100644 --- a/scripts/get_cmake_vars/CMakeLists.txt +++ b/scripts/get_cmake_vars/CMakeLists.txt @@ -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") diff --git a/scripts/ports.cmake b/scripts/ports.cmake index b4e31b51135926..58cb1e5533cf6f 100644 --- a/scripts/ports.cmake +++ b/scripts/ports.cmake @@ -1,5 +1,5 @@ # rebuild: 1 -cmake_minimum_required(VERSION 3.17) +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") diff --git a/scripts/vcpkgTools.xml b/scripts/vcpkgTools.xml index 76f2a5301357a0..49b86e92be9658 100644 --- a/scripts/vcpkgTools.xml +++ b/scripts/vcpkgTools.xml @@ -28,13 +28,6 @@ 1e81c37d3b144cfb81478140e7921f314134845d2f0e0f941109ef57d510e7bd37dda6cc292ec00782472e7f1671349b857be9aac1c3f974423c8d1875a46302 cmake-3.20.2-linux-x86_64.tar.gz - - 3.12.4 - cmake-3.12.4-FreeBSD-x86_64/bin/cmake - https://github.com/ivysnow/CMake/releases/download/v3.12.4/cmake-3.12.4-FreeBSD-x86_64.tar.gz - b5aeb2de36f3c29757c9404e33756da88580ddfa07f29079c7f275ae0d6d018fdfe3f55d54d1403f38e359865cf93436e084c6b1ea91f26c88bc01dde3793479 - cmake-3.12.4-FreeBSD-x86_64.tar.gz - 2.26.2-1 mingw32\bin\git.exe From d6759b0f9054868cfeceaae71c51e1988fe654e4 Mon Sep 17 00:00:00 2001 From: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com> Date: Mon, 7 Jun 2021 16:43:52 -0700 Subject: [PATCH 06/11] Update scripts/buildsystems/vcpkg.cmake Co-authored-by: ras0219 <533828+ras0219@users.noreply.github.com> --- scripts/buildsystems/vcpkg.cmake | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/buildsystems/vcpkg.cmake b/scripts/buildsystems/vcpkg.cmake index 98a7e1ebefe5d0..22a3e9d2704f63 100644 --- a/scripts/buildsystems/vcpkg.cmake +++ b/scripts/buildsystems/vcpkg.cmake @@ -47,8 +47,7 @@ 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) -# requires CMake 3.14 -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) # Manifest options and settings if(NOT DEFINED VCPKG_MANIFEST_DIR) From 8a9820bd899c5f0cec01ae2e5e8762900b7d68d9 Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Wed, 9 Jun 2021 11:17:25 -0700 Subject: [PATCH 07/11] suggestion from dg0yt --- docs/maintainers/cmake-guidelines.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/maintainers/cmake-guidelines.md b/docs/maintainers/cmake-guidelines.md index 0f9582a4d6de07..c8b47b403df82d 100644 --- a/docs/maintainers/cmake-guidelines.md +++ b/docs/maintainers/cmake-guidelines.md @@ -63,6 +63,7 @@ We hope that they will make both forwards and backwards compatibility easier. - This must be checked if necessary. - All port-based scripts must use `include_guard(GLOBAL)` to avoid being included multiple times. +- `set(VAR )` should not be used. Use `set(VAR)` to unset a variable. ### CMake Versions to Require From 8e46aefc0e3200be45a7ccf91c48bf5168ff9560 Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Wed, 9 Jun 2021 13:00:47 -0700 Subject: [PATCH 08/11] okay dg0yt is right --- docs/maintainers/cmake-guidelines.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/maintainers/cmake-guidelines.md b/docs/maintainers/cmake-guidelines.md index c8b47b403df82d..c89ead4f70a2b7 100644 --- a/docs/maintainers/cmake-guidelines.md +++ b/docs/maintainers/cmake-guidelines.md @@ -63,7 +63,8 @@ We hope that they will make both forwards and backwards compatibility easier. - This must be checked if necessary. - All port-based scripts must use `include_guard(GLOBAL)` to avoid being included multiple times. -- `set(VAR )` should not be used. Use `set(VAR)` to unset a variable. +- `set(VAR )` should not be used. Use `unset(VAR)` to unset a variable, + and `set(VAR "")` to set it to an empty string. ### CMake Versions to Require From 1aadd2cc853d11df610c1eafa50a46031cf704f3 Mon Sep 17 00:00:00 2001 From: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com> Date: Wed, 9 Jun 2021 16:34:47 -0700 Subject: [PATCH 09/11] Apply suggestions from code review thanks robert! Co-authored-by: Robert Schumacher --- docs/maintainers/cmake-guidelines.md | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/docs/maintainers/cmake-guidelines.md b/docs/maintainers/cmake-guidelines.md index c89ead4f70a2b7..35f4cb465893cd 100644 --- a/docs/maintainers/cmake-guidelines.md +++ b/docs/maintainers/cmake-guidelines.md @@ -15,7 +15,7 @@ We hope that they will make both forwards and backwards compatibility easier. ## The Guidelines -- We always use `cmake_parse_arguments` rather than function parameters, +- We always use `cmake_parse_arguments()` rather than function parameters, or referring to `${ARG}`. - This doesn't need to be followed for "script-local helper functions" - Exception: exclusively positional parameters, like out variables. @@ -34,10 +34,10 @@ We hope that they will make both forwards and backwards compatibility easier. - (i.e., `message(FATAL_ERROR "blah was passed extra arguments: ${ARGN}")`) - We always use functions, not macros or top level code. - Exception: `vcpkg.cmake`'s `find_package`. -- Scripts in the scripts tree should not be expected to need changes +- Scripts in the scripts tree should not be expected to need observable changes as part of normal operation. - - Example: `vcpkg_acquire_msys` has hard-coded packages and versions. - We believe that this is unacceptable. + - 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: @@ -53,7 +53,8 @@ We hope that they will make both forwards and backwards compatibility easier. - There are no "pointer" parameters (where a user passes a variable name rather than the contents) except for out parameters. -- Undefined names are not referenced. +- 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`. - `CACHE` variables are not used. - Exception: internal global variables to avoid duplicating work. @@ -64,7 +65,7 @@ We hope that they will make both forwards and backwards compatibility easier. - 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 an empty string. + 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 @@ -89,10 +90,11 @@ We hope that they will make both forwards and backwards compatibility easier. ### Naming Variables - `cmake_parse_arguments`: set prefix to `"arg"` -- local variables are named `snake_case` -- Internal global variable names are named `Z_VCPKG_`. -- External experimental global variable names are named `X_VCPKG_`. -- Internal functions are named `z_vcpkg_*` +- 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_]_`, where `` is the name of the function they are a helper to, and `` is what the helper function does. From 957c85158f818a56fffd02d933f47fc27abf61d0 Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Fri, 2 Jul 2021 09:02:45 -0700 Subject: [PATCH 10/11] update guidelines slightly --- docs/maintainers/cmake-guidelines.md | 41 ++++++++++++++++++---------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/docs/maintainers/cmake-guidelines.md b/docs/maintainers/cmake-guidelines.md index 35f4cb465893cd..6bdc6529463b3f 100644 --- a/docs/maintainers/cmake-guidelines.md +++ b/docs/maintainers/cmake-guidelines.md @@ -15,10 +15,17 @@ We hope that they will make both forwards and backwards compatibility easier. ## The Guidelines -- We always use `cmake_parse_arguments()` rather than function parameters, - or referring to `${ARG}`. - - This doesn't need to be followed for "script-local helper functions" - - Exception: exclusively positional parameters, like out variables. +- Except for out-parameters, we always use `cmake_parse_arguments()` + rather than function parameters or referring to `${ARG}`. + - 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() + ``` + - 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}`), and should be named according to local rules (i.e. `snake_case`). @@ -33,6 +40,8 @@ We hope that they will make both forwards and backwards compatibility easier. 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. @@ -42,7 +51,7 @@ We hope that they will make both forwards and backwards compatibility easier. except those which are intended to be passed as multiple arguments. - Example: ```cmake - set(working_directory) + set(working_directory "") if(DEFINED arg_WORKING_DIRECTORY) set(working_directory "WORKING_DIRECTORY" "${arg_WORKING_DIRECTORY}") endif() @@ -50,22 +59,24 @@ We hope that they will make both forwards and backwards compatibility easier. # else calls do_the_thing(WORKING_DIRECTORY "${arg_WORKING_DIRECTORY}") do_the_thing(${working_directory}) ``` -- There are no "pointer" parameters - (where a user passes a variable name rather than the contents) - except for 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 "")`. +- 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`. -- `CACHE` variables are not used. - - Exception: internal global variables to avoid duplicating work. +- Out parameters are only set in `PARENT_SCOPE`, and are never read. +- `CACHE` variables are used only for global variables which are shared among functions, + and for internal state to avoid duplicating work. - `include()`s are only allowed in `ports.cmake` or `vcpkg-port-config.cmake`. - `foreach(RANGE)`'s arguments _must always be_ natural numbers, and `` _must always be_ less than or equal to ``. - - This must be checked if necessary. + - This must be checked. - 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_ +- `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 From 35807ea368113327a70dfa33a5d948692bbdb6ac Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Tue, 6 Jul 2021 09:40:17 -0700 Subject: [PATCH 11/11] Robert CRs --- docs/maintainers/cmake-guidelines.md | 33 +++++++++++++++++++--------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/docs/maintainers/cmake-guidelines.md b/docs/maintainers/cmake-guidelines.md index 6bdc6529463b3f..77cdf737204148 100644 --- a/docs/maintainers/cmake-guidelines.md +++ b/docs/maintainers/cmake-guidelines.md @@ -17,6 +17,12 @@ We hope that they will make both forwards and backwards compatibility easier. - Except for out-parameters, we always use `cmake_parse_arguments()` rather than function parameters or referring to `${ARG}`. + - 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}`), + 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}")`, after checking `ARGC`. - Out-parameters should be the first parameter to a function. Example: ```cmake function(format out_var) @@ -25,12 +31,6 @@ We hope that they will make both forwards and backwards compatibility easier. set("${out_var}" "${buffer}" PARENT_SCOPE) endfunction() ``` - - 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}`), - 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}")`, after checking `ARGC`. - 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. @@ -66,13 +66,26 @@ We hope that they will make both forwards and backwards compatibility easier. 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. -- `CACHE` variables are used only for global variables which are shared among functions, - and for internal state to avoid duplicating work. +- 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 `` _must always be_ less than or equal to ``. - - This must be checked. + - 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,