-
Notifications
You must be signed in to change notification settings - Fork 234
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
feat: linux musl detection #502
base: master
Are you sure you want to change the base?
feat: linux musl detection #502
Conversation
There is some flakiness in the tests, I've had to rerun a couple, I've not ran enough jobs from master to see if this is something that has been introduced by this change, or an issue in the ffi lib or the test setup. [xUnit.net 00:00:01.24] PactNet.Tests.Verifier.Messaging.MessagingProviderTests.HandleMessage_NoDescription_ReturnsBadRequest [FAIL]
[xUnit.net 00:00:01.24] PactNet.Exceptions.PactFailureException : Unable to start the internal messaging server
[xUnit.net 00:00:01.24] ---- System.Net.HttpListenerException : Address in use
[xUnit.net 00:00:01.24] Stack Trace:
[xUnit.net 00:00:01.24] /home/runner/work/pact-net/pact-net/src/PactNet/Verifier/Messaging/MessagingProvider.cs(85,0): at PactNet.Verifier.Messaging.MessagingProvider.Start(JsonSerializerOptions settings)
[xUnit.net 00:00:01.24] /home/runner/work/pact-net/pact-net/tests/PactNet.Tests/Verifier/Messaging/MessagingProviderTests.cs(45,0): at PactNet.Tests.Verifier.Messaging.MessagingProviderTests..ctor(ITestOutputHelper output)
[xUnit.net 00:00:01.24] at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
[xUnit.net 00:00:01.24] at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
[xUnit.net 00:00:01.24] ----- Inner Stack Trace -----
[xUnit.net 00:00:01.24] at System.Net.HttpEndPointManager.GetEPListener(String host, Int32 port, HttpListener listener, Boolean secure)
[xUnit.net 00:00:01.24] at System.Net.HttpEndPointManager.AddPrefixInternal(String p, HttpListener listener)
[xUnit.net 00:00:01.24] at System.Net.HttpEndPointManager.AddListener(HttpListener listener)
[xUnit.net 00:00:01.24] at System.Net.HttpListener.Start()
[xUnit.net 00:00:01.24] /home/runner/work/pact-net/pact-net/src/PactNet/Verifier/Messaging/MessagingProvider.cs(75,0): at PactNet.Verifier.Messaging.MessagingProvider.Start(JsonSerializerOptions settings)
[xUnit.net 00:00:01.24] Output:
[xUnit.net 00:00:01.24] Starting messaging provider at http://localhost:49152/pact-messages/
dbug: Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware[1]
Request matched endpoint 'Provider.Orders.OrdersController.GetByIdAsync (Provider)'
info: Microsoft.AspNetCore.Routing.EndpointMiddleware[0]
Executing endpoint 'Provider.Orders.OrdersController.GetByIdAsync (Provider)' |
Hello! Just wondering if there is any information on when this will be merged / released. Many thanks :) |
@YOU54F can you please let me know when this PR will be merged and released.? |
hey its beyond my control as i am the contributor not the maintainer |
It's been school holidays so I've been on vacation and I'm a bit behind. I'll review this properly tomorrow when I'm not on my phone, but my initial observations are:
|
we dont linux arm64 gh machines yet and dotnet wont run under qemu tiered targets are literally the premise of the rust. if we dont have ci, then a user cant raise a reproducible bug for that platform so it requires someone with that combo to reproduce. people can argue than it should be supported but if we cant test it, it gets tricky, so we cant ask for all the good stuff reproducers and the like. i dont get your argument. its open source software; no warranty implied or given |
hope you had a nice hol, yeah the csproj change looks huge but it’s actually quite small. we need to wrap a code block which shifts the change down and shows a large delta. happy to annotate or walk you through the changes. |
i was also to repro this against master and locally, but haven’t ascertained the cause yet |
In terms of the technical change itself, that looks OK, and thanks for linking into some of the MS detection stuff so we can be sure we're being consistent. It needs some tidying up but I'll do that afterwards. The two points above are still concerns though. Rust might have the concept of "tiered" support, but PactNet doesn't 😀 A user raising a bug still rightly wants their bug fixed. We can't really hide behind "sorry, it's open source". That doesn't stop me being tagged on issues and doesn't stop people repeatedly asking for updates. In terms of the CI, it appears GitLab Actions plans to support ARM runners (source) so we can add proper support there later. It's probably worth splitting this into two separate changes instead of trying to tackle both musl and ARM at the same time, because x64 musl and ARM currently have different implications for support and CI. ARM can be added later once GHA supports it (like they do with Mac ARM runners now) and once we have a good answer for how to run it locally to reproduce issues, but x64 musl shouldn't have to wait for ARM support to be mature. One additional thing that I'd like to see investigated first is what happens when you attempt to run on different versions of Alpine. We're just seeing Alpine 3.20 images starting to come through now, but 3.19 will be supported for a long time to come yet. For example, the official .Net images ship with both Alpine 3.19 and 3.20 variants at the moment. What happens if you try to use the library on 3.19 and 3.20? Do they both work or does one break? |
## Rationale pact-reference has introduced musl and arm64 based ffi libraries for linux - pact-foundation/pact-reference#416 Tracking Issue - pact-foundation/roadmap#30 ## Issues Resolved fixes pact-foundation#498 fixes pact-foundation#496 fixes pact-foundation#500 fixes pact-foundation#374 fixes pact-foundation#387 ## Backwards Compatibility Linux glibc based hosts take precedence, so if any error occurs during musl detection. I do not anticipate breaking changes for users ## Implementation notes ### .NET notes - Docs - [Uses MSBuild Exec task](https://learn.microsoft.com/en-us/visualstudio/msbuild/exec-task?view=vs-2022) - MSBuild Blog Posts - [Cross-Platform Build Events in .NET Core using MSBuild](https://jeremybytes.blogspot.com/2020/05/cross-platform-build-events-in-net-core.html) - [MSBuild 101: Using the exit code from a command](https://www.creepingcoder.com/2020/06/01/msbuild-101-using-the-exit-code-from-a-command/) - Stack OverFlow - [Set PropertyGroup property to Exec output](https://stackoverflow.com/questions/76583824/set-propertygroup-property-to-exec-output) - .NET runtime musl detection code - https://github.com/dotnet/runtime/blob/a50ba0669353893ca8ade8568b0a7d210b5a425f/src/mono/llvm/llvm-init.proj\#L7 - https://github.com/dotnet/runtime/blob/a50ba0669353893ca8ade8568b0a7d210b5a425f/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.Unix.cs\#L78t ### Conditions for execution musl detection will run if - if linux - if /lib/ld-musl-(x86_64|aarch64).so.1 exists - if ldd bin/sh | grep musl is true (musl lib is loaded, rather than glibc) will continue on error, reverting back to glibc based libaries. ### Supported musl targets should work for multiple musl based distroes if - /lib/ld-musl-(x86_64|aarch64).so.1 exists - ldd is available (available by default in alpine images) Tested on Alpine ARM64 / AMD64.
ce5e411
to
a41c667
Compare
Have split out linux-arm64 (libc) support into a separate PR. #521 Would you like x64 musl and arm64 musl changes split as well?
Fair shout, have added 3.19 / 3.20 variants of the alpine containers. Tags taken from https://github.com/dotnet/dotnet-docker/blob/main/README.sdk.md#linux-amd64-tags I've personally tested back to Alpine 3.15 with the shared library, although I imagine most users will be on much later versions |
Rationale
pact-reference has introduced musl and arm64 based ffi libraries for linux
Tracking Issue
Issues Resolved
fixes #498
fixes #496
fixes #500
fixes #374
fixes #387
Backwards Compatibility
Linux glibc based hosts take precedence, so if any error occurs during musl detection. I do not anticipate breaking changes for users
Implementation notes
.NET notes
Conditions for execution
musl detection will run if
will continue on error, reverting back to glibc based libaries.
Supported musl targets
should work for multiple musl based distroes if
Tested on Alpine ARM64 / AMD64.
Caveats
DOTNET_NUGET_SIGNATURE_VERIFICATION
tofalse
Compatibility
Operating System
Due to using a shared native library instead of C# for the main Pact logic only certain OSs are supported:
Support