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

Update to .net core 3.1. #3571

Merged
merged 13 commits into from
Mar 6, 2020
Merged

Update to .net core 3.1. #3571

merged 13 commits into from
Mar 6, 2020

Conversation

grokys
Copy link
Member

@grokys grokys commented Feb 14, 2020

What does the pull request do?

  • Update sample apps to netcoreapp3.1
  • Remove netfx targets from samples as they seem to interfere somehow with net core targets
  • Pin SDK in global.json.

@grokys grokys requested review from kekekeks and a team February 14, 2020 14:03
@MarchingCube
Copy link
Collaborator

Maybe we can bump benchmarks as well? Since I was always doing that manually anyway.

@MarchingCube
Copy link
Collaborator

It also would be great to multi target .net standard 2.1 as well in the main projects.

@grokys
Copy link
Member Author

grokys commented Feb 14, 2020

Ah yeah, I also need to bump unit tests.

Not sure about dual-targeting .net standard 2.1, what does everyone think? Is it likely to cause us any problems?

@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFrameworks>net461;netcoreapp2.0</TargetFrameworks>
<TargetFrameworks>net461;netcoreapp3.1</TargetFrameworks>
Copy link
Member Author

Choose a reason for hiding this comment

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

Not 100% sure we want to change this?

Copy link
Member

Choose a reason for hiding this comment

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

Host app TFM should match the TFM of Avalonia.DesktopRuntime. Which is not changed by this PR.

@grokys
Copy link
Member Author

grokys commented Feb 14, 2020

Also /Packages/Avalonia/Avalonia.csproj multi-targets netstandard2.0;net461;netcoreapp2.0 - not sure why it does this, do we want to change that to 3.1 too?

@grokys
Copy link
Member Author

grokys commented Feb 14, 2020

Ok, seems nuke is failing on .NET core 3.1, looks like we're going to have to upgrade it, but there have been a lot of breaking changes between 0.12 and 0.24, so this looks like it's not going to be as straightforward as I'd hoped. Marking this PR WIP for now.

@grokys grokys changed the title Update to .net core 3.1. WIP: Update to .net core 3.1. Feb 14, 2020
@kekekeks
Copy link
Member

Could we use both .NET Core 3.1 and the oldest supported one instead? We are targeting netcoreapp2.0 TFM anyway.

@grokys
Copy link
Member Author

grokys commented Feb 14, 2020

Ah yeah, i meant to change Avalonia.DesktopRuntime to 3.1 as well. Is there any reason to target 2.0 still?

@kekekeks
Copy link
Member

OSX 10.12 is not supported by .NET Core 3.1. Not sure if we care about that.

@MarchingCube
Copy link
Collaborator

@kekekeks It looks like only 7% people use macOS 10.12 at the moment and it is shrinking (according to https://gs.statcounter.com/macos-version-market-share/desktop/worldwide).

@grokys It would be good to figure out .Net Standard 2.1 support soon, since it is blocking certain work (like improving our string tokenizers and providing more span based optimizations).

@Gillibald
Copy link
Contributor

What exactly will perform better with the tokenizer under. Net Standard 2.1?

@MarchingCube
Copy link
Collaborator

@Gillibald Almost everything :) Since it currently works on substrings https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Base/Utilities/StringTokenizer.cs#L43. And .Net Standard 2.1 adds ReadOnlySpan<T> based overloads for parsing, so we could do tokenization without allocations.

https://apisof.net/catalog/System.Double.TryParse(ReadOnlySpan%3CChar%3E,NumberStyles,IFormatProvider,Double)

@jmacato
Copy link
Member

jmacato commented Feb 15, 2020

It'd be great if we could move all our targets to netstandard2.1 and netcoreapp3.1. so many perf stuff in it as @MarchingCube said.

@kekekeks
Copy link
Member

kekekeks commented Feb 15, 2020

netstandard2.1 is not supported by net4xx TFMs. Which makes it impossible to run our unit tests on Mono. That would be a road blocker for mobile platform support 👎

We can add netstandard2.1 if and only if we'll be multitargeting netstandard2.0 as well with polyfilling missing APIs.

@jkoritzinsky
Copy link
Collaborator

IIRC Mono does support netstandard2.1 but there’s just no good way with the standard tooling to do it.

@kekekeks
Copy link
Member

@jkoritzinsky That's simply because there is no desktop Mono target framework moniker. It has to reuse net4xx while being a superset of that API.

@kekekeks
Copy link
Member

Also, keep in mind, that while .NET 5 will have Mono as a runtime option, Xamarin platforms aren't migrating from the full mono codebase anytime soon.

@jkoritzinsky
Copy link
Collaborator

I don’t know if you saw the new TFM design spec for .NET 5, but Xamarin is planning on moving to a .NET 5 based TFM if all goes according to plan.

@kekekeks
Copy link
Member

If I understand correctly, the BCL will still be based on mono/mcs/class rather than one from dotnet/runtime repo.

@jkoritzinsky
Copy link
Collaborator

Now that I’m not sure about. But considering a very significant portion of the BCL is shared, I wouldn’t worry about BCL differences being our problem. Our Mono-specific issues have almost always been runtime issues around either bugs or underdefined behavior.

@kekekeks
Copy link
Member

Also, keep in mind that since we'd be shipping our NativeControlHost in the next release, embedding WPF/WinForms controls into Avalonia is now possible. With existing Avalonia into WPF/WinForms embedding, the drop of net4xx TFM support effectively removes a rather painless way of migrating existing applications, people will have to update their existing WPF code first instead of migrating to Avalonia directly.

@jkoritzinsky
Copy link
Collaborator

I think we should do the following:

Make Avalonia.dll a ref assembly for all of Avalonia that targets netstandard2.0 with its implementation assembly just type-forwarding. Make the rest of the assemblies implementation-only and multitargetting. That way we can have a unified API surface on all platforms, have “public” types we don’t expose to users, and use platform-specific APIs under the hood for performance.

@kekekeks
Copy link
Member

Make the rest of the assemblies implementation-only and multitargetting.

That would conflict with how AppBuilder currently works. Right now it's only provided by netcoreappXX and net4XX TFMs. On mobile it's provided by backend-specific Avalonia.iOS and Avalonia.Android packages. It's not provided by netstandard2.0 dll.

@jkoritzinsky
Copy link
Collaborator

AppBuilder is in the Avalonia.Desktop package right? I was referring to the stuff in the Avalonia package only. Sorry about not clarifying that.

@kekekeks
Copy link
Member

kekekeks commented Feb 15, 2020

AppBuilder is in Avalonia package itself. The only thing that's provided by Avalonia.Desktop is UsePlatformDetect().

@kekekeks
Copy link
Member

kekekeks commented Feb 15, 2020

For now I'd suggest to write the list of things that we need from netstandard2.1 and see if those could be polyfilled. It's not like we need Span overloads for System.IO.Stream that can't be implemented outside of BCL.

@ahopper
Copy link
Contributor

ahopper commented Feb 15, 2020

I did try using the span parse overloads on Geometry.Parse, this did reduce the number of allocations but actually had only a small speed increase, the underlying double parsing code is slow and dominates, probably because it has to allow for every possible formatting option. There are possibly bigger gains by using custom number span parsing (compatible with net standard 2.0) or better still do the parsing in xamlil at build time.

@grokys
Copy link
Member Author

grokys commented Feb 15, 2020

Could we just #if stuff depending on the target framework? Or would that be too much of a hassle?

@jkoritzinsky
Copy link
Collaborator

I’m fine with using #if per TFM. I want the ref assembly though to ensure that our public surface area is consistent.

@kekekeks
Copy link
Member

kekekeks commented Feb 16, 2020

the underlying double parsing code is slow and dominates, probably because it has to allow for every possible formatting option

I guess we should just use some specialized parser.

Could we just #if stuff depending on the target framework? Or would that be too much of a hassle?

As long as it's something like:

int Foo(int bar)
{
#if NETSTANDARD2.1
      return System.Something.Foo(bar);
#else
      //Polyfill implementation here
#endif

}

Having separate code paths would be a maintenance burden

@matkoch
Copy link
Contributor

matkoch commented Feb 18, 2020

@grokys @kekekeks please let me know if you need any help with updating the build.

@grokys
Copy link
Member Author

grokys commented Feb 18, 2020

Thanks @matkoch! Am I right in thinking that nuke 0.12 doesn't support .net core 3.1?

@matkoch
Copy link
Contributor

matkoch commented Feb 19, 2020 via email

@kekekeks
Copy link
Member

I'd suggest to leave the Nuke-based project as is and update it in a separate PR.

@grokys grokys mentioned this pull request Mar 3, 2020
@grokys
Copy link
Member Author

grokys commented Mar 3, 2020

I've merged in #3596 (which updates the nuke build to 0.24) and CI now passes. Unmarking as WIP.

@grokys grokys changed the title WIP: Update to .net core 3.1. Update to .net core 3.1. Mar 3, 2020
Copy link
Collaborator

@MarchingCube MarchingCube left a comment

Choose a reason for hiding this comment

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

LGTM. Unless anybody has objections - we should merge this in and then deal with consequences if any arise :)

@MarchingCube MarchingCube merged commit a65a64a into master Mar 6, 2020
@MarchingCube MarchingCube deleted the update/net31 branch March 6, 2020 20:54
danwalmsley pushed a commit that referenced this pull request Apr 16, 2020
Update to .net core 3.1.
# Conflicts:
#	azure-pipelines.yml
#	global.json
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.

8 participants