[scripts-audit] add guidelines for cmake by strega-nil · Pull Request #18319 · microsoft/vcpkg

strega-nil · 2021-06-07T19:50:31Z

No description provided.

docs/maintainers/cmake-guidelines.md

dg0yt · 2021-06-07T21:22:42Z

Any hint about the min version of CMake scripts must be compatible with / the version scripts may assume without checks?

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

scripts/buildsystems/vcpkg.cmake

Co-authored-by: ras0219 <533828+ras0219@users.noreply.github.com>

dg0yt · 2021-06-08T05:05:28Z

How about not allowing

set(VAR )

because it is a common trap (in particular with initialization of lists) that the real intention is

set(VAR "")

but the actual effect is

unset(VAR)

which unhides cache variables. So there are two explicit alternatives which are not ugly.

I would also encourage use of

string(APPEND VAR " xxx")

over

set(VAR "${VAR} xxx")

because it makes accidental

set(VAR "${VAR};xxx")

stand out more clearly.

Neumann-A · 2021-06-08T05:42:31Z

set(VAR) is used to create an empty variable in the local/current scoped thus hiding any higher scoped variables. Setting it to an empty string instead is rarely meant and doesn't play nice with list(APPEND)

dg0yt · 2021-06-08T06:01:07Z

set(VAR) is used to create an empty variable in the local/current scoped thus hiding any higher scoped variables. Setting it to an empty string instead is rarely meant and doesn't play nice with list(APPEND)

This is what I thought until recently. But it is wrong, and that's why I'm bringing it up:

$ cat demo.cmake
set(FOO "Initial")
set(FOO "Cached value" CACHE STRING "doc")
message(STATUS "${FOO}")
set(FOO )
message(STATUS "${FOO}")
list(APPEND FOO "bar")
message(STATUS "${FOO}")
set(FOO "")
list(APPEND FOO "bar")
message(STATUS "${FOO}")
$ cmake -P demo.cmake 
-- Cached value
-- Cached value
-- Cached value;bar
-- bar

dg0yt · 2021-06-08T06:33:06Z

After adding one more message(STATUS "${FOO}"), it turns out that (at least) in script mode, the non-cache variable (aka "set" binding) doesn't even hide the cached variable (aka cache entry). This is in contradiction to cmake documentation. Oh no.

Neumann-A · 2021-06-08T06:45:57Z

your example should use at least one scope change
cache variables in script mode are kund of funky since there is no concept of cache in script mode(no CMakeCache.txt to store them). So all Cache variables become priority read access.

dg0yt · 2021-06-08T07:33:49Z

your example should use at least one scope change

The result doesn't change when I wrap the code in function(...) ... endfunction().

cache variables in script mode are kund of funky since there is no concept of cache in script mode(no CMakeCache.txt to store them). So all Cache variables become priority read access.

I'm aware that there is no persistent cache in script mode but there are still the different types of variables.
Variable evaluation is documented cmake behaviour, regardless of script mode.
The documented behaviour of set(VAR ) is unsetting (actually creating an "unset" binding), regardless of scope.

I opened https://gitlab.kitware.com/cmake/cmake/-/issues/22285.

dg0yt · 2021-06-08T11:57:34Z

I opened https://gitlab.kitware.com/cmake/cmake/-/issues/22285.

A fix is already merged for CMake 3.21, and there will be a new policy, CMP0126.

My original point stands: set(VAR ) does NOT create an empty variable, and so it doesn't hide anything.

docs/maintainers/cmake-guidelines.md

thanks robert! Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

PhoebeHui · 2021-06-11T06:16:26Z

REGRESSION: opencv4:arm-uwp: There is not enough space on the disk.
REGRESSION: qt5-sensors:x64-windows-static-md: Source download issue
REGRESSION: qt5-scxml:x64-windows-static-md: Source download issue
REGRESSION: otl:x64-linux: The hash changed, need update it. Fixed by PR [vcpkg baseline][otl] Update to 40463 #18383

strega-nil-ms · 2021-06-11T22:11:47Z

/azp run

azure-pipelines · 2021-06-11T22:11:56Z

Azure Pipelines successfully started running 1 pipeline(s).

strega-nil-ms · 2021-06-14T17:32:39Z

/azp run

azure-pipelines · 2021-06-14T17:32:48Z

Azure Pipelines successfully started running 1 pipeline(s).

Neumann-A · 2021-06-15T07:50:28Z

what about guidelines for vcpkg-cmake-wrapper.cmake?
#18393 (comment) shows that it seems to be required

dg0yt · 2021-06-15T08:02:46Z

what about guidelines for vcpkg-cmake-wrapper.cmake?

Fair, but the dominant problem is that documentation is completely missing vcpkg-cmake-wrapper.cmake and vcpkg-cmake-config.cmake.

docs/maintainers/cmake-guidelines.md

dg0yt · 2021-06-16T12:18:58Z

docs/maintainers/cmake-guidelines.md

+  - 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


For vcpkg-cmake-wrapper.cmake scripts, version 3.1 even excludes if("value" IN_LIST FEATURES). This makes it more complicated to deal with port features or even with arguments to find_package (e.g. port tiff).

vcpkg.cmake must assume a version of CMake back to 3.1 in general

Is sentence is nonsense and should be ignored. I think the correct phrasing would be that vcpkg.cmake must assume that the toplevel CMakeLists.txt might not have a cmake_minimum_required or a very low cmake_minimum_required. As such, setting appropriate cmake policies are a requirement. vcpkg assume that it is at least executed with a cmake binary of <some cmake version|preferly the cmake version used by vcpkg>.
If there is somebody really arguing for a cmake binary of 3.1 I would like to have him/her invent vcpkg.old.cmake .....

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.)

What is the reason to keep it at 3.1?
What is the threshold for moving it to another version?
As I already wrote:

For vcpkg-cmake-wrapper.cmake scripts, version 3.1 even excludes if("value" IN_LIST FEATURES).

which is relevant for dealing with transitive dependencies.

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.

strega-nil-ms · 2021-06-17T17:31:41Z

@dg0yt @Neumann-A that would be good guidelines, but these are meant more for the scripts under the scripts tree.

docs/maintainers/cmake-guidelines.md

ras0219-msft · 2021-07-04T10:59:26Z

docs/maintainers/cmake-guidelines.md

+- `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.


Suggested change

- This must be checked.

- This must be checked by <insert copypaste code snippet here>.

PhoebeHui · 2021-07-05T06:19:09Z

Tensorflow:x64-osx failure would be fixed by PR #18812

strega-nil-ms · 2021-07-06T17:02:50Z

Closed for rollup PR #18838

[scripts-audit] add guidelines for cmake

@strega-nil

* [rollup:2021-07-06 1/8] PR #18272 (@strega-nil) [scripts-audit] vcpkg_from_* * [rollup:2021-07-06 2/8] PR #18319 (@strega-nil) [scripts-audit] add guidelines for cmake * [rollup 2021-07-06 3/8] PR #18410 (@mheyman) [vcpkg-cmake-config] documentation fix * [rollup:2021-07-06 4/8] PR #18488 (@strega-nil) [scripts-audit] vcpkg_execute_* * [rollup:2021-07-06 5/8] PR #18517 (@strega-nil) [scripts-audit] vcpkg_extract_source_archive * [rollup:2021-07-06 6/8] PR #18674 (@NancyLi1013) [vcpkg doc] Update examples * [rollup:2021-07-06 7/8] PR #18695 (@JackBoosY) [vcpkg] Update the minimum version of vcpkg * [rollup:2021-07-06 8/8] PR #18758 (@ras0219-msft) [vcpkg_from_git] Fix error if downloads folder does not exist * build docs! * fix bond:*-windows * fix nmap Co-authored-by: nicole mazzuca <mazzucan@outlook.com> Co-authored-by: Michael Heyman <Michael.Heyman@jhuapl.edu> Co-authored-by: NancyLi1013 <lirui09@beyondsoft.com> Co-authored-by: JackBoosY <yuzaiyang@beyondsoft.com> Co-authored-by: Robert Schumacher <ras0219@outlook.com>

[scripts-audit] add guidelines for cmake

e21bdba

Neumann-A reviewed Jun 7, 2021

View reviewed changes

docs/maintainers/cmake-guidelines.md Outdated Show resolved Hide resolved

Neumann-A reviewed Jun 7, 2021

View reviewed changes

docs/maintainers/cmake-guidelines.md Show resolved Hide resolved

Neumann-A reviewed Jun 7, 2021

View reviewed changes

docs/maintainers/cmake-guidelines.md Outdated Show resolved Hide resolved

Neumann-A reviewed Jun 7, 2021

View reviewed changes

docs/maintainers/cmake-guidelines.md Show resolved Hide resolved

strega-nil added 2 commits June 7, 2021 14:00

cr changes

0f35779

Neumann-A CRs

731ffec

add guidelines on minimum CMake version

e3bd0c5

get_cmake_vars requires version 3.17, thus all scripts should require 3.17 update cmake_minimum_required in scripts/ports.cmake

Neumann-A reviewed Jun 7, 2021

View reviewed changes

docs/maintainers/cmake-guidelines.md Show resolved Hide resolved

require 3.20

e9c311d

ras0219 reviewed Jun 7, 2021

View reviewed changes

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

strega-nil-ms added info:internal labels Jun 7, 2021

Update scripts/buildsystems/vcpkg.cmake

d6759b0

Co-authored-by: ras0219 <533828+ras0219@users.noreply.github.com>

JonLiu1993 assigned PhoebeHui Jun 8, 2021

suggestion from dg0yt

8a9820b

dg0yt reviewed Jun 9, 2021

View reviewed changes

docs/maintainers/cmake-guidelines.md Outdated Show resolved Hide resolved

okay dg0yt is right

8e46aef

ras0219-msft approved these changes Jun 9, 2021

View reviewed changes

Apply suggestions from code review

1aadd2c

thanks robert! Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

PhoebeHui mentioned this pull request Jun 11, 2021

[vcpkg baseline][otl] Update to 40463 #18383

Merged

PhoebeHui added the category:scripts-audit label Jun 15, 2021

dg0yt reviewed Jun 16, 2021

View reviewed changes

BillyONeal approved these changes Jun 28, 2021

View reviewed changes

ras0219-msft approved these changes Jul 1, 2021

View reviewed changes

Merge from master

bed284c

PhoebeHui added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jul 2, 2021

update guidelines slightly

957c851

strega-nil-ms force-pushed the scripts-guidelines branch from eb2215f to 957c851 Compare July 2, 2021 16:05

strega-nil requested a review from ras0219-msft July 2, 2021 22:04

ras0219-msft reviewed Jul 4, 2021

View reviewed changes

Robert CRs

35807ea

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

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

Merged

strega-nil-ms closed this Jul 6, 2021

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

[rollup:2021-07-06 2/8] PR microsoft#18319 (@strega-nil)

f9a2aa7

[scripts-audit] add guidelines for cmake

strega-nil deleted the scripts-guidelines branch July 16, 2021 21:58

	- This must be checked.
	- This must be checked by <insert copypaste code snippet here>.

Conversation

strega-nil commented Jun 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dg0yt commented Jun 7, 2021

Uh oh!

Uh oh!

Uh oh!

dg0yt commented Jun 8, 2021

Uh oh!

Neumann-A commented Jun 8, 2021

Uh oh!

dg0yt commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dg0yt commented Jun 8, 2021

Uh oh!

Neumann-A commented Jun 8, 2021

Uh oh!

dg0yt commented Jun 8, 2021

Uh oh!

dg0yt commented Jun 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PhoebeHui commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strega-nil-ms commented Jun 11, 2021

Uh oh!

azure-pipelines bot commented Jun 11, 2021

Uh oh!

strega-nil-ms commented Jun 14, 2021

Uh oh!

azure-pipelines bot commented Jun 14, 2021

Uh oh!

Neumann-A commented Jun 15, 2021

Uh oh!

dg0yt commented Jun 15, 2021

Uh oh!

Uh oh!

dg0yt Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

Neumann-A Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

strega-nil-ms Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

dg0yt Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

strega-nil-ms Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strega-nil-ms commented Jun 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ras0219-msft Jul 4, 2021

Choose a reason for hiding this comment

Uh oh!

PhoebeHui commented Jul 5, 2021

Uh oh!

strega-nil-ms commented Jul 6, 2021

Uh oh!

Reviewers

dg0yt commented Jun 8, 2021 •

edited

Loading

PhoebeHui commented Jun 11, 2021 •

edited

Loading

strega-nil-ms Jun 29, 2021 •

edited

Loading