[draft/RFC] Make tool ports first class citizens #31748

BillyONeal · 2023-06-01T00:57:07Z

There are several outstanding efforts trying to introduce dependencies from vcpkg to various host support tools, or that wish to provide tools. vcpkg has no less than 5 different ways to get these dependencies:

tool ports
vcpkg fetch
- known internally
- vcpkgTools.xml
vcpkg_find_acquire_program
vcpkg artifacts

None of these do everything we want, see the following support matrix:

This introduces a new helper port, vcpkg-find-acquire-tool, which exposes the guts of vcpkg_find_acquire_program for tool port consumption. Moreover, it uses the new VALIDATOR feature of find_program to close the gap between what vcpkg_find_acquire_program was able to do and vcpkg fetch is able to do.

Remaining work necessary to complete this:

Work currently blocked by this:

[vcpkg-rust] Rust support #30251 (rust support)
[boringssl] Update to 2023-04-20 #30986 (boringssl update)
[vcpkg-tool-ispc]add new port #29285 (vcpkg-tool-ispc)
[vcpkg-tool-meson] Update meson to a more recent version #28084 (meson update)
[vcpkg-tool-llvm] more tool ports #23659 (vcpkg-tool-llvm)
[vcpkg_configure_make] Misc fixes #31228 (tries to add vcpkg_find_acquire_program(VSCLANG))

Outstanding guidelines notes (to be added to based on other maintainers' comments):

Tool ports should NOT respect VCPKG_FORCE_SYSTEM_BINARIES; we believe that setting is poorly designed and will deprecate use of that setting going forward. Introducing VCPKG_FORCE_SYSTEM_<ToolName> might make sense.
Tool ports must not affect the behavior of ports that do not use bits exposed by the tool port. In particular, the existing vcpkg-tool-meson and vcpkg-tool-ninja break this rule by trying to influence what vcpkg_find_acquire_program does.
Tool ports must not do the actual work in their vcpkg-port-config.cmake, they must instead declare a function that does the actual work.
Tool ports include dependent vcpkg-port-config.cmake files in their own vcpkg-port.config.cmake files to ensure their functions are available.
Port cmake helper functions should be in a file named after their function to make it easy to find with Ctrl+P et al.
Tool ports can, but are not required to, use CMake cache entries; we expect most uses of tool port helper functions will only lookup the tool name once.
cmake_parse_arguments(PARSE_ARGV arg 1 "" "" "")

Check for required args:

    foreach(arg_name IN ITEMS  )
        if(NOT DEFINED "arg_${arg_name}")
            message(FATAL_ERROR "${arg_name} is required.")
        endif()
    endforeach()

Check for too many args:

    if(DEFINED arg_UNPARSED_ARGUMENTS)
        message(FATAL_ERROR "Unrecognized arguments: ${arg_UNPARSED_ARGUMENTS}")
    endif()

include_guard(GLOBAL)
Tool ports do not attempt to run their helper function inside portfile.cmake to make things fail early.
cmake-helpers include dependent vcpkg-port-config.cmake files in their own vcpkg-port-config.cmake file.

Reviewing individual commits highly recommended.

Drive by fix inverted sourceforge_args test. Drive by fix missing scripts in ports.cmake.

…wn instead.

…e it to avoid needing to do the 'temporarily editable hack'.

…ore we didn't have a version lock.

Neumann-A · 2023-06-01T06:33:50Z

Should we fix the couple of ports that seem to be accidentally getting the python3 interpreter when they appear to be making python bindings and thus should be using built-from-source Python here?

Yes

(have to check the rest of the PR)

ports/folly/portfile.cmake

ports/tensorflow-common/tensorflow-common.cmake

Neumann-A · 2023-06-01T06:38:24Z

ports/tensorflow-common/tensorflow-common.cmake

@@ -41,7 +41,8 @@ if(CMAKE_HOST_WIN32)
 	set(PYTHON3 "${MSYS_ROOT}/mingw64/bin/python3.exe")
 	vcpkg_execute_required_process(COMMAND ${PYTHON3} -c "import site; print(site.getsitepackages()[0])" WORKING_DIRECTORY ${CURRENT_BUILDTREES_DIR} LOGNAME prerequisites-pypath-${TARGET_TRIPLET} OUTPUT_VARIABLE PYTHON_LIB_PATH)


Is this also generating python bindings?

Good question. The tensorflow ports are broken for windows. #30582 (comment)

Neumann-A · 2023-06-01T06:39:01Z

ports/triton/portfile.cmake

@@ -17,7 +17,7 @@ vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS

 set(ADDITIONAL_OPTIONS "")
 if(PYTHON_BINDINGS)


did i miss that in the python bindings list?

The python bindings list was not exhaustive. Will investigate them as separate changes before this one lands.

Neumann-A · 2023-06-01T06:40:15Z

ports/vcpkg-find-acquire-tool/vcpkg_find_acquire_tool.cmake

+# This function is called through find_path or find_program to validate that the program works
+function(z_vcpkg_try_find_acquire_tool_validator result candidate)
+    vcpkg_execute_in_download_mode(
+        COMMAND ${candidate} $CACHE{z_vcpkg_try_find_acquire_tool_validator_VERSION_COMMAND}


Why do you use $CACHE here?

Because find_program passes no context information to the validator, we have to route things through "globals". Since these need to come from the cache, I said $CACHE to make that excruciatingly clear.

Would you prefer I just remove $CACHE or do you have better suggestions on how to smuggle this information into the validator?

Would you prefer I just remove $CACHE

Just remove CACHE. I don't see a reason to actually use that.

passes no context information to the validator, we have to route things through "globals"

The context is scoped through the call site. (as all function calls are in cmake)
Example:

cmake_minimum_required(VERSION 3.25) function(git_validator result item) message(STATUS "Scoped variable:${var}") set(${result} TRUE PARENT_SCOPE) endfunction() set(var "Hello from the validator!" ) find_program(GIT NAMES git VALIDATOR git_validator)

Output:

E:\vcpkg_folders>cmake -P find_git.cmake -- Scoped variable:Hello from the validator!

so no need to smuggle anything.

Neumann-A · 2023-06-01T06:41:18Z

ports/vcpkg-find-acquire-tool/vcpkg_find_acquire_tool.cmake

+    # Given VERSION_PREFIX is "my fancy program" and program_version_output is
+    # "my fancy program   1.2.0", extract the dots-numeric part
+    if(DEFINED CACHE{z_vcpkg_try_find_acquire_tool_validator_VERSION_PREFIX})


Can we pass a VERSION_REGEX instead? (or even a VERSION_VALIDATOR?)

I don't necessarily object to VERSION_REGEX; I just looked at what the existing tools we care about do, and for them prefix was sufficient. I'm not sure how to document what the resulting regex interface would look like; how does the regex tell us where the version part is located?

CMake regex' inability to assert positions at line boundaries does not help :(

Then version VERSION_VALIDATOR it is ;). Yeah i did not think about CMAKE_MATCH_X being needed for the implementation

Neumann-A · 2023-06-01T06:45:07Z

ports/vcpkg-tool-meson/portfile.cmake

@@ -20,7 +20,8 @@ set(supported_on_unix ON)
 set(version_command --version)
 set(extra_search_args EXACT_VERSION_MATCH)

-vcpkg_find_acquire_program(PYTHON3)
+include("${CMAKE_CURRENT_LIST_DIR}/../vcpkg-tool-python3-interpreter/vcpkg-port-config.cmake")


another include. Still necessary?

This one was not necessary, good point.

Neumann-A · 2023-06-01T06:46:24Z

ports/vcpkg-tool-python3-interpreter/vcpkg.json

@@ -0,0 +1,10 @@
+{
+  "name": "vcpkg-tool-python3-interpreter",
+  "version": "3.0.0",


Suggested change

"version": "3.0.0",

"version": "3",

?

Neumann-A · 2023-06-01T06:47:52Z

ports/vcpkg-tool-python3-interpreter/vcpkg.json

+  "dependencies": [
+    "vcpkg-find-acquire-tool"
+  ]


maybe add a feature which will force the usage of the python3 port ? Or would you consider that a antipattern?

I consider that an antipattern. This tool port is only for "I need a python to run a python script and I do not otherwise care about the python ecosystem"; if you otherwise care about that you need to depend on python.

Yes, find_package(PYTHON3 does not make this clear :(

This tool port is only for "I need a python to run a python script and I do not otherwise care about the python ecosystem"; if you otherwise care about that you need to depend on python.

But there should be a rule how the python port exposes its binary.

But there should be a rule how the python port exposes its binary.

I don't think the existing tool ports are consistent enough for that; not every tool refers to a single individual binary. Guidelines are just guidelines, if a tool port needs something different, that's going to be reasonable to do.

scripts/cmake/vcpkg_extract_source_archive.cmake

scripts/cmake/z_vcpkg_extract_archive.cmake

scripts/ports.cmake

Neumann-A · 2023-06-01T07:02:37Z

ports/angle/portfile.cmake

@@ -67,7 +67,7 @@ vcpkg_download_distfile(GNI_TO_CMAKE_PY
 )

 # Generate CMake files from GN / GNI files
-vcpkg_find_acquire_program(PYTHON3)
+vcpkg_find_acquire_python3_interpreter(PYTHON3)


vcpkg_find_acquire_python3_interpreter(PYTHON3)

Is kind of annoying and long. can we get something like:
vcpkg_get(<X>) and make that call vcpkg_find_acquire_<X>(X)

Having:

vcpkg_find_acquire_python3_interpreter(PYTHON3) # may need to be added to path vcpkg_find_acquire_meson(MESON) vcpkg_find_acquire_flex(FLEX) # may need to be added to path

is kind of verbose compared to something like

vcpkg_get(INTERPRETER python3 ADD_TO_PATH) vcpkg_get(PYSCRIPT meson) vcpkg_get(TOOL flex ADD_TO_PATH)

(the explicit dep in the manifest will still be verbose.)

dg0yt · 2023-06-01T07:12:19Z

Shall tool ports install tools to the installed tree (assets + fat artifacts, e.g. vcpkg-tool-bazel/gn/ninja), or put files in an ad-hoc location (buildtrees, downloads -> assets + thin artifacts, e.g. vcpkg-tool-lessmsi)?
Shall tool ports be able to fetch varying versions (e.g. gn or python may be needed with different exact versions by different consumers)?

Neumann-A · 2023-06-01T07:25:40Z

Main points for discussion in my eyes:

should there be an abstraction to obtain the tool [draft/RFC] Make tool ports first class citizens #31748 (comment)
should there be a VERSION_VALIDATOR [draft/RFC] Make tool ports first class citizens #31748 (comment)
currently the only example is only for external tools. How should tools be handled which need to be build? (What is the workflow?)

Tool ports should NOT respect VCPKG_FORCE_SYSTEM_BINARIES; we believe that setting is poorly designed and will deprecate use of that setting going forward. Introducing VCPKG_FORCE_SYSTEM_ might make sense.

How is VCPKG_FORCE_SYSTEM_BINARIES different from looping over all VCPKG_FORCE_SYSTEM_<ToolName> and setting them to true? I don't think VCPKG_FORCE_SYSTEM_<ToolName> is necessary. It can be forced in the triplet due to CACHE variables (and if you want to avoid abi hashing simply include some other cmake file setting the values.)

Tool ports must not do the actual work in their vcpkg-port-config.cmake, they must instead declare a function that does the actual work.

I am ok with that but explicitly calling out to vcpkg_find_acquire_python3_interpreter(PYTHON3) is kind of redundant to the automatic include(vcpkg-port-config.cmake) and automatically calling vcpkg_find_acquire_python3_interpreter(PYTHON3) at the end of it unless you want to do something like vcpkg_get(INTERPRETER python3 ADD_TO_PATH) and pass additional parameters or allow additional injection from the portfile into the function.

Check for required args:

maybe add a z_<function> which can test that.

cmake_parse_arguments(PARSE_ARGV arg 1 "" "" "")

it is cmake_parse_arguments(PARSE_ARGV 1 arg "" "" ""). Also 0 is OUTPUT var ?

Should cmake-helper to cmake-helper dependencies include the vcpkg-port-config.cmake file, or the individual function files?

I am for the chaining/former approach since otherwise another helper port has implementation details of the upstream port. Depending on vcpkg-port-config.cmake to be available is a vcpkg convention and not up to a port.

Tool ports can, but are not required to, use CMake cache entries; we expect most uses of tool port helper functions will only lookup the tool name once.

Not using CACHE entries means disallowing overwrites from the triplet.

Thomas1664 · 2023-06-01T08:36:17Z

ports/vcpkg-find-acquire-tool/vcpkg_find_acquire_tool.cmake

+        string(FIND
+            "${program_version_output}"
+            "$CACHE{z_vcpkg_try_find_acquire_tool_validator_VERSION_PREFIX}" prefix_offset)
+        # If there's no matching prefix, this isn't even the program we're looking for, so bail


Suggested change

# If there's no matching prefix, this isn't even the program we're looking for, so bail

# If there's no matching prefix, this isn't even the program we're looking for, so fail

?

I did mean bail but I agree fail is probably more universally understandable; changed.

scripts/cmake/vcpkg_find_acquire_program(NUGET).cmake

Thomas1664 · 2023-06-01T08:52:41Z

Should we fix the couple of ports that seem to be accidentally getting the python3 interpreter when they appear to be making python bindings and thus should be using built-from-source Python here?

Definitely yes!

IMO we should not build things like LLVM, Nodejs or Python from source when used as a tool.

BurningEnlightenment

Has it been considered to just outright drop Windows x86 32Bit support for (developer) tools? AFAICT there isn't any Windows 32Bit version with (extended) support left.

BurningEnlightenment · 2023-06-02T08:03:37Z

ports/vcpkg-tool-python3-interpreter/vcpkg_find_acquire_python3_interpreter.cmake

+        if(is64_bit)
+            vcpkg_find_acquire_tool(
+                OUT_TOOL_PATH out_var_value
+                OUT_TOOL_ACQUIRED tool_acquired
+                OUT_EXTRACTED_ROOT out_extract_root
+                TOOL_NAME python
+                VERSION ${program_version}
+                DOWNLOAD_FILENAME "python-${program_version}-embed-win32.zip"


Logic error: if (is64_bit) => embed-win32.zip

The branches need to be inverted.

Neumann-A · 2023-06-13T15:50:06Z

A single function as an interface, backed by a registry which is extended via tool ports. Something like ...

That goes into the direction of vcpkg_get i suggested in #31748 (comment). However, I currently come to the conclusion that is something which can be later introduced.

A port using vcpkg_find_acquire_program(bazel) could be upgraded to a new major version by just updating the port's the dependency on the tool port.

That will work independent of the approach.

Similar patterns may be seen in python, gn, llvm etc.. vcpkg just tried to avoid the topic. But if vcpkg is to stay, there will be more cases.

With python2 there is already precedence. But i think it will rather be the exception than the rule and stuff will be handled like CMake.

BillyONeal · 2023-06-19T21:59:24Z

Q: How are tool ports different than e.g. atlmfc ? A: atlmfc is always a system provided thing that the toolchain makes available by default; there is no information that needs to be conveyed into a build system to turn on its use.

BillyONeal · 2023-06-19T22:01:54Z

@dg0yt I don't think we're opposed to the concept of asking for a version here; we're just changing enough things with this proposal and that opens a whole can of worms with negotiation etc. that I don't want to hold the rest for. That's why I'm saying "not right now, but not necessarily opposed to the idea".

dg0yt · 2023-06-20T04:31:18Z

Q: How are tool ports different than e.g. atlmfc ? A: atlmfc is always a system provided thing that the toolchain makes available by default; there is no information that needs to be conveyed into a build system to turn on its use.

Hm, I thought about the atlmfc pattern when I proposed the VSCLANG tool.

atlmfc is needed for the target triplet. Most tools are desired for (i.e. to be run on) the host triplet.
The actual atlmfc and vsclang are expected to come with the system (toolchain). Other tools must be downloaded (and cached at some point).
atlmfc and tools must be made available to the actual build process.
- atlmfc does copying to include where it is found implicitly, but it shouldn't be needed.
- Some tool ports do copying to tools where they are found implicitly, but it shouldn't be needed.
- vfap and some tool ports provide the location explicitly only, via an output variable.
  AFAIU this is the proposed pattern here.
  However, this specific pattern more or less locks out user projects : they cannot call maintainer functions.

We depend on a significant set of such tools which are not yet sufficiently reflected: autoconf, automake, libtool, ... This is an issue because it leads to poor error reporting. (I know that ports should print there requirements, but this leads to a lot of redundant and inconsistent information.)

Neumann-A · 2023-07-10T19:30:05Z

So where do we stand here? I would like to see this move forward somehow to unblock a few PRs of mine ;)

dg0yt · 2023-08-07T07:16:02Z

So where do we stand here? I would like to see this move forward somehow to unblock a few PRs of mine ;)

PING

Osyotr · 2023-08-07T08:53:11Z

scripts/cmake/vcpkg_find_acquire_program(GO).cmake

-set(download_sha512 2a1e539ed628c0cca5935d24d22cf3a7165f5c80e12a4003ac184deae6a6d0aa31f582f3e8257b0730adfc09aeec3a0e62f4732e658c312d5382170bcd8c94d8)
+if(CMAKE_HOST_WIN32)
+    set(download_urls "https://dl.google.com/go/go${tool_subdirectory}.zip")
+    set(download_filename "go${tool_subdirectory}.zip")


nit: tool_subdirectory and paths_to_search should also be under if(CMAKE_HOST_WIN32). The latter must also be quoted.

Osyotr · 2023-08-07T09:06:22Z

scripts/cmake/vcpkg_find_acquire_program(NUGET).cmake

@@ -2,8 +2,10 @@ set(program_name nuget)
 set(tool_subdirectory "5.11.0")
 set(paths_to_search "${DOWNLOADS}/tools/nuget-${tool_subdirectory}-windows")


nit: this should also be under if(CMAKE_HOST_WIN32)

Osyotr · 2023-08-07T09:18:10Z

ports/vcpkg-tool-python3-interpreter/vcpkg_find_acquire_python3_interpreter.cmake

+            vcpkg_list(APPEND installed_pythons ${more_maybe_pythons})
+        endforeach()
+
+        if(is64_bit)


Would be nice to have arm64 check, too

I agree but for purposes of this PR I don't want to expand the feature set beyond what vcpkg_find_acquire_program does today.

Osyotr · 2023-08-07T09:20:42Z

ports/vcpkg-tool-python3-interpreter/vcpkg_find_acquire_python3_interpreter.cmake

+        endif()
+
+        if(tool_acquired)
+            file(REMOVE "${out_extract_root}/python310._pth")


I think there should be a comment about why this is needed in vcpkg (probably not in this PR)

I agree but I don't know. This is copy pasta from vcpkg_find_acquire_program I think.

Osyotr · 2023-08-07T09:25:10Z

ports/vcpkg-tool-meson/vcpkg.json

+  "dependencies": [
+    {
+      "name": "vcpkg-tool-python3-interpreter",
+      "host": true


For consistency, we should either remove host from dependencies from tool ports, or add them everywhere else (vcpkg-tool-python3-interpreter etc).

vcpkg-tool-python3-interpreter should probably be marked with "supports": "native" then?

It it already marked

I'll ask maintainers. I have a mild preference for removing it when "supports":"native" just by virtue of being shorter.

FTR supports can be overridden at the command line, or restrictions can be removed in future revisions.

Good point, this is a reason to keep it as it is

@ras0219-msft argues that 'not host:true' is more correct in the --allow-unsupported universe, because in, for example:

include("${CMAKE_CURRENT_LIST_DIR}/../vcpkg-find-acquire-tool/vcpkg-port-config.cmake")

this means the selected triplet for the dependency needs to match the selected triplet of the port.

Poll: We want host:true even when supports:native?

@JavierMatosD No.
@ras0219-msft No. (Strongly opposed to host:true.)
@BillyONeal @vicroms I'm convinced by @ras0219-msft
@data-queue No.

ports/openscap/vcpkg.json

Osyotr · 2023-08-07T09:43:22Z

ports/libxslt/portfile.cmake

@@ -18,7 +18,7 @@ vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
        "crypto"          LIBXSLT_WITH_CRYPTO
 )
 if("python" IN_LIST FEATURES)
-    vcpkg_find_acquire_program(PYTHON3)
+    vcpkg_find_acquire_python3_interpreter(PYTHON3)
    list(APPEND FEATURE_OPTIONS "-DPYTHON_EXECUTABLE=${PYTHON3}")
    list(APPEND FEATURE_OPTIONS_RELEASE "-DLIBXSLT_PYTHON_INSTALL_DIR=${CURRENT_PACKAGES_DIR}/lib/site-packages")


This looks wrong. Should probably be lib/python${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}/site-packages.
And the port should depend on python3 package.

I think we should move everything to lib/site-packages or tools/python3/lib/site-packages. There is no need to hardcode the python version. But that is another PR in my eyes.

BurningEnlightenment · 2023-09-25T18:31:29Z

Soo, since the Perl update in #33854 got green-lit, can we maybe agree to update some other rarely used / unlikely to break things like Go while waiting for this?

BillyONeal · 2023-09-25T18:35:56Z

Soo, since the Perl update in #33854 got green-lit, can we maybe agree to update some other rarely used / unlikely to break things like Go while waiting for this?

If the change is extremely unlikely to be breaking I think updates to vcpkg_find_acquire_program things are fair game. The things currently listed as blocked by this are either (1) totally new tools, or (2) things known to cause breakage. (As in, updating the tool version required corresponding changes in ports)

Notably, #33854 required no edits to ports/*. (If it did require such edits, it would not have been OK to merge)

…unctions. This updates the list of helper port scripts in ports.cmake to ensure it has every file in scripts/cmake, and changes the caller to conditionally call the function rather than conditionally include. This was extracted from microsoft#31748 but I'm not sure we want to do it.

This is a continuation of the work started in microsoft#31748 . There I tried to boil the ocean of all tools at the same time which turned into something unreviewable. Instead, this adds only python and meson, and a first caller of each.

vipcxj · 2024-03-09T15:13:48Z

I think vcpkg should provide a mechanism to globally set the tool version to be used, and each port can individually specify the tool version it needs, so that when a port needs a higher version of tool which not exists in vcpkg, it only needs to add this version of tool as a port and specify the port to use it, which won't affect any of the older ports.
Specifically, when relying on vcpkg-tool-meson as a host dependency, if the version is not specified, vcpkg should choose a specific reliable version instead of the latest version. When a port needs to use a newer meson, just specify the version number in the dependency. And for host dependencies like meson, the ontology must be allowed to have different versions at the same time. Different ports can use different mesons.

gerard-ryan-immersaview · 2024-05-09T04:48:00Z

Is there any update on this?

BillyONeal · 2024-06-20T06:29:38Z

Closing this as there has been no action in more than 4 months. Doesn't mean we are giving up on tool ports, but I don't think the comments here are going to be useful in a subsequent review

BillyONeal added 7 commits May 31, 2023 14:28

Extract z_vcpkg_extract_archive from vcpkg_find_acquire_program.

19e95f0

Drive by fix inverted sourceforge_args test. Drive by fix missing scripts in ports.cmake.

Remove 'supported_on_unix' and use whether a download location is kno…

9ffaf6f

…wn instead.

Remove extra_search_args and add exact_version_match.

2576e24

Fix z_vcpkg_fixup_rpath_in_dir to run when run rather than include.

db1aaba

Extract vcpkg_download_sourceforge from vcpkg_from_sourceforge and us…

fe39cc9

…e it to avoid needing to do the 'temporarily editable hack'.

Introduce vcpkg-find-acquire-tool and vcpkg-tool-python3-interpreter.

e8a6334

Reduce vcpkg-python3-interpreter version to reflect that on Linux bef…

89d8dfe

…ore we didn't have a version lock.

BillyONeal added info:internal This PR or Issue was filed by the vcpkg team. category:infrastructure Pertaining to the CI/Testing infrastrucutre labels Jun 1, 2023

FrankXie05 assigned jimwang118 Jun 1, 2023