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

remove netstandard1.6 replace with netstandard2.0 #202

Merged
merged 3 commits into from
Aug 1, 2024

Conversation

hahn-kev
Copy link
Collaborator

@hahn-kev hahn-kev commented Jul 31, 2024

This PR attempts to remove support for netstanard1.6 as this requires a custom SortKey class which triggers down stream issues with a number of projects. The main cause of this is as follows:
SIL.WritingSystems netstandard2.0 version uses the netstandard1.6 version of this library, this means SIL.WritingSystems includes references to Icu.SortKey (here). When an App (or Tests) targets net8.0 and adds an explicit reference to icu.net (required for the config file), the app gets the net8.0 version of ICU, this version does not include the Icu.SortKey class. This means when trying to call IcuRulesCollator.GetSortKey you will get the following error at runtime:

  Error Message:
   System.TypeLoadException : Could not load type 'Icu.SortKey' from assembly 'icu.net, Version=2.10.0.0, Culture=neutral, PublicKeyToken=416fdd914afa6b66'.
  Stack Trace:
     at SIL.WritingSystems.IcuRulesCollator.GetSortKey(String source)
   at SIL.WritingSystems.Tests.IcuRulesCollationDefinitionTests.Collation_GetSortKeyWorks() in C:\dev\libpalaso\SIL.WritingSystems.Tests\IcuRulesCollationDefinitionTests.cs:line 96
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

This could probably be solved by always including SortKey, however I don't see the need to support netstandard1.6. netstandard2.0 drops support for .net core 1.0 and 1.1, Mono 4.6, and a couple old Xamarin versions. See table here

This change is related to sillsdev/libpalaso#1330 and sillsdev/liblcm#306

…ove related conditional compilation. Should fix issues where a library is compiled against netstandard2.0 but the consumer is a net8.0 project which results in the runtime error: Could not load type 'Icu.SortKey' from assembly 'icu.net'
Copy link

Test Results

       5 files  ±0     403 suites  ±0   5s ⏱️ -1s
   431 tests ±0     427 ✔️ ±0    4 💤 ±0  0 ±0 
2 193 runs  ±0  2 095 ✔️ ±0  98 💤 ±0  0 ±0 

Results for commit 63cbe25. ± Comparison against base commit b7b0666.

Copy link
Member

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. Also needs a line in CHANGELOG.md (I guess this is a breaking change).

source/icu.net/SafeEnumeratorHandle.cs Show resolved Hide resolved
@hahn-kev hahn-kev requested a review from ermshiperete August 1, 2024 01:34
@hahn-kev
Copy link
Collaborator Author

hahn-kev commented Aug 1, 2024

@ermshiperete I added a changelog summary and removed the ReliabilityContract attributes from non framework builds.

@hahn-kev hahn-kev merged commit 58ce4f3 into master Aug 1, 2024
7 checks passed
@hahn-kev hahn-kev deleted the remove-netstandard1.6 branch August 1, 2024 09:10
Copy link
Member

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

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

Since this is a breaking change, the commit message needs a line +semver:major so that the major version number gets bumped.

@@ -16,6 +16,20 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

### Breaking
Copy link
Member

@ermshiperete ermshiperete Aug 1, 2024

Choose a reason for hiding this comment

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

(After the fact: this should be ### Removed)

@ermshiperete
Copy link
Member

See #203 for follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants