Skip to content

Commit

Permalink
Resolve 14506, avoid libidn2 for our curl dependency (#14601)
Browse files Browse the repository at this point in the history
- Reviews the proposal of @jay to resolve libidn2 feature election curl/curl#6362
- Uses -U2 for the patch, to ensure placement but not useless collisions
- Moves extended text of the problems addressed to the patch

This additional patch to dodge idn2 is draft, but hopefully resembles the final patch
to let us pass a simple toggle to avoid libidn2 for machines deployed without
libidn2 libs. In any case, any dynamic additional ld requirement goes against the
static build model, and libidn2 would have to be added to the envoy build.
As we expect to deprecate curl, this would be movement in the wrong direction.

Risk Level: low
Testing: local
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: Linux ldd bindings
Fixes: #14506

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
  • Loading branch information
wrowe authored Jan 8, 2021
1 parent 846f27c commit 4302eb1
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 10 deletions.
2 changes: 2 additions & 0 deletions bazel/foreign_cc/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ envoy_cmake_external(
"CURL_CA_PATH": "none",
"CMAKE_USE_OPENSSL": "off",
"OPENSSL_ROOT_DIR": "$EXT_BUILD_DEPS",
# Avoid libidn2
"USE_LIBIDN2": "off",
# NGHTTP2.
"USE_NGHTTP2": "on",
"NGHTTP2_LIBRARY": "$EXT_BUILD_DEPS/nghttp2",
Expand Down
68 changes: 64 additions & 4 deletions bazel/foreign_cc/curl.patch
Original file line number Diff line number Diff line change
@@ -1,14 +1,61 @@
#commit 743021d6c7abba91c47e5be8035ff0497f2b78bd
#Author: Jay Satiro <raysatiro@yahoo.com>
#Date: Tue Dec 22 15:31:03 2020 -0500
#
# cmake: Add an option to disable libidn2
#
# New option USE_LIBIDN2 defaults to ON for libidn2 detection. Prior to
# this change libidn2 detection could not be turned off in cmake builds.
#
# Reported-by: William A Rowe Jr
#
# Fixes https://github.com/curl/curl/issues/6361
# Closes #xxxx
#
#commit e952764adbb89f37dbf227a48a55cc57c60b537d
#Author: William A Rowe Jr <wrowe@vmware.com>
#Date: Wed Oct 7 14:32:49 2020 -0500
#
# Correct fragile windows assumptions
#
# - 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;
# https://github.com/bazelbuild/rules_foreign_cc/issues/426
diff --git a/CMakeLists.txt b/CMakeLists.txt
index ec1cfa782..0c5a72f00 100644
index 6a1a6fe8e..777ee122f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -42,0 +42,5 @@
+# revert CMake bug triggered by curl's defined max CMake policy version, see https://gitlab.kitware.com/cmake/cmake/-/issues/21288
@@ -40,4 +40,9 @@
cmake_minimum_required(VERSION 3.2...3.16 FATAL_ERROR)

+# Revert CMake bug triggered by curl's defined max CMake policy version, see https://gitlab.kitware.com/cmake/cmake/-/issues/21288
+if(POLICY CMP0091)
+ cmake_policy(SET CMP0091 OLD)
+endif()
+
@@ -249,3 +254,6 @@
set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/CMake;${CMAKE_MODULE_PATH}")
include(Utilities)
@@ -248,7 +253,10 @@ endif()

if(CURL_STATIC_CRT)
- set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
- set(CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE} /MT")
- set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} /MTd")
Expand All @@ -18,3 +65,16 @@ index ec1cfa782..0c5a72f00 100644
+ string(REGEX REPLACE "/MD" "/MT" ${flags_var} "${${flags_var}}")
+ endif()
+ endforeach()
endif()

@@ -619,5 +627,9 @@ endif()

# Check for idn
-check_library_exists_concat("idn2" idn2_lookup_ul HAVE_LIBIDN2)
+option(USE_LIBIDN2 "Use libidn2 for IDN support" ON)
+set(HAVE_LIBIDN2 OFF)
+if(USE_LIBIDN2)
+ check_library_exists_concat("idn2" idn2_lookup_ul HAVE_LIBIDN2)
+endif()

# Check for symbol dlopen (same as HAVE_LIBDL)
10 changes: 4 additions & 6 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -633,12 +633,10 @@ def _com_github_curl():
build_file_content = BUILD_ALL_CONTENT + """
cc_library(name = "curl", visibility = ["//visibility:public"], deps = ["@envoy//bazel/foreign_cc:curl"])
""",
# Patch curl 7.72.0 due to CMake's problematic implementation of policy `CMP0091`
# introduced in CMake 3.15 and then deprecated in CMake 3.18. Curl forcing the CMake
# ruleset to 3.16 breaks the Envoy windows fastbuild target.
# Also cure a fatal assumption creating a static library using LLVM `lld-link.exe`
# adding dynamic link flags, which breaks the Envoy clang-cl library archive step.
# Upstream patch submitted: https://github.com/curl/curl/pull/6050
# Patch curl 7.74.0 due to CMake's problematic implementation of policy `CMP0091`
# and introduction of libidn2 dependency which is inconsistently available and must
# not be a dynamic dependency on linux.
# Upstream patches submitted: https://github.com/curl/curl/pull/6050 & 6362
# TODO(https://github.com/envoyproxy/envoy/issues/11816): This patch is obsoleted
# by elimination of the curl dependency.
patches = ["@envoy//bazel/foreign_cc:curl.patch"],
Expand Down

0 comments on commit 4302eb1

Please sign in to comment.