Migrate to supported .NET Core versions #787

wieslawsoltes · 2021-01-08T08:40:55Z

Reference Issue

What does this implement/fix? Explain your changes.

Migrates to supported .NET Core versions:
- from .NET Core 2.2 (Out of support version) to .NET Core 2.1
- from .NET Core 3.0 (Out of support version) to .NET Core 3.1
Adds support for .NET 5.0 as it's is currently recommended .NET SDK

.NET Core 2.1 and 3.1 are LTS (Long-term support) releases, .NET 5.0 is basically basically new version of netstandard.

https://dotnet.microsoft.com/download/dotnet-core

Any other comments?

wieslawsoltes · 2021-01-08T08:47:41Z

I think adding net5.0 target would also be good, we are already using it on AppVeyor for building and Source Generators require new compiler toolchain so this would not introduce any new dependency.

wieslawsoltes · 2021-01-08T08:48:22Z

I will fix later samples, there are some breaking changes in netcoreapp3.1 and net5.0-windows regarding Windows Forms.

https://docs.microsoft.com/en-us/dotnet/core/compatibility/winforms

This reverts commit b7f8d87.

mrbean-bremen

Looks good to me, and makes sense in my opinion, though I would like to hear from @tebjan about this (regarding removing support for unsupported versions).
I got an initial GH Actions workflow running based on this branch, though one test is failing both in .NetCore 3.1 and .Net 5 (__issue-084-01 comparison test). I have not looked into this, but would probably handle this in a separate issue, if this is not a false positive (which happens with the comparison tests).

wieslawsoltes · 2021-01-09T12:33:58Z

Looks good to me, and makes sense in my opinion, though I would like to hear from @tebjan about this (regarding removing support for unsupported versions).

Well I am and moving netcoreapp2.2 to netcoreapp2.1 so it's is event better, netcoreapp3.0 to netcoreapp3.1 is not that bad as we have netcoreapp2.1, so I think in terms of compatibility we are pretty good. The net5.0 is pretty important as it is becoming new standard target.

I got an initial GH Actions workflow running based on this branch, though one test is failing both in .NetCore 3.1 and .Net 5 (__issue-084-01 comparison test). I have not looked into this, but would probably handle this in a separate issue, if this is not a false positive (which happens with the comparison tests).

I have checked the test (quickly) and it seems that issue is with saving and reloading the svg, not sure what is wrong, maybe I need to update System.Drawing package.

mrbean-bremen · 2021-01-09T12:38:20Z

I think it makes sense for @tebjan to make a release with the old supported versions, and merge this afterwards. I would make a PR for GitHub actions after this is merged (with the current version, the tests for .NetCore 2.2 fail because it is not supported). As for the failing test - if you don't find the problem right away, I would create a new issue for this and temproraily remove this image from the tests. It still works with the older versions, so this is not a regression.

wieslawsoltes · 2021-01-09T12:43:16Z

I think it makes sense for @tebjan to make a release with the old supported versions, and merge this afterwards.

That's fine with me.

I would make a PR for GitHub actions after this is merged (with the current version, the tests for .NetCore 2.2 fail because it is not supported).

You need to install:

.NET Core sdk 2.2.207
.NET Core sdk 3.0.103
to make it work.

As for the failing test - if you don't find the problem right away, I would create a new issue for this and temproraily remove this image from the tests. It still works with the older versions, so this is not a regression.

The new issue would be good.

wieslawsoltes · 2021-01-09T13:09:38Z

Testing the failing test:

       [Test]
        public void Test_Issue_084_01()
        {
            var svgPath = Path.Combine(AssemblyDirectory, "..", "..", "..", "..", "W3CTestSuite", "svg");
            var file = Directory.GetFiles(svgPath, "__issue-084-01.svg").FirstOrDefault(); 
            var doc1 = SvgDocument.Open<SvgDocument>(file);

            using (var memStream = new MemoryStream())
            {
                var targetFrameworkAttribute = Assembly.GetExecutingAssembly()
                    .GetCustomAttributes(typeof(System.Runtime.Versioning.TargetFrameworkAttribute), false)
                    .SingleOrDefault() as System.Runtime.Versioning.TargetFrameworkAttribute;
                
                
                doc1.Write(memStream);
                memStream.Position = 0;

                var streamReader = new StreamReader(memStream);
                var str = streamReader.ReadToEnd();
                File.WriteAllText($"c:\\DOWNLOADS\\__issue-084-01_{targetFrameworkAttribute.FrameworkName}.svg", str);

                memStream.Position = 0;

                var baseUri = doc1.BaseUri;
                var doc2 = SvgDocument.Open<SvgDocument>(memStream);
                doc2.BaseUri = baseUri;
            }

__issue-084-01.zip

wieslawsoltes · 2021-01-09T13:12:20Z

Looks like the .NETCoreApp,Version=v3.1 and .NETCoreApp,Version=v5.0 produce different floating points for some reason.

wieslawsoltes · 2021-01-09T13:17:58Z

Seems to be related https://devblogs.microsoft.com/dotnet/floating-point-parsing-and-formatting-improvements-in-net-core-3-0/

mrbean-bremen · 2021-01-09T14:01:25Z

__issue-084-01.zip

Cannot download this file, as it supposedly contains a virus.

wieslawsoltes · 2021-01-09T14:10:35Z

__issue-084-01.zip

Cannot download this file, as it supposedly contains a virus.

It contains only svg's.

__issue-084-01.svg.txt
_issue-084-01.NETCoreApp,Version=v2.1.svg.txt
_issue-084-01.NETCoreApp,Version=v3.1.svg.txt
_issue-084-01.NETCoreApp,Version=v5.0.svg.txt
_issue-084-01.NETFramework,Version=v4.5.2.svg.txt
_issue-084-01.NETFramework,Version=v4.6.1.svg.txt

wieslawsoltes · 2021-01-09T14:36:05Z

@mrbean-bremen I will add float extension method for ToString, so we have one place for all conversion from float to string and can control output.

Source/DataTypes/SvgViewBox.cs

mrbean-bremen · 2021-01-09T20:41:02Z

Here is the test result for this branch (with the failing test removed, Windows only).

wieslawsoltes · 2021-01-09T20:55:47Z

Here is the test result for this branch (with the failing test removed, Windows only).

This is great, I think I have smaller repro for failing issue.

mrbean-bremen · 2021-01-10T17:38:40Z

Do you think it makes sense to add all or some benchmark tests to the CI build? Currently only the unittests are run.

wieslawsoltes · 2021-01-10T17:48:04Z

Do you think it makes sense to add all or some benchmark tests to the CI build? Currently only the unittests are run.

I don't think they will be comparable, probably by limited CI machine resources and will increase the build times.

Maybe we can add separate job only for benchmarks, the cpu performance will vary but memory usage can be useful.

mrbean-bremen · 2021-01-10T17:49:20Z

Maybe we can add separate job only for benchmarks

Yes, this is what I have been thinking.

wieslawsoltes · 2021-01-10T17:50:43Z

Do you think it makes sense to add all or some benchmark tests to the CI build? Currently only the unittests are run.

I don't think they will be comparable, probably by limited CI machine resources and will increase the build times.

Maybe we can add separate job only for benchmarks, the cpu performance will vary but memory usage can be useful.

I would start we this benchmark:

dotnet run -c Release -f netcoreapp3.1 ./Tests/Svg.Benchmark/Svg.Benchmark.csproj -- -f '*SvgDocument_*'

mrbean-bremen · 2021-01-10T18:24:06Z

Yeah, this increases the build times a bit, but that is still acceptable. Could further parallelize the tests, if needed, and probably add an option to show less output.

wieslawsoltes · 2021-01-10T18:35:18Z

Yeah, this increases the build times a bit, but that is still acceptable. Could further parallize the tests, if needed, and probably add an option to show less output.

Yes that looks acceptable.

mrbean-bremen · 2021-01-12T17:32:32Z

@wieslawsoltes - I'm going to merge this now, if you don't object - the release is out!

wieslawsoltes · 2021-01-12T17:35:12Z

@wieslawsoltes - I'm going to merge this now, if you don't object - the release is out!

@mrbean-bremen No objections, ready to merge.

Migrate to supported .NET Core versions

905b53d

wieslawsoltes and others added 16 commits January 8, 2021 08:49

Fix compiler warnings

ed32d5f

Add support for .NET 5.0

fc6f09b

Remove no longer needed file

17928ab

Remove no longer needed file

7d887af

Use new version of .NET Core SDK for Windows Forms apps

b2b01f1

Use ContextMenuStrip

720ccc0

Use Microsoft.NET.Sdk.WindowsDesktop

2966080

Fix menu

9706567

Update unit test packages

0f1416f

Update ImageComparisonTest.cs

b7f8d87

Revert "Update ImageComparisonTest.cs"

f4c933c

This reverts commit b7f8d87.

ViewBox is never null

19a6963

Check for null

91c88a6

The size is never null

59225c5

The values are never null

5cced07

Use non obsolate property

0853cae

mrbean-bremen approved these changes Jan 9, 2021

View reviewed changes

Update System.Drawing.Common to latest version

e5327d0

mrbean-bremen mentioned this pull request Jan 9, 2021

Failing test for .NET Core 3.1 and .NET 5 #789

Open

wieslawsoltes added 2 commits January 9, 2021 15:47

Added ToSvgString extension method for float

94491c0

Use G7 format specifier to be compatible across all target frameworks.

c871d1f

mrbean-bremen reviewed Jan 9, 2021

View reviewed changes

Source/DataTypes/SvgViewBox.cs Show resolved Hide resolved

wieslawsoltes added 2 commits January 9, 2021 21:15

Added ToStringBenchmarks

8832f99

Add netcoreapp3.1 targte for test runner

6783648

wieslawsoltes added 4 commits January 9, 2021 22:04

Repro for issue svg-net#789

a4929e7

Smaller repro for svg-net#789

9902432

Add smaller repro for svg-net#789

ab81444

Update __issue-789-03.svg

71bda97

Merge branch 'master' into MigrateToSupportedDotNetCoreVersions

0e3cc18

mrbean-bremen merged commit 58424cc into svg-net:master Jan 12, 2021

wieslawsoltes deleted the MigrateToSupportedDotNetCoreVersions branch January 14, 2021 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to supported .NET Core versions #787

Migrate to supported .NET Core versions #787

wieslawsoltes commented Jan 8, 2021 •

edited

Loading

wieslawsoltes commented Jan 8, 2021

wieslawsoltes commented Jan 8, 2021 •

edited

Loading

mrbean-bremen left a comment •

edited

Loading

wieslawsoltes commented Jan 9, 2021

mrbean-bremen commented Jan 9, 2021

wieslawsoltes commented Jan 9, 2021

wieslawsoltes commented Jan 9, 2021

wieslawsoltes commented Jan 9, 2021 •

edited

Loading

wieslawsoltes commented Jan 9, 2021

mrbean-bremen commented Jan 9, 2021

wieslawsoltes commented Jan 9, 2021

wieslawsoltes commented Jan 9, 2021

mrbean-bremen commented Jan 9, 2021 •

edited

Loading

wieslawsoltes commented Jan 9, 2021

mrbean-bremen commented Jan 10, 2021

wieslawsoltes commented Jan 10, 2021

mrbean-bremen commented Jan 10, 2021

wieslawsoltes commented Jan 10, 2021 •

edited

Loading

mrbean-bremen commented Jan 10, 2021 •

edited

Loading

wieslawsoltes commented Jan 10, 2021

mrbean-bremen commented Jan 12, 2021

wieslawsoltes commented Jan 12, 2021

Migrate to supported .NET Core versions #787

Migrate to supported .NET Core versions #787

Conversation

wieslawsoltes commented Jan 8, 2021 • edited Loading

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

wieslawsoltes commented Jan 8, 2021

wieslawsoltes commented Jan 8, 2021 • edited Loading

mrbean-bremen left a comment • edited Loading

Choose a reason for hiding this comment

wieslawsoltes commented Jan 9, 2021

mrbean-bremen commented Jan 9, 2021

wieslawsoltes commented Jan 9, 2021

wieslawsoltes commented Jan 9, 2021

wieslawsoltes commented Jan 9, 2021 • edited Loading

wieslawsoltes commented Jan 9, 2021

mrbean-bremen commented Jan 9, 2021

wieslawsoltes commented Jan 9, 2021

wieslawsoltes commented Jan 9, 2021

mrbean-bremen commented Jan 9, 2021 • edited Loading

wieslawsoltes commented Jan 9, 2021

mrbean-bremen commented Jan 10, 2021

wieslawsoltes commented Jan 10, 2021

mrbean-bremen commented Jan 10, 2021

wieslawsoltes commented Jan 10, 2021 • edited Loading

mrbean-bremen commented Jan 10, 2021 • edited Loading

wieslawsoltes commented Jan 10, 2021

mrbean-bremen commented Jan 12, 2021

wieslawsoltes commented Jan 12, 2021

wieslawsoltes commented Jan 8, 2021 •

edited

Loading

wieslawsoltes commented Jan 8, 2021 •

edited

Loading

mrbean-bremen left a comment •

edited

Loading

wieslawsoltes commented Jan 9, 2021 •

edited

Loading

mrbean-bremen commented Jan 9, 2021 •

edited

Loading

wieslawsoltes commented Jan 10, 2021 •

edited

Loading

mrbean-bremen commented Jan 10, 2021 •

edited

Loading