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

Update Pony musl Docker images to Alpine 3.20 #4562

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

SeanTAllen
Copy link
Member

@SeanTAllen SeanTAllen commented Dec 3, 2024

This commit updates our CI to test using Alpine 3.20 for musl testing.
It also updates our release and nightly alpine ponyc images to be based
on Alpine 3.20.

Due to a change in the version of musl in Alpine 3.20, this required
a small patch LLVM 15 to compile. When we move to LLVM 16 or above,
the patch shouldn't be needed.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Dec 3, 2024
@SeanTAllen
Copy link
Member Author

This appears to need the following (or our equiv) LLVM patch: https://github.com/llvm/llvm-project-release-prs/pull/269/files

@SeanTAllen SeanTAllen force-pushed the alpine-3-20 branch 3 times, most recently from 83fb5e3 to af74d8d Compare December 3, 2024 02:43
@SeanTAllen
Copy link
Member Author

SeanTAllen commented Dec 3, 2024

@chalcolith what am i doing wrong here with that patching? i seem to be getting a SHA related error.

@SeanTAllen SeanTAllen force-pushed the alpine-3-20 branch 3 times, most recently from d7b30c1 to 6609fbd Compare December 3, 2024 03:25
@chalcolith
Copy link
Member

@SeanTAllen you wiped out my fix for the hashing

@chalcolith
Copy link
Member

diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
index 68850909..a30d9b37 100644
--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -55,7 +55,7 @@ install(TARGETS blake2
 find_package(Git)

 set(LLVM_DESIRED_HASH "8dfdcc7b7bf66834a761bd8de445840ef68e4d1a")
-set(PATCHES_DESIRED_HASH "3e16c097794cb669a8f6a0bd7600b440205ac5c29a6135750c2e83263eb16a95")
+set(PATCHES_DESIRED_HASH "99d46d25e96861de9847c055aeda1fef13fa0219524b2b7074c9acedb5f7a1c4")

 if(GIT_FOUND)
     if(EXISTS "${PROJECT_SOURCE_DIR}/../.git")
@@ -93,11 +93,9 @@ if(GIT_FOUND)
     set(PATCHES_ACTUAL_HASH "needed_if_no_patches")
     foreach (PATCH ${PONY_LLVM_PATCHES})
         file(STRINGS ${PATCH} patch_file NEWLINE_CONSUME)
-        string(REPLACE "\n" " " patch_file ${patch_file})
-        string(SHA256 patch_file_hash ${patch_file})
-        # message("${PATCH}: '${patch_file_hash}'")
+        string(REPLACE "\n" " " patch_file "${patch_file}")
+        string(SHA256 patch_file_hash "${patch_file}")
         string(CONCAT PATCHES_ACTUAL_HASH ${PATCHES_ACTUAL_HASH} ${patch_file_hash})
-        # message("concat is '${PATCHES_ACTUAL_HASH}'")
     endforeach()
     string(SHA256 PATCHES_ACTUAL_HASH ${PATCHES_ACTUAL_HASH})
     message("Desired hash ${PATCHES_DESIRED_HASH}")

@SeanTAllen SeanTAllen force-pushed the alpine-3-20 branch 3 times, most recently from 868494a to a18fad2 Compare December 3, 2024 03:46
@SeanTAllen
Copy link
Member Author

SeanTAllen commented Dec 3, 2024

@chalcolith ill try that but i tried something like that already and it didnt work.

string(SHA256 patch_file_hash "${patch_file}") was erroring out.

@SeanTAllen
Copy link
Member Author

@chalcolith i'm not sure why your patch worked and mine didn't but thanks.

@SeanTAllen SeanTAllen changed the title Update supported Alpine to 3.20 Update Pony musl Docker images to Alpine 3.20 Dec 3, 2024
@SeanTAllen SeanTAllen requested a review from a team December 3, 2024 04:20
@SeanTAllen SeanTAllen added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Dec 3, 2024
@ponylang-main ponylang-main removed the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Dec 3, 2024
@ponylang-main
Copy link
Contributor

Hi @SeanTAllen,

The changelog - changed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4562.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@SeanTAllen SeanTAllen marked this pull request as ready for review December 3, 2024 04:45
@SeanTAllen
Copy link
Member Author

We currently include usage of lseek64 whenever we are building pony on Linux but alpine with musl is missing lseek64 in recent versions.

Will need to address.

@SeanTAllen
Copy link
Member Author

image

@SeanTAllen
Copy link
Member Author

SeanTAllen commented Dec 3, 2024

I think the approach to fix this would be to add a new lseek function to the runtime.

On Linux:

IF GLIBC is defined, we call into lseek64, if not, we call into lseek

We can pull in the other platforms as well.

We probably need to do the same for ftruncate64 on Linux as I believe musl doesn't support it either.

@SeanTAllen SeanTAllen force-pushed the alpine-3-20 branch 2 times, most recently from c28bd29 to 54ac08a Compare December 3, 2024 14:32
@dipinhora
Copy link
Contributor

according to musl release notes (at https://musl.libc.org/releases.html):

On the API level, the legacy "LFS64" ("large file support") interfaces, which were provided by macros remapping them to their standard names (#define stat64 stat and similar) have been deprecated and are no longer provided under the _GNU_SOURCE feature profile, only under explicit _LARGEFILE64_SOURCE. The latter will also be removed in a future version. Builds broken by this change can be fixed short-term by adding -D_LARGEFILE64_SOURCE to CFLAGS, but should be fixed to use the standard interfaces.

maybe setting -D_LARGEFILE64_SOURCE could be a temporary hack for now?

also, from what i understand, lseek64/ftruncate64/etc should only be necessary on 32 bit builds because 64 bit builds always have off_t as a 64 bit type for all the "normal" lseek/ftruncate/etc functions so maybe we can use the ilp32 platform define that exists in some way?

@SeanTAllen
Copy link
Member Author

SeanTAllen commented Dec 3, 2024

according to musl release notes (at https://musl.libc.org/releases.html):

On the API level, the legacy "LFS64" ("large file support") interfaces, which were provided by macros remapping them to their standard names (#define stat64 stat and similar) have been deprecated and are no longer provided under the _GNU_SOURCE feature profile, only under explicit _LARGEFILE64_SOURCE. The latter will also be removed in a future version. Builds broken by this change can be fixed short-term by adding -D_LARGEFILE64_SOURCE to CFLAGS, but should be fixed to use the standard interfaces.

maybe setting -D_LARGEFILE64_SOURCE could be a temporary hack for now?

It won't help when compiling pony programs that require linking to the proper function from file.pony c-ffi.

also, from what i understand, lseek64/ftruncate64/etc should only be necessary on 32 bit builds because 64 bit builds always have off_t as a 64 bit type for all the "normal" lseek/ftruncate/etc functions so maybe we can use the ilp32 platform define that exists in some way?

lseek/ftruncate/etc functions so maybe we can use the ilp32 platform define that exists in some way?

We always require a 64 bit off_t which glib always supplies via the 64 functions and musl always provides. So checking the platform size doesn't help there. On a 32bit platform we might have 64 bit off_t which we need, but the function to call is not clear because -D_LARGEFILE64_SOURCE is opaque to us and happened at an entirely different time. The safest thing is calling the 64 functions on glibc as it will always be correct and calling the non-64 on musl as that will always be correct.

a couple new runtime methods are inexpensive and quick to add without being a temporary hack. assuming the rest of @ponylang/committer agrees with the approach.

another option is dropping support for 32 bit platforms. Then we can "just use lseek" for all Linux.

@dipinhora
Copy link
Contributor

It won't help when compiling pony programs that require linking to the proper function from file.pony c-ffi.

blargh.. i always forget about this and the fancy preprocessor magic that happens in glibc for this stuff.. 8*/

another option is dropping support for 32 bit platforms. Then we can "just use lseek" for all Linux.

or drop 32 bit support for all non-glibc or non-musl platforms by using ilp32 define and picking either lseek64 or lseek and always using lseek for all 64 bit builds.. picking 32 bit for glibc only would get us passing the CI since we don't explicitly have a build for 32 bit musl iirc..

either way.. i'm sure you and the others will figure out the right way to move forward..


on an semi-unrelated note, apparently a lot of 32 bit distributions are working on switching time_t from 32 bits to 64 bits... more "fun"... 8*/

@SeanTAllen
Copy link
Member Author

Ill work on adding lseek and ftruncate wrappers to the runtime.

@SeanTAllen SeanTAllen force-pushed the alpine-3-20 branch 5 times, most recently from b563989 to d7d0d17 Compare December 3, 2024 21:40
@SeanTAllen
Copy link
Member Author

lseek64 is in place. "truncate64" is up next.

@SeanTAllen SeanTAllen marked this pull request as draft December 3, 2024 21:51
@SeanTAllen SeanTAllen force-pushed the alpine-3-20 branch 2 times, most recently from 50ff80c to 6ff3071 Compare December 4, 2024 00:06
@SeanTAllen SeanTAllen marked this pull request as ready for review December 4, 2024 00:57
@SeanTAllen
Copy link
Member Author

@jemc can I get a review?

This commit updates our CI to test using Alpine 3.20 for musl testing.
It also updates our release and nightly alpine ponyc images to be based
on Alpine 3.20.

Due to a change in the version of musl in Alpine 3.20, this required
a small patch LLVM 15 to compile. When we move to LLVM 16 or above,
the patch shouldn't be needed.

The new version of musl doesn't support LSF64 methods like lseek64.
In order to keep the standard library working with the new musl,
this commit adds a couple new "pony_os" functions for encapsulating
lseek and ftruncate behavior that varies by OS and on Linux, between
musl and Glibc.
@SeanTAllen SeanTAllen merged commit 323bcc7 into main Dec 4, 2024
24 checks passed
@SeanTAllen SeanTAllen deleted the alpine-3-20 branch December 4, 2024 03:33
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Dec 4, 2024
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.

5 participants