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

[FEATURE] Make the Views.WindowsForms and Views.WPF actually target .NET Core #1029

Closed
3 tasks done
mattleibow opened this issue Nov 22, 2019 · 17 comments
Closed
3 tasks done
Milestone

Comments

@mattleibow
Copy link
Contributor

mattleibow commented Nov 22, 2019

Is your feature request related to a problem? Please describe.
Right now, installing the SkiaSharp.Views.WindowsForms or SkiaSharp.Views.WPF packages will give a warning saying that it is not targeting the right thing and all that.

  • SkiaSharp.Views.Desktop.Common
  • SkiaSharp.Views.WPF
  • SkiaSharp.Views.WindowsForms

Describe the solution you'd like
We would like a nice package that targets both net45 and netcoreapp

Describe alternatives you've considered
We can use the NoWarn feature, but that is extra work that is unnecessary.

@TysonMN
Copy link
Contributor

TysonMN commented Dec 17, 2019

I might be able to help get dual targeting working for View.WPF.


Currently, View.WPF references NuGet packages OpenTK and OpenTK.GLControl, but the project still compiles if they are removed.

Are these references needed? Are they only runtime dependencies on them?


(After removing references to OpenTK and OpenTK.GLControl...)

I can get View.WPF to build without any warnings when targeting either net45 or netcoreapp3.0 but not when targeting both. I could share more details about this, but there is another possibility.

Instead of targeting net45 and netcoreapp3.0, what about targeting net472 and netcoreapp3.0?

This makes things easier, because Views.Desktop.Common.NetStandard targets netstandard2.0, so both net472 and netcoreapp3.0 can target that (source).


Even after removing the references to OpenTK and OpenTK.GLControl and dual targeting net472 and netcoreapp3.0, I still had a problem (copying a pdb file). I can share more details about this as well if this is the direction that should be taken.

@TysonMN
Copy link
Contributor

TysonMN commented Dec 17, 2019

I can get View.WPF to build without any warnings when targeting either net45 or netcoreapp3.0 but not when targeting both. I could share more details about this...

Quoting from #1011 (comment)

Because during compilation it took the dll from SkiaSharp.Views.Forms instead of SkiaSharp.Views.Forms.WPF. I think the error is that SkiaSharp.Views.Forms.WPF has a dependency on SkiaSharp.Views.Forms. (which it shouldn't have because both SkiaSharp.Views.Forms.WPF and SkiaSharp.Views.Forms provide SkiaSharp.Views.Forms.dll). And nuget decides in my case to use the SkiaSharp.Views.Forms nuget package dll which does not work on WPF.

I think this describes a problem I had as well. Both SkiaSharp.Desktop and SkiaSharp.NetStandard produce a DLL named SkiaSharp.dll. Then when dual targeting, the target that comes second in the list fails to find the appropriate SkiaSharp.dll.

@mattleibow mattleibow changed the title [FEATURE] Make the Views.WindowsForms and Views.WPF atually target .NET Core [FEATURE] Make the Views.WindowsForms and Views.WPF actually target .NET Core Dec 17, 2019
@mattleibow
Copy link
Contributor Author

I might be able to help get dual targeting working for View.WPF.

THANKS!


It could also be an issue with all the old versions of things used in the master branch. Try using the dev/vs-2019 branch. I will be merged that in soon and it makes life much easier. I think the multi-targeting is broken a bit with the old .net core 2.1

@TysonMN
Copy link
Contributor

TysonMN commented Dec 18, 2019

Both SkiaSharp.Desktop and SkiaSharp.NetStandard produce a DLL named SkiaSharp.dll. Then when dual targeting, the target that comes second in the list fails to find the appropriate SkiaSharp.dll.

...Try using the dev/vs-2019 branch. ...

It is better. That problem with SkiaSharp.dll is gone, but for simplicity before, I didn't mention that a similar problem exists where SkiaSharp.Views.Desktop.Common and SkiaSharp.Views.Desktop.Common.NetStandard both produce a DLL named SkiaSharp.Views.Desktop.Common.dll.

I think I can combine SkiaSharp.Views.Desktop.Common and SkiaSharp.Views.Desktop.Common.NetStandard into one project called SkiaSharp.Views.Desktop.Common that multitargets netstandard2.0, net45, and netcoreapp3.0.

Does that sounds like a good idea?

mattleibow pushed a commit that referenced this issue Dec 19, 2019
* Views.Desktop.Common now multitargets Standard 2.0, Framework 4.5, and Core 3.0 #1029
* Removed Views.Desktop.Common.NetStandard
@mattleibow
Copy link
Contributor Author

Does that sounds like a good idea?

It does :)

PR merged.

@TysonMN
Copy link
Contributor

TysonMN commented Dec 19, 2019

Currently View.WPF references NuGet packages OpenTK and OpenTK.GLControl, but the project still compiles if they are removed.

Are these references needed? Are they only runtime dependencies on them?

I think these OpenTK dependencies are now the only obstacle preventing View.WPF from targeting .NET Core.

Does anyone know anything about these dependencies?

@mattleibow
Copy link
Contributor Author

Maybe the references can be removed for WPF.

But, I see the OpenTK is built for net20, so even if the references are not "official" .net core, it should work...

But, for WPF, if we aren't using it, then we can remove it.

@mattleibow
Copy link
Contributor Author

mattleibow commented Dec 19, 2019

What is the actual reason you can't target netcoreapp with a reference to OpenTK? Is it a compiler error? Runtime error? I know I have a sample netforeapp3.0 that uses the WPF views as is.

@TysonMN
Copy link
Contributor

TysonMN commented Dec 19, 2019

But, I see the OpenTK is built for net20, so even if the references are not "official" .net core, it should work...
...
What is the actual reason you can't target netcoreapp with a reference to OpenTK? Is it a compiler error? Runtime error? I know I have a sample netcoreapp3.0 that uses the WPF views as is.

My concern is just these warnings.

'OpenTK 3.0.1' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8' instead of the project target framework '.NETCoreApp,Version=v3.0'. This package may not be fully compatible with your project.
'OpenTK.GLControl 3.0.1' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8' instead of the project target framework '.NETCoreApp,Version=v3.0'. This package may not be fully compatible with your project.

I also noticed that these packages target .NET Framework 2.0. It does seem like a .NET Core 3.0 application should be able to utilize .NET Framework 2.0 dependencies (based on this table).

Maybe the references can be removed for WPF.
...
...if we aren't using it, then we can remove it.

They aren't being used at compile time. That much is for sure. Sometimes though a NuGet package needs to be included because of a runtime-only dependency. Maybe that is the case here. I don't know.

Even if the compatibility isn't perfect or these dependencies can be removed, adding the dual target seems better than the status quo. I will make a PR soon.

P.S.
The next major version of OpenTK will target .NET Standard 2.0 opentk/opentk#823

@TysonMN TysonMN mentioned this issue Dec 19, 2019
4 tasks
@mattleibow
Copy link
Contributor Author

mattleibow commented Dec 19, 2019

Oh no! 😭

opentk/opentk#823:

Change all namespaces to the new root

EDIT

I see the discussion and the reasons. The new namespace will be OpenToolkit - which is a nice name, just a bit more work ;)

@TysonMN
Copy link
Contributor

TysonMN commented Dec 19, 2019

The new namespace will be OpenToolkit - which is a nice name, just a bit more work ;)

Should be able to make quick work of it via find/replace.

@mattleibow
Copy link
Contributor Author

I'm a bit more worried about people updating the package and then blowing up at runtime or something.

mattleibow pushed a commit that referenced this issue Dec 21, 2019
* Views.WPF now multitargets Framework 4.5 and Core 3.0 #1029
* added .NET Core 3.0 to View.Desktiop.Common nuspec file
* added .NET Core 3.0 to View.WPF nuspec file
@mattleibow
Copy link
Contributor Author

I see the actual WPF nuspec did not depend on OpenTK, so I think we should be good with the updates. Either that or I have been release broken packages 😆

@TysonMN
Copy link
Contributor

TysonMN commented Dec 21, 2019

Great observation :)

PR #1078 up for that change

@mattleibow
Copy link
Contributor Author

This is now complete since #1067 and #1069

@TysonMN
Copy link
Contributor

TysonMN commented Dec 22, 2019

I didn't make any changes to Views.Windows Forms. I think it still targets only .NET Framework.

Should this issue stay open until it is also dual targeting with .NET Core?

@mattleibow mattleibow reopened this Dec 22, 2019
@mattleibow
Copy link
Contributor Author

Yep, it should. Thanks.

mattleibow added a commit that referenced this issue Dec 22, 2019
* Upgraded a good few things
 - multitargeting for netstandard1.3, netstandard2.0, netcoreapp3.0 and net45
 - iOS, tvOS, macOS, watchOS now builds on non-macOS (not embedded binaries)
 - removed empty assembly infos
 - using Directory.Build.* to clean up a few things
 - improved the msbuild tasks to properly skip
* Add the targets file to the nuget
* Let all the tests run before failing the build
* Pack and validate in one go
* Update the samples to use the new projects
* Use VS 2017 for native builds for now
* Update Dockerfiles
* removed references from Views.WPF to OpenTK #1029 (#1078)
* Install the type redirector
* Update depot_tools
* Don't do any signing for macOS

Co-authored-by: Tyson Williams <34664007+bender2k14@users.noreply.github.com>
mattleibow added a commit that referenced this issue Dec 27, 2019
mattleibow added a commit that referenced this issue Dec 27, 2019
@mattleibow mattleibow added this to the v1.68.2 milestone Apr 29, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants