-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
run writingsystem tests against dotnet 8 #1336
Conversation
… add a test to exhibit an issue with icu.net and SIL.WritingSystems when using net8.0
…hecking for "Unknown Language" in the name. Fixes a number of tests that were failing due to incorrectly detecting unknown cultures.
…ver specified and expect NLS (Windows only globalization) to be used instead of ICU.
# Conflicts: # SIL.Core/Reflection/ReflectionHelper.cs
…-writingsystems-dotnet-8 # Conflicts: # SIL.WritingSystems.Tests/SIL.WritingSystems.Tests.csproj
It looks like a lot of our WritingSystem tests (especially related to sorting) are not expecting dotnet 8 to use ICU natively. To resolve this I went ahead and used a configuration flag so that instead of dotnet using ICU it will use NLS which is what .Net Framework uses, this meets the expectations of our tests. That said many of these tests may be over specified, so we should consider if we want to change them or not. |
…tests. Update version of icu.net used by SIL.Windows.Forms.Keyboarding to 3.0.0-beta.296
So this behaves correctly on Windows 8, and Windows 7? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 8 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)
Previously, jasonleenaylor (Jason Naylor) wrote…
from my testing on windows 7 the CultureInfo constructor throws if you try to use an unknown culture, so yeah this will work on windows 7 I suppose since you can't have an invalid instance. |
@ermshiperete can you see why appveyor is failing? The error looks like it's trying to use dotnet 5, however according to https://www.appveyor.com/docs/windows-images-software/ the Visual Studio 2019 image has dotnet 8 installed, and I can't see anywhere that we've hardcoded dotnet 5 in our build scripts. Addtionally using msbuild on Palaso.proj works fine locally, even when I set CI to true. |
|
Ditto. Their docs say that the Visual Studio 2019 image includes the .NET 8 SDK, but in practice I've found that that's false and you need the 2022 image in order for .NET 8 to work. See sillsdev/flexbridge#407 for example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hahn-kev)
SIL.WritingSystems/CultureInfoExtensions.cs
line 15 at r3 (raw file):
Previously, hahn-kev (Kevin Hahn) wrote…
from my testing on windows 7 the CultureInfo constructor throws if you try to use an unknown culture, so yeah this will work on windows 7 I suppose since you can't have an invalid instance.
We did the same thing [in L10nSharp], for reference (sillsdev/l10nsharp#118)
There's a bug when using SIL.WritingSystems from a dotnet 8 app, but only when you explicitly reference icu.net from that app.
This PR adds a test to expose that issue, while also fixing it by updating the icu.net version. Since this is a dotnet 8 specific issue it also runs the SIL.WritingSystems tests against dotnet 8
Issue explanation: sillsdev/icu-dotnet#202
this replaces PR #1330 which was made on a fork
This change is