From 30e2acd26d0cd85f7b06df4c4819d1e015cb13fa Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Thu, 4 Feb 2021 21:27:30 -0800 Subject: [PATCH 1/9] Add scripts tree extraction notes. --- docs/specifications/scripts-extraction.md | 50 +++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 docs/specifications/scripts-extraction.md diff --git a/docs/specifications/scripts-extraction.md b/docs/specifications/scripts-extraction.md new file mode 100644 index 00000000000000..d114c246497c22 --- /dev/null +++ b/docs/specifications/scripts-extraction.md @@ -0,0 +1,50 @@ +# Scripts Tree Extraction + +## Background + +We extracted vcpkg-tool as part of a future wherein Registries are the primary mechanism for interacting with the ports tree, which would allow the vcpkg tool and associated artifacts to be deployed and figure the rest out on their own. Unfortunately, we have concurrently edited things in the so called "scripts" tree which lives in support of ports but really probably belongs in the vcpkg-tool repo. + +Moreover, as part of stabilizing registries, the interface exposed by the scripts tree becomes contractual rather than something we can change in concert with ports, since we can no longer see the universe of ports to validate that changes are correct. + +To that end we are auditing the contents of the scripts tree to make sure it is a solid foundation for future work. + +## Audit Points + +The following are assertions we want to be able to make about the contents of the scripts tree. Note that this does *not* refer to `vcpkg.cmake` since that needs to work with older versions of cmake. + +These are design ideals that we may break in some limited cases where that makes sense. + +* We always use `cmake_parse_arguments` rather than function arguments. +* All `cmake_parse_arguments` use `PARSE_ARGV` for resistance to embedded semicolons. +* All `foreach` loops use `IN LISTS` for resistance to embedded semicolons. +* The variable `${ARGV}` is unreferenced. +* We use functions, not macros or top level code. +* 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) +* Local variables are not named anything special (not `Z_`). +* Internal global variable names are named `Z_VCPKG_`. +* External experimental global variable names are named `X_VCPKG_`. +* Public global variables are named `VCPKG_`. +* 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. +* `include()`s are removed and fixes to `port.cmake` et al. are made as necessary to avoid this. + +## Prognosis + +Not everything should remain in the scripts tree. As part of this audit, each helper will be dealt with in one of several ways: + +* Stay in scripts tree +* Deleted outright +* Moved to a tool port +* Deprecated + +# Current todo list + +Changes to be made: + +* `execute_process`' `OVERRIDDEN_EXECUTE_PROCESS` should get `Z_` +* `execute_process`' override should be a function rather than a macro +* Most of vcpkg_acquire_msys needs to become a tool port, except for the content of the mirror list. +* Calls to vcpkg_acquire_msys that call PACKAGES are deprecated; the tool port will use NO_DEFAULT_PACKAGES DIRECT_PACKAGES From 933f37d89976cb2e40bd48c14dc17cc754cf134e Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Thu, 4 Feb 2021 21:28:26 -0800 Subject: [PATCH 2/9] Add the execute_process fixes. --- scripts/cmake/execute_process.cmake | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/cmake/execute_process.cmake b/scripts/cmake/execute_process.cmake index 2207cfab25022d..422cf242496ce2 100644 --- a/scripts/cmake/execute_process.cmake +++ b/scripts/cmake/execute_process.cmake @@ -7,12 +7,12 @@ is enabled. In order to execute a process in Download Mode call `vcpkg_execute_in_download_mode()` instead. #]===] -if (NOT DEFINED OVERRIDEN_EXECUTE_PROCESS) - set(OVERRIDEN_EXECUTE_PROCESS ON) +if (NOT DEFINED Z_OVERRIDEN_EXECUTE_PROCESS) + set(Z_OVERRIDEN_EXECUTE_PROCESS ON) if (DEFINED VCPKG_DOWNLOAD_MODE) - macro(execute_process) + function(execute_process) message(FATAL_ERROR "This command cannot be executed in Download Mode.\nHalting portfile execution.\n") - endmacro() + endfunction() endif() endif() From 27df3b2805451e737ff92199ad35765adb87250b Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Fri, 5 Feb 2021 09:58:20 -0800 Subject: [PATCH 3/9] Update docs/specifications/scripts-extraction.md --- docs/specifications/scripts-extraction.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/specifications/scripts-extraction.md b/docs/specifications/scripts-extraction.md index d114c246497c22..9d1cd25744c93d 100644 --- a/docs/specifications/scripts-extraction.md +++ b/docs/specifications/scripts-extraction.md @@ -44,7 +44,7 @@ Not everything should remain in the scripts tree. As part of this audit, each he Changes to be made: -* `execute_process`' `OVERRIDDEN_EXECUTE_PROCESS` should get `Z_` -* `execute_process`' override should be a function rather than a macro +* ~~`execute_process`' `OVERRIDDEN_EXECUTE_PROCESS` should get `Z_`~~ +* ~~`execute_process`' override should be a function rather than a macro~~ * Most of vcpkg_acquire_msys needs to become a tool port, except for the content of the mirror list. * Calls to vcpkg_acquire_msys that call PACKAGES are deprecated; the tool port will use NO_DEFAULT_PACKAGES DIRECT_PACKAGES From 36d44dcf43d998d9c1b4d83ad4299c682b7b5cf2 Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Fri, 5 Feb 2021 10:46:46 -0800 Subject: [PATCH 4/9] Update docs/specifications/scripts-extraction.md --- docs/specifications/scripts-extraction.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/specifications/scripts-extraction.md b/docs/specifications/scripts-extraction.md index 9d1cd25744c93d..94ced9253c509d 100644 --- a/docs/specifications/scripts-extraction.md +++ b/docs/specifications/scripts-extraction.md @@ -23,6 +23,7 @@ These are design ideals that we may break in some limited cases where that makes * Local variables are not named anything special (not `Z_`). * Internal global variable names are named `Z_VCPKG_`. * External experimental global variable names are named `X_VCPKG_`. +* Internal functions are named `z_vcpkg_*` * Public global variables are named `VCPKG_`. * 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. From 407e5c68756fb2c8741bc3eaf12e5d890b846d36 Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Thu, 11 Feb 2021 15:56:14 -0800 Subject: [PATCH 5/9] add notes 2020-02-11 --- docs/specifications/scripts-extraction.md | 96 ++++++++++++++++++++--- 1 file changed, 87 insertions(+), 9 deletions(-) diff --git a/docs/specifications/scripts-extraction.md b/docs/specifications/scripts-extraction.md index 94ced9253c509d..420e344a9c1fee 100644 --- a/docs/specifications/scripts-extraction.md +++ b/docs/specifications/scripts-extraction.md @@ -20,11 +20,6 @@ These are design ideals that we may break in some limited cases where that makes * The variable `${ARGV}` is unreferenced. * We use functions, not macros or top level code. * 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) -* Local variables are not named anything special (not `Z_`). -* Internal global variable names are named `Z_VCPKG_`. -* External experimental global variable names are named `X_VCPKG_`. -* Internal functions are named `z_vcpkg_*` -* Public global variables are named `VCPKG_`. * 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. @@ -32,6 +27,15 @@ These are design ideals that we may break in some limited cases where that makes * `CACHE` variables are not used. * `include()`s are removed and fixes to `port.cmake` et al. are made as necessary to avoid this. +### 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_*` +* Public global variables are named `VCPKG_`. + ## Prognosis Not everything should remain in the scripts tree. As part of this audit, each helper will be dealt with in one of several ways: @@ -43,9 +47,83 @@ Not everything should remain in the scripts tree. As part of this audit, each he # Current todo list +Notes: +- Every `vcpkg_*_` should be extracted to a single `` port + Changes to be made: -* ~~`execute_process`' `OVERRIDDEN_EXECUTE_PROCESS` should get `Z_`~~ -* ~~`execute_process`' override should be a function rather than a macro~~ -* Most of vcpkg_acquire_msys needs to become a tool port, except for the content of the mirror list. -* Calls to vcpkg_acquire_msys that call PACKAGES are deprecated; the tool port will use NO_DEFAULT_PACKAGES DIRECT_PACKAGES +- [x] `execute_process`: + - [x] `execute_process`' `OVERRIDDEN_EXECUTE_PROCESS` should get `Z_` + - [x] `execute_process`' override should be a function rather than a macro +- [ ] `vcpkg_acquire_msys`: + - [ ] Most of vcpkg_acquire_msys needs to become a tool port, except for the content of the mirror list. + - [ ] Calls to vcpkg_acquire_msys that call PACKAGES are deprecated; the tool port will use NO_DEFAULT_PACKAGES DIRECT_PACKAGES +- [ ] `vcpkg_add_to_path`: + - [ ] audit +- [ ] `vcpkg_apply_patches`: + - [ ] audit + - [ ] deprecate, add `z_vcpkg_apply_patches` as an internal function +- [ ] `vcpkg_build_cmake`: + - [ ] extract to ports + - [ ] audit +- [ ] `vcpkg_build_gn`: + - [ ] audit + - [ ] deprecate in favor of `vcpkg_build_ninja` +- [ ] `vcpkg_build_make`: + - [ ] extract to ports + - [ ] audit + - [ ] remove/deprecate `ENABLE_INSTALL` (depending on whether it's used) (see also `INSTALL_TARGET`) + - [ ] look and see if `MAKEFILE` is used usefully vs `SUBPATH`; if not, deprecate/remove depending on whether it's used +- [ ] `vcpkg_build_msbuild`: + - [ ] deprecate + - [ ] audit + - [ ] update modern ports to use `vcpkg_install_msbuild()` +- [ ] `vcpkg_build_ninja`: + - [ ] extract to ports + - [ ] audit +- [ ] `vcpkg_build_nmake`: + - [ ] extract to ports + - [ ] audit +- [ ] `vcpkg_build_qmake`: + - [ ] extract to ports + - [ ] audit +- [ ] `vcpkg_buildpath_length_warning`: + - [ ] audit +- [ ] `vcpkg_check_features`: + - [ ] audit +- [ ] `vcpkg_check_linkage`: + - [ ] audit +- [ ] `vcpkg_clean_executables_in_bin`: + - [ ] deprecate, rename to `z_vcpkg_clean_executables_in_bin` + - [ ] audit +- [ ] `vcpkg_clean_msbuild`: + - [ ] extract to port + - [ ] audit +- [ ] `vcpkg_common_definitions`: + - [ ] audit +- [x] `vcpkg_common_functions`: + - already deprecated +- [ ] `vcpkg_configure_cmake`: + - [ ] extract to port + - [ ] audit +- [ ] `vcpkg_configure_gn`: + - [ ] extract to port + - [ ] audit +- [ ] `vcpkg_configure_make`: + - [ ] extract to port + - [ ] audit: _serious_ work + - Notes: use xcopy or robocopy +- [ ] `vcpkg_configure_meson`: + - [ ] extract to port + - [ ] audit +- [ ] `vcpkg_configure_qmake`: + - [ ] extract to port + - [ ] audit +- [ ] `vcpkg_copy_pdbs`: + - [ ] audit + - [ ] we may want to consider baking this into the tool, with a policy for disabling it +- [ ] `vcpkg_copy_sources`: + - [ ] add to scripts +- [ ] `vcpkg_copy_tool_dependencies`: + - [ ] add macOS stuff + - [ ] need to implement this into the vcpkg tool as opposed to using scripts From 478016072309892a569b7df6678889fdd8403995 Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Fri, 12 Feb 2021 19:33:22 +0000 Subject: [PATCH 6/9] further refinements also, remove the work list, to be replaced by issue #16188 --- docs/specifications/scripts-extraction.md | 133 +++++----------------- 1 file changed, 28 insertions(+), 105 deletions(-) diff --git a/docs/specifications/scripts-extraction.md b/docs/specifications/scripts-extraction.md index 420e344a9c1fee..a235f18cafbd99 100644 --- a/docs/specifications/scripts-extraction.md +++ b/docs/specifications/scripts-extraction.md @@ -8,122 +8,45 @@ Moreover, as part of stabilizing registries, the interface exposed by the script To that end we are auditing the contents of the scripts tree to make sure it is a solid foundation for future work. +The work list is contained in [Issue #16188]. + +[Issue #16188]: https://github.com/microsoft/vcpkg/issues/16188 + ## Audit Points The following are assertions we want to be able to make about the contents of the scripts tree. Note that this does *not* refer to `vcpkg.cmake` since that needs to work with older versions of cmake. These are design ideals that we may break in some limited cases where that makes sense. -* We always use `cmake_parse_arguments` rather than function arguments. -* All `cmake_parse_arguments` use `PARSE_ARGV` for resistance to embedded semicolons. -* All `foreach` loops use `IN LISTS` for resistance to embedded semicolons. -* The variable `${ARGV}` is unreferenced. -* We use functions, not macros or top level code. -* 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. -* `include()`s are removed and fixes to `port.cmake` et al. are made as necessary to avoid this. +- We always use `cmake_parse_arguments` rather than function parameters, or referring to `${ARG}`. + - Exception: there are exclusively positional parameters. This should be _very rare_. + - Note: in cases where there are positional parameters along with non-positional parameters, positional parameters should be referred to by `arg_UNPARSED_ARGUMENTS`. +- All `cmake_parse_arguments` use `PARSE_ARGV` for resistance to embedded semicolons. +- All `foreach` loops use `IN LISTS` for resistance to embedded semicolons. +- The variable `${ARGV}` is unreferenced. +- We use functions, not macros or top level code. +- 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. +- `include()`s are removed and fixes to `port.cmake` et al. are made as necessary to avoid this. ### 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_*` -* Public global variables are named `VCPKG_`. +- `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_*` +- Public global variables are named `VCPKG_`. ## Prognosis Not everything should remain in the scripts tree. As part of this audit, each helper will be dealt with in one of several ways: -* Stay in scripts tree -* Deleted outright -* Moved to a tool port -* Deprecated - -# Current todo list - -Notes: -- Every `vcpkg_*_` should be extracted to a single `` port - -Changes to be made: - -- [x] `execute_process`: - - [x] `execute_process`' `OVERRIDDEN_EXECUTE_PROCESS` should get `Z_` - - [x] `execute_process`' override should be a function rather than a macro -- [ ] `vcpkg_acquire_msys`: - - [ ] Most of vcpkg_acquire_msys needs to become a tool port, except for the content of the mirror list. - - [ ] Calls to vcpkg_acquire_msys that call PACKAGES are deprecated; the tool port will use NO_DEFAULT_PACKAGES DIRECT_PACKAGES -- [ ] `vcpkg_add_to_path`: - - [ ] audit -- [ ] `vcpkg_apply_patches`: - - [ ] audit - - [ ] deprecate, add `z_vcpkg_apply_patches` as an internal function -- [ ] `vcpkg_build_cmake`: - - [ ] extract to ports - - [ ] audit -- [ ] `vcpkg_build_gn`: - - [ ] audit - - [ ] deprecate in favor of `vcpkg_build_ninja` -- [ ] `vcpkg_build_make`: - - [ ] extract to ports - - [ ] audit - - [ ] remove/deprecate `ENABLE_INSTALL` (depending on whether it's used) (see also `INSTALL_TARGET`) - - [ ] look and see if `MAKEFILE` is used usefully vs `SUBPATH`; if not, deprecate/remove depending on whether it's used -- [ ] `vcpkg_build_msbuild`: - - [ ] deprecate - - [ ] audit - - [ ] update modern ports to use `vcpkg_install_msbuild()` -- [ ] `vcpkg_build_ninja`: - - [ ] extract to ports - - [ ] audit -- [ ] `vcpkg_build_nmake`: - - [ ] extract to ports - - [ ] audit -- [ ] `vcpkg_build_qmake`: - - [ ] extract to ports - - [ ] audit -- [ ] `vcpkg_buildpath_length_warning`: - - [ ] audit -- [ ] `vcpkg_check_features`: - - [ ] audit -- [ ] `vcpkg_check_linkage`: - - [ ] audit -- [ ] `vcpkg_clean_executables_in_bin`: - - [ ] deprecate, rename to `z_vcpkg_clean_executables_in_bin` - - [ ] audit -- [ ] `vcpkg_clean_msbuild`: - - [ ] extract to port - - [ ] audit -- [ ] `vcpkg_common_definitions`: - - [ ] audit -- [x] `vcpkg_common_functions`: - - already deprecated -- [ ] `vcpkg_configure_cmake`: - - [ ] extract to port - - [ ] audit -- [ ] `vcpkg_configure_gn`: - - [ ] extract to port - - [ ] audit -- [ ] `vcpkg_configure_make`: - - [ ] extract to port - - [ ] audit: _serious_ work - - Notes: use xcopy or robocopy -- [ ] `vcpkg_configure_meson`: - - [ ] extract to port - - [ ] audit -- [ ] `vcpkg_configure_qmake`: - - [ ] extract to port - - [ ] audit -- [ ] `vcpkg_copy_pdbs`: - - [ ] audit - - [ ] we may want to consider baking this into the tool, with a policy for disabling it -- [ ] `vcpkg_copy_sources`: - - [ ] add to scripts -- [ ] `vcpkg_copy_tool_dependencies`: - - [ ] add macOS stuff - - [ ] need to implement this into the vcpkg tool as opposed to using scripts +- Stay in scripts tree +- Deleted outright +- Moved to a tool port +- Deprecated From 432ef4d7f0bec38e920f28bdb8ab6325dc2e1be5 Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Fri, 12 Feb 2021 19:42:55 +0000 Subject: [PATCH 7/9] minor changes --- docs/specifications/scripts-extraction.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/specifications/scripts-extraction.md b/docs/specifications/scripts-extraction.md index a235f18cafbd99..589d627b5a026e 100644 --- a/docs/specifications/scripts-extraction.md +++ b/docs/specifications/scripts-extraction.md @@ -20,6 +20,11 @@ These are design ideals that we may break in some limited cases where that makes - We always use `cmake_parse_arguments` rather than function parameters, or referring to `${ARG}`. - Exception: there are exclusively positional parameters. This should be _very rare_. + - 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}`. - Note: in cases where there are positional parameters along with non-positional parameters, positional parameters should be referred to by `arg_UNPARSED_ARGUMENTS`. - All `cmake_parse_arguments` use `PARSE_ARGV` for resistance to embedded semicolons. - All `foreach` loops use `IN LISTS` for resistance to embedded semicolons. From ca293c748205c6ae38b23159b8cbf30d20f3da8a Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Fri, 12 Feb 2021 21:10:40 +0000 Subject: [PATCH 8/9] instructions for helper functions --- docs/specifications/scripts-extraction.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/specifications/scripts-extraction.md b/docs/specifications/scripts-extraction.md index 589d627b5a026e..02e89f823a4f01 100644 --- a/docs/specifications/scripts-extraction.md +++ b/docs/specifications/scripts-extraction.md @@ -45,6 +45,11 @@ These are design ideals that we may break in some limited cases where that makes - 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_`. ## Prognosis From 6b73607555fd56dad82bfab0bc736caf2238bea7 Mon Sep 17 00:00:00 2001 From: nicole mazzuca Date: Fri, 12 Feb 2021 22:33:12 +0000 Subject: [PATCH 9/9] foreach(RANGE) doesn't do what I thought it did --- docs/specifications/scripts-extraction.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/specifications/scripts-extraction.md b/docs/specifications/scripts-extraction.md index 02e89f823a4f01..a617a77413f735 100644 --- a/docs/specifications/scripts-extraction.md +++ b/docs/specifications/scripts-extraction.md @@ -37,6 +37,8 @@ These are design ideals that we may break in some limited cases where that makes - Out parameters only set `PARENT_SCOPE`. - `CACHE` variables are not used. - `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