Skip to content

[vcpkg-cmake] Catch wrong MAYBE_UNUSED_VARIABLES#38249

Merged
data-queue merged 19 commits intomicrosoft:masterfrom
dg0yt:vcpkg-cmake-options
Apr 30, 2024
Merged

[vcpkg-cmake] Catch wrong MAYBE_UNUSED_VARIABLES#38249
data-queue merged 19 commits intomicrosoft:masterfrom
dg0yt:vcpkg-cmake-options

Conversation

@dg0yt
Copy link
Copy Markdown
Contributor

@dg0yt dg0yt commented Apr 18, 2024 •
edited
Loading

#36124 (comment),
#24633 (comment).
Related (fix): #38223 (merged here.)

jimwang118 and others added 15 commits April 3, 2024 08:31
@dg0yt dg0yt mentioned this pull request Apr 18, 2024
Merged
@FrankXie05 FrankXie05 added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Apr 18, 2024
@dg0yt
Copy link
Copy Markdown
Contributor Author

dg0yt commented Apr 19, 2024

Works as desired:

CMake Error at /mnt/vcpkg-ci/installed/x64-linux/share/vcpkg-cmake/vcpkg_cmake_configure.cmake:40 (message):
  Option MAYBE_UNUSED_VARIABLES must be used with variables names.  The
  following items are invalid: -DDCMTK_USE_FIND_PACKAGE_WIN_DEFAULT=ON
Call Stack (most recent call first):
  ports/dcmtk/portfile.cmake:40 (vcpkg_cmake_configure)
  scripts/ports.cmake:175 (include)


error: building dcmtk:x64-linux failed with: BUILD_FAILED
...
CMake Error at /mnt/vcpkg-ci/installed/x64-linux/share/vcpkg-cmake/vcpkg_cmake_configure.cmake:40 (message):
  Option MAYBE_UNUSED_VARIABLES must be used with variables names.  The
  following items are invalid: -DCMAKE_DISABLE_FIND_PACKAGE_GLEW=ON
  -DCMAKE_DISABLE_FIND_PACKAGE_glfw3=ON
  -DCMAKE_DISABLE_FIND_PACKAGE_LibXml2=ON
  -DCMAKE_DISABLE_FIND_PACKAGE_OCaml=ON
  -DCMAKE_DISABLE_FIND_PACKAGE_OpenGL=ON -DCMAKE_DISABLE_FIND_PACKAGE_Qt5=ON
  -DCMAKE_DISABLE_FIND_PACKAGE_Subversion=ON
Call Stack (most recent call first):
  ports/mdl-sdk/portfile.cmake:119 (vcpkg_cmake_configure)
  scripts/ports.cmake:175 (include)


error: building mdl-sdk:x64-linux failed with: BUILD_FAILED

@dg0yt dg0yt marked this pull request as ready for review April 19, 2024 04:08
FrankXie05
FrankXie05 previously approved these changes Apr 23, 2024
View reviewed changes
@FrankXie05 FrankXie05 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Apr 23, 2024
@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Apr 23, 2024
ras0219-msft
ras0219-msft reviewed Apr 23, 2024
View reviewed changes
ports/vcpkg-cmake/vcpkg.json Outdated
"version-date": "2023-05-04",
"documentation": "https://vcpkg.io/en/docs/maintainers/ports/vcpkg-cmake.html",
"version-date": "2024-04-18",
"documentation": "https://learn.microsoft.com/vcpkg/maintainers/authoring-script-ports",
Copy link
Copy Markdown
Contributor

@ras0219-msft ras0219-msft Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"documentation": "https://learn.microsoft.com/vcpkg/maintainers/authoring-script-ports",
"documentation": "https://learn.microsoft.com/vcpkg/maintainers/functions/vcpkg_cmake_install",

authoring-script-ports covers generically authoring any script port. While it would be good to have a dedicated landing page for the vcpkg-cmake helper all-up, I think for now we should link to the "primary" helper function's documentation.

Copy link
Copy Markdown
Contributor Author

@dg0yt dg0yt Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the primary helper function is vcpkg_cmake_configure which does all the heavy lifting and receives all the improvements. vpkg_cmake_install isn't much more than a thin wrapper for running ninja.

Copy link
Copy Markdown
Contributor

@ras0219-msft ras0219-msft Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, I'd be happy to see a link to https://learn.microsoft.com/vcpkg/maintainers/functions/vcpkg_cmake_configure.

@JavierMatosD JavierMatosD mentioned this pull request Apr 24, 2024
Closed
7 tasks
@dg0yt dg0yt mentioned this pull request Apr 25, 2024
Merged
@FrankXie05 FrankXie05 removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Apr 26, 2024
@data-queue data-queue merged commit 2eab0f6 into microsoft:master Apr 30, 2024
@dg0yt dg0yt deleted the vcpkg-cmake-options branch April 30, 2024 19:05
yurybura pushed a commit to yurybura/vcpkg that referenced this pull request May 8, 2024
@dg0yt @jimwang118 @yurybura
[vcpkg-cmake] Catch wrong MAYBE_UNUSED_VARIABLES (microsoft#38249)
69cfa96 
microsoft#36124 (comment),  
microsoft#24633 (comment).
Related (fix): microsoft#38223 (merged
here.)

---------

Co-authored-by: Jim-Wang <wangzhijie05@beyondsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@ras0219-msft ras0219-msft ras0219-msft left review comments

@JavierMatosD JavierMatosD JavierMatosD approved these changes

@data-queue data-queue data-queue approved these changes

@FrankXie05 FrankXie05 FrankXie05 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@FrankXie05 FrankXie05

Labels

category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dg0yt @ras0219-msft @JavierMatosD @FrankXie05 @data-queue @jimwang118