-
Notifications
You must be signed in to change notification settings - Fork 7.6k
[scripts-audit] add guidelines for cmake #18319
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
Changes from 8 commits
e21bdba
0f35779
731ffec
e3bd0c5
e9c311d
d6759b0
8a9820b
8e46aef
1aadd2c
bed284c
957c851
35807ea
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,101 @@ | ||
| # 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<N>}`. | ||
| - 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<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`. | ||
| - 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. | ||
| - Example: `vcpkg_acquire_msys` has hard-coded packages and versions. | ||
| We believe that this is unacceptable. | ||
|
strega-nil-ms marked this conversation as resolved.
Outdated
|
||
| - 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. | ||
|
strega-nil-ms marked this conversation as resolved.
Outdated
|
||
| - 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 `<start>` _must always be_ less than or equal to `<stop>`. | ||
| - 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 `unset(VAR)` to unset a variable, | ||
| and `set(VAR "")` to set it to an empty string. | ||
|
strega-nil-ms marked this conversation as resolved.
Outdated
|
||
|
|
||
| ### CMake Versions to Require | ||
|
|
||
| - All CMake scripts, except for `vcpkg.cmake`, | ||
|
strega-nil-ms marked this conversation as 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.
|
||
| - 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 | ||
|
Contributor
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. For
Contributor
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 sentence is nonsense and should be ignored. I think the correct phrasing would be that
Contributor
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. 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.)
Contributor
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. What is the reason to keep it at 3.1?
which is relevant for dealing with transitive dependencies.
Contributor
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. 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.
|
||
| ### Naming Variables | ||
|
|
||
| - `cmake_parse_arguments`: set prefix to `"arg"` | ||
|
strega-nil-ms marked this conversation as resolved.
|
||
| - local variables are named `snake_case` | ||
|
strega-nil-ms marked this conversation as resolved.
Outdated
|
||
| - Internal global variable names are named `Z_VCPKG_`. | ||
|
strega-nil-ms marked this conversation as resolved.
Outdated
|
||
| - External experimental global variable names are named `X_VCPKG_`. | ||
|
strega-nil-ms marked this conversation as resolved.
Outdated
|
||
| - Internal functions are named `z_vcpkg_*` | ||
|
strega-nil-ms marked this conversation as resolved.
Outdated
|
||
| - 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_`. | ||
Uh oh!
There was an error while loading. Please reload this page.