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

Windows flaw when building with bazel in modern CMake due to erroneous policy CMP0091 #426

Closed
wrowe opened this issue Sep 15, 2020 · 1 comment

Comments

@wrowe
Copy link

wrowe commented Sep 15, 2020

At the envoy project, we have been very successful ensuring that Bazel's elections of compilation and linker flags are respected by using the -GNinja target with CMAKE_BUILD_TYPE "Bazel" for cmake_external() components.

This causes all of the various CMAKE_C_FLAGS_RELEASE, CMAKE_C_FLAGS_DEBUG choices etc. to be silently dropped. (If we wish, bazel's cmake_external() could pass CMAKE_C_FLAGS_BAZEL, but this is unnecessary since they are just passed as CMAKE_C_FLAGS.) Each bazel invocation of rules_foreign_cc invokes cmake to configure the appropriate CMAKE_C_FLAGS and other command line options.

In the curl project, we encountered the following problem with their explicit cmake rulesets;
cmake_minimum_required(VERSION 3.2...3.16 FATAL_ERROR)

This triggers a new CMake 3.15 rule CMP0091, which introduces a bug on Windows. That rule causes CMAKE_MSVC_RUNTIME_LIBRARY to be set to a fixed runtime, overriding the BAZEL target elections. If this policy is unset, the original behavior is restored, and the runtime elections are left in CMAKE_C_FLAGS_RELEASE etc, where they are harmless.

It turns out that toggling the CURL_STATIC_CRT works around this policy flaw in this specific case, but only by coincidence. We should avoid spelling out static/dynamic runtime elections to each CMake-based component of a given project, and always control the runtime election through bazel's own logic using toggles such as --features=fully_static_link --features=static_link_msvcrt and --dynamic_mode=off.

We would suggest that cmake_external() should inject the following line into crosstool_bazel.cmake within CMake_script.sh, which would suppress this flaw, irrespective of which version of CMake is invoked;

cmake_policy(SET CMP0091 OLD)
htuch pushed a commit to envoyproxy/envoy that referenced this issue Sep 16, 2020
Picks up fix of CVE-2020-8231: libcurl: wrong connect-only connection
(Envoy does NOT use the affected CURLOPT_CONNECT_ONLY toggle,
this change simply satisfies overly-simplistic audits, and this further
reinforces issue #11816 )

Resolve premature force-AF_UNIX toggle in our working branch and
disables UNIX_SOCKETS on windows, until afunix.h is used for sys/un.h
(Breakage is between curl 7.69 and curl 7.72, which doesn't pick up afunix.h
on windows; will address and resolve upstream before revisiting at envoy.)

Work around misplaced -MD cflag on Windows build
See bazel-contrib/rules_foreign_cc#426
for comprehensive discussion of the defect. This workaround
could be dropped if rules_foreign_cc works around it for us.

Preparing for specific corrections to get curl building curl.lib
with cmake using clang-cl on Windows, starting at envoy and pushing
the fix(es) to CMakeLists.txt upstream.

Risk Level: low
Testing: local
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
@wrowe
Copy link
Author

wrowe commented Oct 6, 2020

According to the final paragraph of documentation on this problematic CMake behavior;

"This policy was introduced in CMake version 3.15. Use the cmake_policy() command to set it to OLD or NEW explicitly. Unlike many policies, CMake version 3.18.4 does not warn when this policy is not set and simply uses OLD behavior." https://cmake.org/cmake/help/v3.18/policy/CMP0091.html

Contrary to the following auto-appended Note, it seems clear this unwise change will permanantly default to 'OLD' (correct) behavior, and the 'NEW' behavior is actually deprecated. I don't see any action the bazel rules_foreign_cc can take to dodge this misapplication of the policy by the curl project, and so this should just be treated as closed/CANT_FIX.

@wrowe wrowe closed this as completed Oct 6, 2020
wrowe added a commit to wrowe/curl that referenced this issue Oct 7, 2020
- Locking CMake to 3.16 breaks all features and corrections applied to
  CMake 3.17 and later, including the correction of the poorly designed
  and now abandoned Windows CRT election policy CMP0091 (see final para
  of the policy description here:
  https://cmake.org/cmake/help/v3.18/policy/CMP0091.html). Locking to
  rev 3.16 from ensures a more difficult transition to CMake-current

- Windows curl builds previously only adjusted the Release and Debug
  builds, and combined with CMP0091 to break other flavors. Update any
  /MD* flags with /MT* present in the base and four alternate build
  flavors, without introducing conflicting flag values or introducing
  a CRT election where one is not present

- Windows clang-cl builds of curl static libs are broken when using
  link-lld.exe because curl appended the dynamic run time flags to the
  static library lib.exe options. While these were ignored/no-op on
  Windows link.exe, they cause link-lld from LLVM/clang-cl compile
  toolchain to fail to parse the library command.

Summary exists in this bazel-specific bug report;
bazel-contrib/rules_foreign_cc#426
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant