[vcpkg] Fix toolchain compatibility with cmake < 3.15 #18898

dg0yt · 2021-07-10T07:35:38Z

Since 876e67c, the toolchain used list(PREPEND ...) by default. But this cmake command was introduced in cmake 3.15, and the toolchain is meant to work with 3.1 [!].

What does your PR fix?

This PR restores compatibility with cmake < 3.15 for user projects, fixing:

CMake Error at /home/dg0yt/Projekte/vcpkg/vcpkg/scripts/buildsystems/vcpkg.cmake:390 (list):
  list does not recognize sub-command PREPEND

Tested with CMake 3.10 (Ubuntu 18.04).

Which triplets are supported/not supported? Have you updated the CI baseline?

all, no
Does your PR follow the maintainer guide?

yes
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

not needed

dg0yt · 2021-07-10T07:41:18Z

CI didn't rebuild a single port, despite touching vcpkg.cmake.
There is no automated test of building at least a dummy project with the vcpkg toolchain and a supported ancient version of cmake.

dg0yt · 2021-07-10T08:40:15Z

CC @autoantwort This amends #17336.

cenit · 2021-07-10T16:47:40Z

CI didn't rebuild a single port, despite touching vcpkg.cmake.

There is no automated test of building at least a dummy project with the vcpkg toolchain and a supported ancient version of cmake.

a long standing very serious issue
(it has already let slip some bad bugs into master...)

@strega-nil

autoantwort · 2021-07-10T16:59:30Z

a long standing very serious issue
(it has already let slip some bad bugs into master...)

@strega-nil

There is already microsoft/vcpkg-tool#80

dg0yt · 2021-07-10T17:42:03Z

There is already microsoft/vcpkg-tool#80

Not sure if you really want to invalidate binary artifacts for each change, even documentation typos.

And this won't add testing for cmake versions which are claimed to be supported (cf. https://github.com/microsoft/vcpkg/pull/18319/files#r652632414).
(Nobody noticed the current issue until now. Is cmake 3.1 support really needed?)

FTR, you can't work around the current bug with VCPKG_PREFER_SYSTEM_LIBS=ON because this is not passed from the user project to cmake ports built by vcpkg, including detect_compiler and get_cmake_vars.

autoantwort · 2021-07-10T17:50:55Z

(Nobody noticed the current issue until now. Is cmake 3.1 support really needed?)

I thought the same, I searched for an issue complaining about it but couldn't find one.

strega-nil · 2021-07-11T13:51:19Z

scripts/buildsystems/vcpkg.cmake

 if(VCPKG_PREFER_SYSTEM_LIBS)
-    set(Z_VCPKG_PATH_LIST_OP APPEND)
+    list(LENGTH CMAKE_PREFIX_PATH Z_VCPKG_PREFIX_PATH_POS)
+    list(LENGTH CMAKE_LIBRARY_PATH Z_VCPKG_LIBRARY_PATH_POS)
+    list(LENGTH CMAKE_FIND_ROOT_PATH Z_VCPKG_FIND_ROOT_PATH_POS)
 else()
-    set(Z_VCPKG_PATH_LIST_OP PREPEND)
+    set(Z_VCPKG_PREFIX_PATH_POS 0)
+    set(Z_VCPKG_LIBRARY_PATH_POS 0)
+    set(Z_VCPKG_FIND_ROOT_PATH_POS 0)
 endif()


One could probably do:

function(z_vcpkg_path_list_op out_var lst paths) if(VCPKG_PREFER_SYSTEM_LIBS) set("${out_var}" "${lst};${paths}" PARENT_SCOPE) else() set("${out_var}" "${paths};${lst}" PARENT_SCOPE) endif() endfunction()

Does "one could probably do" mean I should update the PR? I would prefer to stick to plain cmake. If there shall be a function I would prefer a name which catures the specific operation "add" or "merge" instead of the generic "op", e.g. z_vcpkg_add_to_system_libs in analogy to VCPKG_PREFER_SYSTEM_LIBS.

I would prefer to switch to this; I don't like how this is currently working. Doing this weird INSERT bodging feels bad.

set("${out_var}" "${paths};${lst}" PARENT_SCOPE) doesn't work correctly for empty lists.
And list(INSERT ...) is unable to append.
😕

Doing this weird INSERT bodging feels bad.

@strega-nil INSERT is also used by VTK to insert into the beginning of CMAKE_MODULE_PATH. It is the correct way to PREPEND.

And list(INSERT ...) is unable to append.

@dg0yt: have you tried using -1 as an index? Otherwise that is the reason why list(APPEND) existed from the beginning while PREPEND was added much later (since INSERT 0 just worked for this instead)

To add: From the 3.0 docs:

If is -1 or lesser, it is indexed from the end of the list, with -1 representing the last list element.

have you tried using -1 as an index? Otherwise that is the reason why list(APPEND) existed from the beginning while PREPEND was added much later (since INSERT 0 just worked for this instead)

To add: From the 3.0 docs:

If is -1 or lesser, it is indexed from the end of the list, with -1 representing the last list element.

Yes, I tried in a separate cmake script. INSERT inserts before the given list element, and INSERT -1 inserts before the last list element.
CMake lists aren't C++ containers...

You can do:

if(VCPKG_PREFER_SYSTEM_LIBS) list(APPEND lst "${paths}") else() list(INSERT lst 0 "${paths}") endif() set("${out_var}" "${lst}" PARENT_SCOPE)

You can do ...

That's what I already did.

oh, sorry, I clearly didn't reread the PR.

scripts/buildsystems/vcpkg.cmake

vicroms · 2021-07-23T17:53:54Z

Will be bundled in next roll-up to trigger world rebuild

strega-nil-ms · 2021-07-26T20:46:18Z

Closed for rollup

[vcpkg] Fix toolchain compatibility with cmake < 3.15

@strega-nil

* [rollup:2021-07-26 1/6] PR #18783 (@strega-nil) [scripts-audit] vcpkg_copy_tools and friends * [rollup:2021-07-26 2/6] PR #18898 (@dg0yt) [vcpkg] Fix toolchain compatibility with cmake < 3.15 * [rollup:2021-07-26 3/6] PR #18980 (@strega-nil) [cmake-guidelines] Minor update, for `if()` * [rollup:2021-07-26 4/6] PR #18981 (@strega-nil) [scripts-audit] vcpkg_check_linkage * [rollup:2021-07-26 5/6] PR #19158 (@Hoikas) [vcpkg.cmake] Fix variable case. * [rollup:2021-07-26 6/6] PR #18839 [scripts-audit] z_vcpkg_get_cmake_vars Co-authored-by: nicole mazzuca <[email protected]>

Fix toolchain compatibility with cmake < 3.15

75ca248

dg0yt marked this pull request as draft July 10, 2021 11:55

dg0yt marked this pull request as ready for review July 10, 2021 11:56

autoantwort approved these changes Jul 11, 2021

View reviewed changes

strega-nil reviewed Jul 11, 2021

View reviewed changes

dg0yt mentioned this pull request Jul 11, 2021

[zlib] Add cmake wrapper #18914

Merged

JonLiu1993 assigned PhoebeHui Jul 12, 2021

PhoebeHui changed the title ~~Fix toolchain compatibility with cmake < 3.15~~ [vcpkg] Fix toolchain compatibility with cmake < 3.15 Jul 12, 2021

PhoebeHui added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Jul 12, 2021

dg0yt added 2 commits July 13, 2021 08:30

Add function z_vcpkg_add_vcpkg_to_cmake_path

16f02ef

Run test port

4426ef5

dg0yt mentioned this pull request Jul 15, 2021

[rollup:2021-07-06] Rollup PR #18838

Merged

strega-nil-ms approved these changes Jul 20, 2021

View reviewed changes

scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved

scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved

scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved

Apply suggestions from code review

59a7839

dg0yt mentioned this pull request Jul 21, 2021

[vcpkg-ci] Add test port for cmake user projects #19034

Merged

vicroms approved these changes Jul 23, 2021

View reviewed changes

vicroms added info:next-rollup and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Jul 23, 2021

strega-nil-ms mentioned this pull request Jul 26, 2021

[rollup] 2021-07-26 #19157

Merged

strega-nil-ms closed this Jul 26, 2021

strega-nil-ms pushed a commit to strega-nil/vcpkg that referenced this pull request Jul 28, 2021

[rollup:2021-07-26 2/6] PR microsoft#18898 (@dg0yt)

8a44dc0

[vcpkg] Fix toolchain compatibility with cmake < 3.15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vcpkg] Fix toolchain compatibility with cmake < 3.15 #18898

[vcpkg] Fix toolchain compatibility with cmake < 3.15 #18898

dg0yt commented Jul 10, 2021

dg0yt commented Jul 10, 2021

dg0yt commented Jul 10, 2021

cenit commented Jul 10, 2021

autoantwort commented Jul 10, 2021

dg0yt commented Jul 10, 2021

autoantwort commented Jul 10, 2021

strega-nil Jul 11, 2021 •

edited

Loading

dg0yt Jul 11, 2021

strega-nil Jul 12, 2021

dg0yt Jul 13, 2021

Neumann-A Jul 13, 2021 •

edited

Loading

dg0yt Jul 13, 2021

strega-nil Jul 13, 2021

dg0yt Jul 13, 2021

strega-nil-ms Jul 20, 2021

vicroms commented Jul 23, 2021

strega-nil-ms commented Jul 26, 2021

[vcpkg] Fix toolchain compatibility with cmake < 3.15 #18898

[vcpkg] Fix toolchain compatibility with cmake < 3.15 #18898

Conversation

dg0yt commented Jul 10, 2021

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

dg0yt commented Jul 10, 2021

dg0yt commented Jul 10, 2021

cenit commented Jul 10, 2021

autoantwort commented Jul 10, 2021

dg0yt commented Jul 10, 2021

autoantwort commented Jul 10, 2021

strega-nil Jul 11, 2021 • edited Loading

Choose a reason for hiding this comment

dg0yt Jul 11, 2021

Choose a reason for hiding this comment

strega-nil Jul 12, 2021

Choose a reason for hiding this comment

dg0yt Jul 13, 2021

Choose a reason for hiding this comment

Neumann-A Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

dg0yt Jul 13, 2021

Choose a reason for hiding this comment

strega-nil Jul 13, 2021

Choose a reason for hiding this comment

dg0yt Jul 13, 2021

Choose a reason for hiding this comment

strega-nil-ms Jul 20, 2021

Choose a reason for hiding this comment

vicroms commented Jul 23, 2021

strega-nil-ms commented Jul 26, 2021

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?

strega-nil Jul 11, 2021 •

edited

Loading

Neumann-A Jul 13, 2021 •

edited

Loading