Open YOU54F opened 6 months ago
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:
I'm not keen on the idea of different CI being defined for different platforms instead of a single CI definition. Things like the pass/fail annotation on GitHub commits are important, and if that has a tick but the other CI is failing then that's really bad. That's not to mention the maintenance burden.
I'm not comfortable introducing the entire new concept of "tiered" support. I think something is either supported or not, and users will quite rightly not accept the response of "oh sorry, that's tier 2 so it's broken" as if that in any way changes the response they'd expect to their bug report.
the delta on the csproj is big and appears to have embedded scripting...? That needs very careful consideration during a proper review.
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.
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)'
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?
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
Have split out linux-arm64 (libc) support into a separate PR. #521
Would you like x64 musl and arm64 musl changes split as well?
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?
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