Skip to content

Commit fe462a2

Browse files
committed
Align lib/alpine.func with the tools tree, and fix what the comparison found
alpine-parity.yml warns when a pull request changes a shared function on one side only. It cannot say anything about drift that is already there, and nobody had looked. Comparing all 22 shared functions turned up six real divergences, two of them bugs that had nothing to do with Alpine being different and everything to do with the copy never being reviewed against the original. github_api_call was broken twice over: http_code=$(curl -fsSL -w "%{http_code}" ... || echo 000) With -f, curl exits non-zero on an HTTP error while -w still prints the status, so the `|| echo 000` appended to it. http_code came out as "404000", which matches neither the 404 arm nor the 403 arm -- both were dead code. Every failure fell through to the catch-all, meaning a 404 was retried three times, roughly six seconds spent on a URL that was never going to exist, and then reported as "failed with HTTP 404000". The second one: header="-H Authorization:Bearer\ ${GITHUB_TOKEN}" # ... used as $header Expanded unquoted, that splits into three words: curl received a valueless Authorization header (which it drops) and the token as a second URL to fetch. Alpine never authenticated to the GitHub API, however valid the token was -- so it took the anonymous rate limit on every call. Both are fixed and checked against a local server: 200 returns 0 with the body written, 404 returns 22 immediately, 403 returns 22 after backoff, each with its own message. The rest were contract mismatches rather than broken behaviour: - get_cached_version returned 0 with no cache file, where the tools tree returns 1. `if get_cached_version app` was true on Alpine with empty output. - download_file, extract_version_from_json and get_latest_github_release returned 1 for everything. The tools tree distinguishes 22 (could not reach it) from 250 (reached it, no version). Exit codes are what the error handler and the telemetry read, so every Alpine failure was landing in the same undifferentiated exit-1 bucket that already dominates the error stats. - create_temp_dir was a bare mktemp -d with no cleanup, leaking one directory per call; the tools tree registers them for removal on exit. - get_os_info had no version_full, so asking for it fell through to the default arm and returned the OS id instead of a version. - get_system_arch ignored the dpkg|uname|both argument the other side takes. Alpine has no dpkg so every mode resolves through uname, but it accepts the argument now instead of silently dropping it. Left alone deliberately: version_gt. The two implementations genuinely disagree -- for "1.2.0-rc1" against "1.2.0" Alpine says false and the tools tree says true -- and the tools tree is the one that is wrong, since a release candidate ranks below its release. Its only caller is should_upgrade, which has no callers at all. Fixing dead code properly means implementing semver ordering that works under BusyBox, which does not belong in a change that is already touching eight live functions.
1 parent 7968172 commit fe462a2

1 file changed

Lines changed: 91 additions & 25 deletions

File tree

‎lib/alpine.func‎

Lines changed: 91 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,9 @@ get_cached_version() {
3737
cat "/var/cache/app-versions/${app}_version.txt"
3838
return 0
3939
fi
40-
return 0
40+
# 1, not 0. The tools tree returns 1 here, and `if get_cached_version app`
41+
# was therefore true on Alpine with nothing cached and empty output.
42+
return 1
4143
}
4244

4345
version_gt() {
@@ -57,6 +59,10 @@ version_gt() {
5759
}'
5860
}
5961

62+
# The tools tree takes dpkg|uname|both and defaults to dpkg. Alpine has no
63+
# dpkg, so every mode resolves through uname -- but the argument is accepted
64+
# rather than silently ignored, so a caller written against the other side
65+
# behaves the same here.
6066
get_system_arch() {
6167
local arch
6268
arch=$(uname -m 2>/dev/null || echo "")
@@ -65,19 +71,41 @@ get_system_arch() {
6571
echo "$arch"
6672
}
6773

74+
# The tools tree registers every temp dir for cleanup on exit. Doing nothing
75+
# here leaked one per call, so keep a list and clear it the same way.
76+
_alpine_temp_dirs=()
77+
_alpine_cleanup_temp_dirs() {
78+
local d
79+
for d in "${_alpine_temp_dirs[@]}"; do
80+
[ -n "$d" ] && [ -d "$d" ] && rm -rf "$d"
81+
done
82+
_alpine_temp_dirs=()
83+
}
84+
6885
create_temp_dir() {
69-
mktemp -d
86+
local tmp_dir
87+
tmp_dir=$(mktemp -d) || return 1
88+
if [ "${_alpine_temp_cleanup_registered:-0}" != "1" ]; then
89+
trap _alpine_cleanup_temp_dirs EXIT
90+
_alpine_temp_cleanup_registered=1
91+
fi
92+
_alpine_temp_dirs+=("$tmp_dir")
93+
echo "$tmp_dir"
7094
}
7195

7296
get_os_info() {
7397
local field="${1:-all}"
7498
[ -z "${_OS_ID:-}" ] && _OS_ID=$(awk -F= '/^ID=/{gsub(/"/,"",$2); print $2}' /etc/os-release 2>/dev/null)
7599
[ -z "${_OS_CODENAME:-}" ] && _OS_CODENAME=$(awk -F= '/^VERSION_CODENAME=/{gsub(/"/,"",$2); print $2}' /etc/os-release 2>/dev/null)
76100
[ -z "${_OS_VERSION:-}" ] && _OS_VERSION=$(awk -F= '/^VERSION_ID=/{gsub(/"/,"",$2); print $2}' /etc/os-release 2>/dev/null)
101+
# version_full was missing, so asking for it fell through to *) and returned
102+
# the OS id instead of a version.
103+
[ -z "${_OS_VERSION_FULL:-}" ] && _OS_VERSION_FULL=$(awk -F= '/^VERSION=/{gsub(/"/,"",$2); print $2}' /etc/os-release 2>/dev/null)
77104
case "$field" in
78105
id) echo "$_OS_ID" ;;
79106
codename) echo "$_OS_CODENAME" ;;
80107
version | version_id) echo "$_OS_VERSION" ;;
108+
version_full) echo "$_OS_VERSION_FULL" ;;
81109
all) echo "ID=$_OS_ID CODENAME=$_OS_CODENAME VERSION=$_OS_VERSION" ;;
82110
*) echo "$_OS_ID" ;;
83111
esac
@@ -97,69 +125,107 @@ ensure_dependencies() {
97125

98126
download_file() {
99127
local url="$1" output="$2" max_retries="${3:-3}" show_progress="${4:-false}"
100-
local i=1 curl_opts="-fsSL"
101-
[ "$show_progress" = "true" ] && curl_opts="-fL#"
128+
local i=1
129+
local curl_opts=(-fsSL)
130+
[ "$show_progress" = "true" ] && curl_opts=(-fL#)
102131
while [ $i -le "$max_retries" ]; do
103-
if curl $curl_opts -o "$output" "$url"; then
132+
if curl "${curl_opts[@]}" -o "$output" "$url"; then
104133
return 0
105134
fi
106135
i=$((i + 1))
107-
[ $i -le "$max_retries" ] && sleep 2
136+
[ $i -le "$max_retries" ] && {
137+
msg_warn "Download failed, retrying... (attempt $((i - 1))/$max_retries)"
138+
sleep 2
139+
}
108140
done
109141
msg_error "Failed to download: $url"
110-
return 1
142+
# 250, matching lib/system.func. The exit code is what the error handler and
143+
# the telemetry read; returning 1 filed every Alpine download failure in the
144+
# same undifferentiated bucket as everything else that returns 1.
145+
return 250
111146
}
112147

113148
github_api_call() {
114149
local url="$1" output_file="${2:-/dev/stdout}"
115150
local max_retries=3 retry_delay=2 attempt=1
116-
local header=""
117-
[ -n "${GITHUB_TOKEN:-}" ] && header="-H Authorization:Bearer\ ${GITHUB_TOKEN}"
151+
local http_code
152+
153+
# An array, not a string. The old form built "-H Authorization:Bearer\ $TOKEN"
154+
# and expanded it unquoted, which split into three words: curl got a valueless
155+
# header (which it then drops) and the token as a second URL to fetch. Alpine
156+
# never authenticated, however valid the token was.
157+
local header_args=()
158+
[ -n "${GITHUB_TOKEN:-}" ] && header_args=(-H "Authorization: Bearer ${GITHUB_TOKEN}")
159+
118160
while [ $attempt -le $max_retries ]; do
119-
http_code=$(curl -fsSL -w "%{http_code}" -o "$output_file" \
161+
# -sSL, not -fsSL. With -f curl exits non-zero on an HTTP error while -w
162+
# still prints the status, so the old `|| echo 000` appended to it and
163+
# http_code came out as "404000" -- making the 403 and 404 arms below
164+
# unreachable. Every failure fell through to the catch-all, and a 404 was
165+
# retried three times for a URL that was never going to exist.
166+
http_code=$(curl -sSL -w "%{http_code}" -o "$output_file" \
120167
-H "Accept: application/vnd.github+json" \
121168
-H "X-GitHub-Api-Version: 2022-11-28" \
122-
$header "$url" 2>/dev/null || echo 000)
169+
"${header_args[@]}" "$url" 2>/dev/null) || true
170+
[ -z "$http_code" ] && http_code=000
171+
123172
case "$http_code" in
124173
200) return 0 ;;
125-
403) [ $attempt -lt $max_retries ] && sleep "$retry_delay" || {
126-
msg_error "GitHub API rate limit exceeded"
127-
return 1
174+
401)
175+
msg_error "GitHub API authentication failed (HTTP 401)"
176+
[ -n "${GITHUB_TOKEN:-}" ] && msg_error "GITHUB_TOKEN appears to be invalid or expired"
177+
return 22
178+
;;
179+
403) [ $attempt -lt $max_retries ] || {
180+
msg_error "GitHub API rate limit exceeded (HTTP 403)"
181+
return 22
128182
} ;;
129183
404)
130184
msg_error "GitHub API endpoint not found: $url"
131-
return 1
185+
return 22
132186
;;
133-
*) [ $attempt -lt $max_retries ] && sleep "$retry_delay" || {
187+
*) [ $attempt -lt $max_retries ] || {
134188
msg_error "GitHub API call failed with HTTP $http_code"
135-
return 1
189+
return 22
136190
} ;;
137191
esac
192+
sleep "$retry_delay"
138193
retry_delay=$((retry_delay * 2))
139194
attempt=$((attempt + 1))
140195
done
141-
return 1
196+
return 22
142197
}
143198

144199
extract_version_from_json() {
145200
local json="$1" field="${2:-tag_name}" strip_v="${3:-true}" version
146-
need_tool jq || return 1
201+
need_tool jq || return 250
147202
version=$(printf '%s' "$json" | jq -r ".${field} // empty")
148-
[ -z "$version" ] && return 1
203+
if [ -z "$version" ]; then
204+
msg_warn "JSON field '${field}' is empty in API response"
205+
return 250
206+
fi
149207
[ "$strip_v" = "true" ] && printf '%s' "${version#v}" || printf '%s' "$version"
150208
}
151209

152210
get_latest_github_release() {
153-
local repo="$1" strip_v="${2:-true}" tmp
154-
tmp=$(mktemp) || return 1
211+
local repo="$1" strip_v="${2:-true}" tmp rc version
212+
tmp=$(mktemp) || return 250
213+
# 22 for "could not reach the API", 250 for "reached it but got no version",
214+
# the same split lib/forge.func uses. Both used to be 1 here, which the error
215+
# handler cannot tell apart from any other failure.
155216
github_api_call "https://api.github.com/repos/${repo}/releases/latest" "$tmp" || {
217+
msg_warn "GitHub API call failed for ${repo}"
156218
rm -f "$tmp"
157-
return 1
219+
return 22
158220
}
159-
extract_version_from_json "$(cat "$tmp")" "tag_name" "$strip_v"
221+
version=$(extract_version_from_json "$(cat "$tmp")" "tag_name" "$strip_v")
160222
rc=$?
161223
rm -f "$tmp"
162-
return $rc
224+
if [ $rc -ne 0 ] || [ -z "$version" ]; then
225+
msg_error "Could not determine latest version for ${repo}"
226+
return 250
227+
fi
228+
printf '%s' "$version"
163229
}
164230

165231
need_tool() {

0 commit comments

Comments
 (0)