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

Add curl 8.8.0 #2233

Closed
wants to merge 3 commits into from
Closed

Add curl 8.8.0 #2233

wants to merge 3 commits into from

Conversation

lalten
Copy link
Contributor

@lalten lalten commented Jun 11, 2024

https://github.com/curl/curl/releases/tag/curl-8_8_0

❯ diff -u modules/curl/8.7.1/patches modules/curl/8.8.0/patches
diff --color -u modules/curl/8.7.1/patches/add_build_file.patch modules/curl/8.8.0/patches/add_build_file.patch
--- modules/curl/8.7.1/patches/add_build_file.patch     2024-06-11 20:41:13.006463267 +0200
+++ modules/curl/8.8.0/patches/add_build_file.patch     2024-06-15 01:07:26.441881282 +0200
@@ -1,13 +1,13 @@
 diff --git a/BUILD.bazel b/BUILD.bazel
 new file mode 100644
-index 000000000..4ec0bacf5
+index 000000000..0eaa6df71
 --- /dev/null
 +++ b/BUILD.bazel
-@@ -0,0 +1,260 @@
+@@ -0,0 +1,301 @@
 +# Copyright The OpenTelemetry Authors
 +# SPDX-License-Identifier: Apache-2.0
 +
-+load("@bazel_skylib//rules:common_settings.bzl", "bool_flag")
++load("@bazel_skylib//rules:common_settings.bzl", "bool_flag", "string_flag")
 +
 +package(features = ["no_copts_tokenization"])
 +
@@ -31,6 +31,35 @@
 +    flag_values = {":http_only": "true"},
 +)
 +
++# Curl's default CA path depends on the OS. If CURL_CA_BUNDLE/CURL_CA_PATH are not set,
++# (and you don't `curl_easy_setopt(curl_, CURLOPT_CAPATH, "/etc/ssl/certs");`), curl
++# will fail to verify server certificates.
++# The string_flag below allows the user to specify their (OS-specific) default CA path
++# like `--@curl//:ca_bundle=/etc/ssl/certs/ca-certificates.crt` on the command line or
++# in .bazelrc. See also https://github.com/curl/curl/blob/curl-8_8_0/acinclude.m4#L1202
++
++string_flag(
++    name = "ca_bundle",
++    build_setting_default = "",
++    make_variable = "CURL_CA_BUNDLE",
++)
++
++config_setting(
++    name = "ca_bundle_is_unset",
++    flag_values = {":ca_bundle": ""},
++)
++
++string_flag(
++    name = "ca_path",
++    build_setting_default = "",
++    make_variable = "CURL_CA_PATH",
++)
++
++config_setting(
++    name = "ca_path_is_unset",
++    flag_values = {":ca_path": ""},
++)
++
 +_BASE_CURL_COPTS = [
 +    "-DCURL_DISABLE_LDAP=1",
 +    "-DENABLE_IPV6=1",
@@ -46,6 +75,7 @@
 +    "-DHAVE_BOOL_T=1",
 +    "-DHAVE_CLOCK_GETTIME_MONOTONIC=1",
 +    "-DHAVE_CONNECT=1",
++    "-DHAVE_DIRENT_H=1",
 +    "-DHAVE_DLFCN_H=1",
 +    "-DHAVE_ENGINE_LOAD_BUILTIN_ENGINES=1",
 +    "-DHAVE_ERRNO_H=1",
@@ -98,6 +128,7 @@
 +    "-DHAVE_NETINET_IN_H=1",
 +    "-DHAVE_NETINET_TCP_H=1",
 +    "-DHAVE_NET_IF_H=1",
++    "-DHAVE_OPENDIR=1",
 +    "-DHAVE_PIPE=1",
 +    "-DHAVE_POLL=1",
 +    "-DHAVE_POLL_FINE=1",
@@ -246,7 +277,17 @@
 +    }) + select({
 +        ":http_only_setting": ["HTTP_ONLY"],
 +        "//conditions:default": [],
++    }) + select({
++        ":ca_bundle_is_unset": [],
++        "//conditions:default": ['CURL_CA_BUNDLE=\\"$(CURL_CA_BUNDLE)\\"'],
++    }) + select({
++        ":ca_path_is_unset": [],
++        "//conditions:default": ['CURL_CA_PATH=\\"$(CURL_CA_PATH)\\"'],
 +    }),
++    toolchains = [
++        ":ca_bundle",
++        ":ca_path",
++    ],
 +    visibility = ["//visibility:public"],
 +    deps = select({
 +        ":use_mbedtls_setting": ["@mbedtls"],
diff --color -u modules/curl/8.7.1/patches/module_dot_bazel.patch modules/curl/8.8.0/patches/module_dot_bazel.patch
--- modules/curl/8.7.1/patches/module_dot_bazel.patch   2024-06-11 20:41:13.006463267 +0200
+++ modules/curl/8.8.0/patches/module_dot_bazel.patch   2024-06-15 01:06:16.856887328 +0200
@@ -1,15 +1,15 @@
 diff --git a/MODULE.bazel b/MODULE.bazel
 new file mode 100644
-index 000000000..0719ffa8a
+index 000000000..24b6c8297
 --- /dev/null
 +++ b/MODULE.bazel
 @@ -0,0 +1,9 @@
 +module(
 +    name = "curl",
-+    version = "8.7.1",
++    version = "8.8.0",
 +    compatibility_level = 0,
 +)
 +
-+bazel_dep(name = "bazel_skylib", version = "1.5.0")
++bazel_dep(name = "bazel_skylib", version = "1.7.1")
 +bazel_dep(name = "mbedtls", version = "3.6.0")
-+bazel_dep(name = "platforms", version = "0.0.8")
++bazel_dep(name = "platforms", version = "0.0.10")

@bazel-io
Copy link
Member

Hello @keith, modules you maintain (curl) have been updated in this PR. Please review the changes.

@Vertexwahn
Copy link
Contributor

Vertexwahn commented Jun 11, 2024

FYI: With Bazel 7.2.0 we have no support for overlays - could instead of patches offer directly BUILD.bazel and MODULE.bazel via overlay

@lalten
Copy link
Contributor Author

lalten commented Jun 12, 2024

FYI: With Bazel 7.2.0 we have no support for overlays - could instead of patches offer directly BUILD.bazel and MODULE.bazel via overlay

looks like that isn't quite ready yet :/ #2240

@lalten lalten force-pushed the curl-8.8.0 branch 2 times, most recently from 7e1ef65 to e4d0f9a Compare June 13, 2024 20:45
+ flag_values = {":http_only": "true"},
+)
+
+# Curl's default CA path depends on the OS. If CURL_CA_BUNDLE/CURL_CA_PATH are not set,
Copy link
Member

Choose a reason for hiding this comment

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

i think we should provide some defaults here. i started on this here https://github.com/bazelbuild/bazel-central-registry/pull/1943/files#diff-400335528b8cf846395d95f2877a3ff732563a63b7b0ef2be7bac192532e78acR627 actually not even allowing overrides. I think overrides are probably fine but this kinda locks us into a API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something similar was my first approach, in 16dfd0d.

I think it would be OK to start small and build this out iteratively.

It would also be great to allow the user to bring-your-own hermetic cabundle although admittedly I haven't actually tried that yet.

Copy link
Member

Choose a reason for hiding this comment

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

i think the defaults i linked are probably fine to start with so it "just works" in the common case on the explicitly supported OS's

keith added a commit that referenced this pull request Jul 8, 2024
Based on #2233
but provides platform specific defaults
@keith keith mentioned this pull request Jul 8, 2024
@keith
Copy link
Member

keith commented Jul 8, 2024

submitted #2381 w/ the platform defaults

keith added a commit to keith/curl that referenced this pull request Jul 9, 2024
Wyverald pushed a commit that referenced this pull request Jul 9, 2024
Based on #2233
but provides platform specific defaults
@lalten
Copy link
Contributor Author

lalten commented Jul 9, 2024

submitted #2381 w/ the platform defaults

Thanks @keith !

@lalten lalten closed this Jul 9, 2024
@lalten lalten deleted the curl-8.8.0 branch July 9, 2024 08:54
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

Successfully merging this pull request may close these issues.

None yet

4 participants