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.

Sign up for GitHub

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

Jump to bottom

shims: add curl-config shim to fix curl-config http2 report issue for ventura builds #18773

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+35 −0
Open

shims: add curl-config shim to fix curl-config http2 report issue for ventura builds #18773

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions Library/Homebrew/shims/mac/super/curl-config
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#!/bin/bash -p

# HOMEBREW_LIBRARY is set by bin/brew
# HOMEBREW_CURL_CONFIG is set by brew.sh
# shellcheck disable=SC2154
source "${HOMEBREW_LIBRARY}/Homebrew/shims/utils.sh"

# Check if HOMEBREW_CURL_CONFIG is set, fallback to system curl-config if not.
curl_config_exec="${HOMEBREW_CURL_CONFIG:-/usr/bin/curl-config}"
Comment on lines +7 to +9
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to introduce HOMEBREW_CURL_CONFIG.

Suggested change
# 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.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to introduce HOMEBREW_CURL_CONFIG.

Agreed.


# Intercept --features and append HTTP2 if needed
if [[ "$1" == "--features" ]]
then
output="$("${curl_config_exec}" "$@")"

# 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"
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why we'd want to claim HTTP2 support is present when curl says its not?

Copy link
Member Author

Choose a reason for hiding this comment

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

curl-config --features did not report HTTP2 prior to macOS 14

even though curl does support http2

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, that should also work.

fi
fi
Comment on lines +16 to +24
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

make sense 👍


echo "${output}"
exit 0
fi

# Fall back to executing the original curl-config
try_exec_non_system "${curl_config_exec}" "$@"
Copy link
Member

@Bo98 Bo98 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

safe_exec "/usr/bin/curl-config" "$@"

echo "Could not execute curl-config. Try HOMEBREW_FORCE_BREWED_CURL_CONFIG=1" >&2
exit 1
Comment on lines +32 to +35
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

Loading