-
Notifications
You must be signed in to change notification settings - Fork 386
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
Add NativeAOT test #1656
Add NativeAOT test #1656
Conversation
src/System.CommandLine/Binding/ArgumentConverter.DefaultValues.cs
Outdated
Show resolved
Hide resolved
src/System.CommandLine/Binding/ArgumentConverter.DefaultValues.cs
Outdated
Show resolved
Hide resolved
@jonsequitur the Ubuntu leg is most likely failing due to lack of native dependencies: https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/docs/prerequisites.md Should we add these dependencies or disable the test for this OS? |
@jonsequitur there are other issues that we need to consider:
|
src/System.CommandLine.Tests/TestApps/NativeAOT/NativeAOT.csproj
Outdated
Show resolved
Hide resolved
src/System.CommandLine/Binding/ArgumentConverter.DefaultValues.cs
Outdated
Show resolved
Hide resolved
I'd prefer to add the dependencies. I'll look into it. |
We've been using
My thought is the hardcoded version to prevent regression is simpler and more important, since here we're testing System.CommandLine and not the AOT compiler. Ingesting new dependencies dynamically can break this CI pipeline, so I would tend to avoid that. |
Co-authored-by: Eric Erhardt <[email protected]>
@adamsitnik The update to Ubuntu 20.04 is merged. Could you rebase this and see if the tests now pass? |
@jonsequitur I've rebased the branch but the test is till failing on Ubuntu. Is this repo using CI machines from MS internal feed? Or some public ones and we can define what is needed? This is what we needed in BDN: |
Do you want to try including that in your PR? |
@jonsequitur it's finally green! |
There was one warning, about the usage of
Array.CreateInstance
. IMO we can't get rid of it until we implement #1638I've disabled this particular warning just to make sure we don't add any new warnings in the meantime. It's not perfect, but still better than no tests at all.