Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vcpkg.cmake] Fix CMP0126 warning when running in trace mode (#23784) #29969

Merged
merged 1 commit into from
May 10, 2023

Conversation

jobor
Copy link
Contributor

@jobor jobor commented Mar 2, 2023

When running CMake 3.21 or newer in trace mode, vcpkg.cmake would trigger the CMP0126 warning. Explicitly set this policy to OLD behavior within the vcpkg.cmake policy scope to silence the warning without changing behaviour.

Fixes #23784

@jobor
Copy link
Contributor Author

jobor commented Mar 2, 2023

After reviewing the warnings I come to the conclusion that it would be safe to set the policy to NEW within the vcpkg.cmake policy scope. Opinions?

@Neumann-A
Copy link
Contributor

Unlike many policies, CMake version 3.26.0-rc5 does not warn when the policy is not set and simply uses OLD behavior. See documentation of the CMAKE_POLICY_WARNING_CMP0126 variable to control the warning.

does not warn when the policy is not set

-> CMake bug ? or did you change CMAKE_POLICY_WARNING_CMPNNNN ?

@dg0yt
Copy link
Contributor

dg0yt commented Mar 2, 2023

After reviewing the warnings I come to the conclusion that it would be safe to set the policy to NEW within the vcpkg.cmake policy scope.

vcpkg.cmake is also used by user projects. So it is not really limited to vcpkg ports. We should be careful before changing it when Kitware decided that it needs a policy.

does not warn when the policy is not set

-> CMake bug ?

IIUC the behaviours is different with trace modes.

@jobor
Copy link
Contributor Author

jobor commented Mar 2, 2023

-> CMake bug ? or did you change CMAKE_POLICY_WARNING_CMPNNNN ?

In trace mode, CMake enables all optional warnings. This is not a bug.

bool cmMakefile::PolicyOptionalWarningEnabled(std::string const& var) const
{
  // Check for an explicit CMAKE_POLICY_WARNING_CMP<NNNN> setting.
  if (cmValue val = this->GetDefinition(var)) {
    return cmIsOn(val);
  }
  // Enable optional policy warnings with --debug-output, --trace,
  // or --trace-expand.
  cmake* cm = this->GetCMakeInstance();
  return cm->GetDebugOutput() || cm->GetTrace();
}

vcpkg.cmake is also used by user projects. So it is not really limited to vcpkg ports. We should be careful before changing it when Kitware decided that it needs a policy.

vcpkg.cmake uses cmake_policy(PUSH/POP), and within these calls we can set policies without affecting user projects.

@Neumann-A
Copy link
Contributor

After reviewing the warnings I come to the conclusion that it would be safe to set the policy to NEW within the vcpkg.cmake policy scope. Opinions?

I don't mind setting it to new.

@@ -38,6 +38,10 @@ if(CMAKE_VERSION VERSION_LESS Z_VCPKG_CMAKE_REQUIRED_MINIMUM_VERSION)
endif()
cmake_policy(PUSH)
cmake_policy(VERSION 3.7.2)
if(CMAKE_VERSION VERSION_GREATER_EQUAL "3.21.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(CMAKE_VERSION VERSION_GREATER_EQUAL "3.21.0")
if(POLICY CMP0126)

@Neumann-A
Copy link
Contributor

In trace mode, CMake enables all optional warnings. This is not a bug.

But the code also says you could actively suppress the warning if I read it correctly. (i.e. by explicitly setting it to false)

@FrankXie05 FrankXie05 added the category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) label Mar 3, 2023
@dg0yt
Copy link
Contributor

dg0yt commented Mar 3, 2023

Trying to get the full picture.

  • The code run at top-level scope.
  • It is scoped by cmake_policy(PUSH/POP).
    However, there are two ways how this policy setting enters the scope of the user projects:

I'm not really against this change. But I want all options to be considered:

  • Do nothing. The user can adjust policies and warnings if desired. Standard behaviour.
  • Force-disable the warning. Needs care to not override an explicit user setting. Mild non-standard behaviour.
  • Force-enable the policy. This PR. Recommended for new CMake code, but would also apply on old find modules and package configs.

Last not least it might make sense to actually check the current warnings. In #23784, I see VCPKG_MANIFEST_DIR. This is a variable under vcpkg's control. Can we change our scripts to avoid the warning?

@jobor
Copy link
Contributor Author

jobor commented Mar 3, 2023

Avoiding the warning without modifying the policies is certainly an option. The pattern where the warning is triggered is:

if(NOT VCPKG_SOME_VAR)
    set(VCPKG_SOME_VAR some default)
endif()
set(VCPKG_SOME_VAR ${VCPKG_SOME_VAR} CACHE PATH "docstring" FORCE)

The minimal impact fix is

if(NOT VCPKG_SOME_VAR)
    set(VCPKG_SOME_VAR some default CACHE PATH "docstring")
endif()
set(VCPKG_SOME_VAR ${VCPKG_SOME_VAR} CACHE PATH "docstring" FORCE)

Or it might be more elegant to use a differently named variable for holding the default value.

If I remember correctly, only two VCPKG_* variables are affected.
I'll play with this approach.

@jobor jobor marked this pull request as draft March 3, 2023 07:48
When running CMake 3.21 or newer in trace or debug mode, vcpkg.cmake
would trigger the CMP0126 warning for VCPKG_MANIFEST_DIR and
VCPKG_INSTALLED_DIR.

The regular variable was used to set the initial value of the cache
variable of the same name. This patch adds the regular variables
Z_VCPKG_MANIFEST_DIR_INITIAL_VALUE and
Z_VCPKG_INSTALLED_DIR_INITIAL_VALUE that are used to initialize their
respective cache variables.
@jobor jobor marked this pull request as ready for review March 3, 2023 08:24
@Cheney-W Cheney-W added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly and removed category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) labels Mar 3, 2023
@FrankXie05
Copy link
Contributor

It's better to adapt to warnings than to avoid them, and I think this one is ok.
We can check whether this warning is triggered elsewhere, and we can refer to the modification. :)

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label May 9, 2023
@vicroms vicroms merged commit 512d62f into microsoft:master May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
6 participants