shims: add curl-config shim to fix curl-config http2 report issue for ventura builds #18773
Conversation
… ventura builds Signed-off-by: Rui Chen <rui@chenrui.dev>
chenrui333
force-pushed
the
add-curl-config-shim
branch
from
3f48597
to
6b3a702
Compare
| # Check if HTTP2 is missing but supported | ||
| if [[ ! "${output}" =~ HTTP2 ]] | ||
| then | ||
| curl_version="$("${HOMEBREW_CURL:-curl}" -V)" | ||
| if [[ "${curl_version}" =~ HTTP/2 ]] | ||
| then | ||
| output="${output} HTTP2" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
It might be easier and cheaper to just add an append_to_cccfg here: https://github.com/Homebrew/brew/blob/main/Library/Homebrew/extend/os/mac/extend/ENV/super.rb#L161 gated to >= 10.13 && < 14, using a letter that isn't already used like h or C or something. Then here you can check if [[ "${HOMEBREW_CCCFG}" = *h* ]] before running most of the code here.
Also makes it more obvious we can remove it if we drop < macOS 14 support in the far future and means none of the hacks will run in macOS 14 and later which is safer in case there's edge cases we've missed.
| fi | ||
|
|
||
| # Fall back to executing the original curl-config | ||
| try_exec_non_system "${curl_config_exec}" "$@" |
There was a problem hiding this comment.
Move this before the code above - it should not apply to non-system curl-config as only system curl-config is bugged.
The bug wasn't because of curl - it was because Apple shipped their own custom curl-config that they neglected to update for a while.
|
|
||
| # Check if HOMEBREW_CURL_CONFIG is set, fallback to system curl-config if not. | ||
| curl_config_exec="${HOMEBREW_CURL_CONFIG:-/usr/bin/curl-config}" |
There was a problem hiding this comment.
I don't think we need to introduce HOMEBREW_CURL_CONFIG.
| # Check if HOMEBREW_CURL_CONFIG is set, fallback to system curl-config if not. | |
| curl_config_exec="${HOMEBREW_CURL_CONFIG:-/usr/bin/curl-config}" |
if you move the try_exec_non_system high enough you can hardcode /usr/bin/curl-config (or /usr/bin/${SHIM_FILE}) elsewhere.
There was a problem hiding this comment.
I don't think we need to introduce
HOMEBREW_CURL_CONFIG.
Agreed.
| safe_exec "/usr/bin/curl-config" "$@" | ||
|
|
||
| echo "Could not execute curl-config. Try HOMEBREW_FORCE_BREWED_CURL_CONFIG=1" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
| safe_exec "/usr/bin/curl-config" "$@" | |
| echo "Could not execute curl-config. Try HOMEBREW_FORCE_BREWED_CURL_CONFIG=1" >&2 | |
| exit 1 | |
| exec "/usr/bin/curl-config" "$@" |
HOMEBREW_FORCE_BREWED_CURL_CONFIG doesn't exist
|
Thanks! Looks good overall. |
| curl_version="$("${HOMEBREW_CURL:-curl}" -V)" | ||
| if [[ "${curl_version}" =~ HTTP/2 ]] | ||
| then | ||
| output="${output} HTTP2" |
There was a problem hiding this comment.
Can you explain why we'd want to claim HTTP2 support is present when curl says its not?
There was a problem hiding this comment.
curl-config --featuresdid not report HTTP2 prior to macOS 14
even though curl does support http2
There was a problem hiding this comment.
Gotcha, thanks. I'd be tempted to just use Homebrew's curl in that case rather than have a shim. Can we use uses_from_macos "curl", since: instead to handle macOS 13 and below?
There was a problem hiding this comment.
yeah, that should also work.
There was a problem hiding this comment.
@MikeMcQuaid that hack (uses_from_macos "curl", since: :sonoma) would actually enable the linkage against system curl, see this build log.
In other words, I still feel like it is better to patch the curl-config report. I know this would be only for ventura builds. Let me know what you think.
cc @Bo98
(we have already merged the linked PRs on the homebrew-core side)
There was a problem hiding this comment.
@MikeMcQuaid that hack (
uses_from_macos "curl", since: :sonoma) would actually enable the linkage against system curl, see this build log.
@chenrui333 on which macOS versions would it link against/not link against system curl? Thanks!
There was a problem hiding this comment.
I know this would be only for ventura builds.
For one, non-latest macOS version this seems overkill, personally, but I'm open to thoughts from other maintainers.
There was a problem hiding this comment.
yeah, this only happens for ventura builds, for sonoma/sequoia builds, it works fine.
There was a problem hiding this comment.
@chenrui333 Thanks, not worth doing for a single OS version IMO, sorry.
|
also trying to use brewed curl for ventura builds. |
|
close as the issues got sorted out in the core tap side. |
|
Thanks @chenrui333 ❤️ |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?seeing some ventura build linkage failure due to curl-config reporting issue, adding the shim as result per @Bo98's suggestion.
uses_from_macos "curl"homebrew-core#197727