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

ci: update musl headers to 6.6 #3921

Merged
merged 2 commits into from
Nov 21, 2024
Merged

Conversation

tammela
Copy link
Contributor

@tammela tammela commented Sep 13, 2024

Update the musl headers in CI to use alpine-linux instead of sabotage-linux.
Alpine also uses musl but follows the linux stable releases, providing more up-to-date headers.

I also took the opportunity to clean up libc-test/build.rs a bit.

@rustbot
Copy link
Collaborator

rustbot commented Sep 13, 2024

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@tammela tammela force-pushed the musl-headers branch 5 times, most recently from 3d166ca to d14a03b Compare September 13, 2024 19:00
@tammela tammela marked this pull request as ready for review September 13, 2024 19:01
@bors
Copy link
Contributor

bors commented Nov 6, 2024

☔ The latest upstream changes (presumably #3922) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Nov 16, 2024

☔ The latest upstream changes (presumably #4042) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Sorry this hasn't been on my radar. Is the switch to Alpine just because of better maintenance?

Suggested some ways to clean up the shell script, also I think there are a couple other musl dockerfiles that will need to be updated now (loongarch?). libc-test/build.rs is probably pretty out of date, if you'd like we can merge the musl changes first and then clean up build.rs in a followup.

Comment on lines 67 to 70
# Download, configure, build, and install musl-sanitized kernel headers.
# This routine piggybacks on: https://git.alpinelinux.org/aports/tree/main/linux-headers?h=3.20-stable
# Alpine follows stable kernel releases, 3.20 uses Linux 6.6 headers.
git clone -n --depth=1 --filter=tree:0 -b 3.20-stable https://gitlab.alpinelinux.org/alpine/aports
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit to make use of variables

Suggested change
# Download, configure, build, and install musl-sanitized kernel headers.
# This routine piggybacks on: https://git.alpinelinux.org/aports/tree/main/linux-headers?h=3.20-stable
# Alpine follows stable kernel releases, 3.20 uses Linux 6.6 headers.
git clone -n --depth=1 --filter=tree:0 -b 3.20-stable https://gitlab.alpinelinux.org/alpine/aports
# Download, configure, build, and install musl-sanitized kernel headers.
# Alpine follows stable kernel releases, 3.20 uses Linux 6.6 headers.
alpine_version=3.20
alpine_git=https://gitlab.alpinelinux.org/alpine/aports
# This routine piggybacks on: https://git.alpinelinux.org/aports/tree/main/linux-headers?h=3.20-stable
git clone -n --depth=1 --filter=tree:0 -b "${alpine_version}-stable" "$alpine_git"

Comment on lines 76 to 100
cp APKBUILD APKBUILD.source
cp APKBUILD APKBUILD.sha512
{
echo "printf \"\$source\""
# shellcheck disable=SC2028
echo "printf \"\$_kernver\n\""
# shellcheck disable=SC2028
echo "printf \"\$pkgver\n\""
} >> APKBUILD.source
echo "printf \"\$sha512sums\"" >> APKBUILD.sha512
KERNEL_VER=$(bash APKBUILD.source | tail -2 | head -1 | tr -d "[:space:]")
PKGVER=$(bash APKBUILD.source | tail -1 | tr -d "[:space:]")
urls=$(bash APKBUILD.source | grep -o 'https.*')
kernel=""
patch=""
for url in $urls; do
base=$(basename "$url")
curl --retry 5 -L "$url" > "$base"
case $base in
linux-*) kernel=$base;;
patch-*) patch=$base;;
esac
done
bash APKBUILD.sha512 | grep "$kernel" >> sha-check
bash APKBUILD.sha512 | grep "$patch" >> sha-check
Copy link
Contributor

@tgross35 tgross35 Nov 19, 2024

Choose a reason for hiding this comment

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

I think this gets a bit cleaner with heredoc and writing the outputs to files rather than filtering:

Suggested change
cp APKBUILD APKBUILD.source
cp APKBUILD APKBUILD.sha512
{
echo "printf \"\$source\""
# shellcheck disable=SC2028
echo "printf \"\$_kernver\n\""
# shellcheck disable=SC2028
echo "printf \"\$pkgver\n\""
} >> APKBUILD.source
echo "printf \"\$sha512sums\"" >> APKBUILD.sha512
KERNEL_VER=$(bash APKBUILD.source | tail -2 | head -1 | tr -d "[:space:]")
PKGVER=$(bash APKBUILD.source | tail -1 | tr -d "[:space:]")
urls=$(bash APKBUILD.source | grep -o 'https.*')
kernel=""
patch=""
for url in $urls; do
base=$(basename "$url")
curl --retry 5 -L "$url" > "$base"
case $base in
linux-*) kernel=$base;;
patch-*) patch=$base;;
esac
done
bash APKBUILD.sha512 | grep "$kernel" >> sha-check
bash APKBUILD.sha512 | grep "$patch" >> sha-check
# Create a version of APKBUILD to extract variables
cp APKBUILD APKBUILD.vars
cat <<- 'EOF' >> APKBUILD.vars
printf "$source" > alpine-source
printf "$_kernver" > alpine-kernver
printf "$pkgver" > alpine-pkgver
printf "$sha512sums" > alpine-sha512sums
EOF
# Execute the build script to write the output files
sh APKBUILD.vars
alpine_source="$(cat alpine-source)"
kernel_version="$(cat alpine-kernver)"
pkg_version="$(cat alpine-pkgver)"
kernel=""
patch=""
urls="$(echo "$alpine_source" | grep -o 'https.*')"
# Download
for url in $urls; do
base=$(basename "$url")
curl --retry 5 -L "$url" > "$base"
case "$base" in
linux-*) kernel="$base";;
patch-*) patch="$base";;
*)
echo "Unexpected basename $base"
exit 1
;;
esac
done
grep "$kernel" alpine-sha512sums >> sha-check
grep "$patch" alpine-sha512sums >> sha-check

(I haven't tested this)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, does CARCH need to be set before running APKBUILD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, does CARCH need to be set before running APKBUILD?

Not in this case, we force the arch to our own $kernel_arch

@tammela
Copy link
Contributor Author

tammela commented Nov 19, 2024

Sorry this hasn't been on my radar. Is the switch to Alpine just because of better maintenance?

Sabotage is pretty much stuck on 4.19.88 for years now. Alpine follows the stable releases from www.kernel.org and uses musl.
As you saw, this is pretty much a hack to piggy back on Alpine's patches for the kernel headers.
I wouldn't claim it's better maintenance over sabotage, but rather it's a better fit since we want to test and add the shiniest new features :)

I will update the PR to take into account your suggestions

@tammela tammela force-pushed the musl-headers branch 4 times, most recently from ae76331 to 9575471 Compare November 19, 2024 16:59
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Sabotage is pretty much stuck on 4.19.88 for years now. Alpine follows the stable releases from www.kernel.org and uses musl. As you saw, this is pretty much a hack to piggy back on Alpine's patches for the kernel headers. I wouldn't claim it's better maintenance over sabotage, but rather it's a better fit since we want to test and add the shiniest new features :)

Thanks for the context, that makes a lot of sense 👍

Two small suggestions for the script then this looks good to me. One other nit - it seems like your bash is indented by three spaces, mind changing to 4 like the rest of the scripts?

Comment on lines 82 to 90
{
echo "printf \"\$source\" > alpine-source"
# shellcheck disable=SC2028
echo "printf \"\$_kernver\n\" > alpine-kernver"
# shellcheck disable=SC2028
echo "printf \"\$pkgver\n\" > alpine-pkgver"
# shellcheck disable=SC2028
echo "printf \"\$sha512sums\n\" > alpine-sha512sums"
} >> APKBUILD.vars
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just use heredoc rather than the nested quoting? Which shouldn't hit the shellcheck warnings

Comment on lines 96 to 106
urls=$(grep -o 'https.*' alpine-source)
kernel=""
patch=""
for url in $urls; do
base=$(basename "$url")
curl --retry 5 -L "$url" > "$base"
case $base in
linux-*) kernel=$base;;
patch-*) patch=$base;;
esac
done
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the following work instead?

Suggested change
urls=$(grep -o 'https.*' alpine-source)
kernel=""
patch=""
for url in $urls; do
base=$(basename "$url")
curl --retry 5 -L "$url" > "$base"
case $base in
linux-*) kernel=$base;;
patch-*) patch=$base;;
esac
done
urls=$(grep -o 'https.*' alpine-source)
for url in $urls; do
base=$(basename "$url")
# We only need to download `linux` and the patch archive, not individual
# patches.
case "$base" in
linux-*) kernel="$base";;
patch-*) patch="$base";;
*) continue;;
esac
curl --retry 5 -L "$url" -o "$base"
grep "$base" alpine-sha512sums >> sha-check
done

Then remove the grep "$kernel/patch" alpine-sha512sums >> sha-check lines below. This makes sure we don't download/use anything that we don't verify the sha of.

If we do actually need to download all the files (revert-broken-uapi.patch and 0003-remove-inclusion-of-sysinfo.h-in-kernel.h.patch), then remove *) continue and >> sha-check, instead just sha512sum -c alpine-sha512sums.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the latter. I added an explicit check to avoid downloading files that do not contain a checksum.

@bors
Copy link
Contributor

bors commented Nov 20, 2024

☔ The latest upstream changes (presumably #4120) made this pull request unmergeable. Please resolve the merge conflicts.

@tammela tammela force-pushed the musl-headers branch 2 times, most recently from 77523c0 to 4c6fc50 Compare November 21, 2024 15:59
Update the musl headers in CI to use alpine-linux instead of sabotage-linux.
Alpine also uses musl but follows the linux stable releases, providing more up-to-date headers.

Signed-off-by: Pedro Tammela <pctammela@gmail.com>
Now that we have Linux 6.6 we can clean up some of the test exceptions.
Not all of them can be removed, so the comments are updated if needed.

Signed-off-by: Pedro Tammela <pctammela@gmail.com>
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Great, thanks for getting this working!

@tgross35 tgross35 added this pull request to the merge queue Nov 21, 2024
@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Nov 21, 2024
@tgross35 tgross35 mentioned this pull request Nov 21, 2024
3 tasks
Merged via the queue into rust-lang:main with commit 0a8f04a Nov 21, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author stable-nominated This PR should be considered for cherry-pick to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants