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

fix build for musl 1.2.3 #67772

Merged
merged 1 commit into from
May 2, 2022
Merged

fix build for musl 1.2.3 #67772

merged 1 commit into from
May 2, 2022

Conversation

ayakael
Copy link
Contributor

@ayakael ayakael commented Apr 8, 2022

Fixes #67763

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 8, 2022
@ayakael ayakael force-pushed the fix-musl-build-123 branch from fa618b3 to 3e0362a Compare April 8, 2022 21:28
Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

Tested it locally on macOS, this fixes the innerloop build.

@ayakael ayakael force-pushed the fix-musl-build-123 branch from 3e0362a to dfbe4d5 Compare April 8, 2022 22:16
@ayakael
Copy link
Contributor Author

ayakael commented Apr 8, 2022

@am11 Perfect, that did it. I'm seeing success on Alpine pipelines. Many thanks!

@am11
Copy link
Member

am11 commented Apr 8, 2022

@ayakael, thanks for adding musl-libc v1.2.3 support early in its lifecycle! 👍

One unique thing about CoreCLR PAL is that it is essentially implementing Windows APIs on Unix, which rest of the code under src/coreclr (incl. JIT) uses on Unix. For instance, in this case, it is the implementation of this win32 API: https://docs.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedexchangepointer.

The way palinternal is written is it undefines then redefines the low level functions (like those belonging to printf() family) which normally come from standard C/C++ toolchain. This is to harmonize the behavior of known APIs with their Windows counterparts, so rest of the code under src/coreclr don't have to care about platform or API behavior differences. Therefore, sometimes you may notice slight mismatches between PAL implementation and the standard C/C++ ones.

@am11 am11 requested a review from janvorli April 8, 2022 23:19
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM modulo the two nits.

@janvorli
Copy link
Member

janvorli commented Apr 9, 2022

@ayakael thank you for the fix!

@ayakael ayakael force-pushed the fix-musl-build-123 branch from dfbe4d5 to f78b63b Compare April 9, 2022 00:21
@ayakael
Copy link
Contributor Author

ayakael commented Apr 9, 2022

@ayakael thank you for the fix!

you're welcome ^^

@ayakael, thanks for adding musl-libc v1.2.3 support early in its lifecycle! +1

One unique thing about CoreCLR PAL is that it is essentially implementing Windows APIs on Unix, which rest of the code under src/coreclr (incl. JIT) uses on Unix. For instance, in this case, it is the implementation of this win32 API: https://docs.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedexchangepointer.

The way palinternal is written is it undefines then redefines the low level functions (like those belonging to printf() family) which normally come from standard C/C++ toolchain. This is to harmonize the behavior of known APIs with their Windows counterparts, so rest of the code under src/coreclr don't have to care about platform or API behavior differences. Therefore, sometimes you may notice slight mismatches between PAL implementation and the standard C/C++ ones.

many thanks for sharing what PAL is for ^^ Being a packager first, programmer after, means that I mostly throw spaguetti at a wall to see if it sticks. It's nice to be able to have a background understanding on why things are as they are.

@nekopsykose
Copy link

this pull request (https://gitlab.alpinelinux.org/alpine/aports/-/commit/2f56551abc53b818bab9f22c4f7972b5ad5aad66 , just missing the two nits) applied onto our alpine build fixed it, and it works again :) (just notifying, since it was alpine that ran into this first)

@ayakael
Copy link
Contributor Author

ayakael commented Apr 9, 2022

this pull request (https://gitlab.alpinelinux.org/alpine/aports/-/commit/2f56551abc53b818bab9f22c4f7972b5ad5aad66 , just missing the two nits) applied onto our alpine build fixed it, and it works again :) (just notifying, since it was alpine that ran into this first)

Many thanks, I had forgot to update (for the two nits - next update since they are just comments)

@mangod9 mangod9 merged commit 809dd7c into dotnet:main May 2, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build of coreclr fails with musl 1.2.3
5 participants