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

Add netstandard2.0 target framework support for System.Xaml #51

Closed
wants to merge 17 commits into from
Closed

Add netstandard2.0 target framework support for System.Xaml #51

wants to merge 17 commits into from

Conversation

wieslawsoltes
Copy link

@wieslawsoltes wieslawsoltes commented Dec 4, 2018

@dnfclas
Copy link

dnfclas commented Dec 4, 2018

CLA assistant check
All CLA requirements met.

@wieslawsoltes wieslawsoltes changed the title Add netstandard2.0 target framework support Add netstandard2.0 target framework support for System.Xaml Dec 4, 2018
@stevenbrix
Copy link
Contributor

We're in the process of setting up a new CI build now that the project is public on GitHub. You'll need to close and re-open this PR in order to kick things off once it is set up. I'll update you when this is complete, thanks!

@stevenbrix
Copy link
Contributor

@wieslawsoltes this should be up and running now. can you please try closing and re-opening this pull request? Thanks!

@wieslawsoltes
Copy link
Author

@stevenbrix rebased the PR to kick the CI 😄

@wieslawsoltes
Copy link
Author

CI fails because of this:

018-12-04T19:06:44.9809062Z System\Xaml\Schema\TypeReflector.cs(1126,26): error CS0109: The member 'TypeReflector.ThreadSafeDictionary<K, V>.TryAdd(K, V)' does not hide an accessible member. The new keyword is not required. [F:\vsagent\61\s\src\Microsoft.DotNet.Wpf\src\System.Xaml\System.Xaml.csproj]
2018-12-04T19:06:44.9810505Z System\Xaml\Schema\TypeReflector.cs(1126,26): error CS0109: The member 'TypeReflector.ThreadSafeDictionary<K, V>.TryAdd(K, V)' does not hide an accessible member. The new keyword is not required. [F:\vsagent\61\s\src\Microsoft.DotNet.Wpf\src\System.Xaml\System.Xaml.csproj]

@vatsan-madhavan
Copy link
Member

vatsan-madhavan commented Dec 4, 2018

@wieslawsoltes, FYI - just running build.cmd locally after cloning and checking out netstandard2.0 results in those same failures.

@wieslawsoltes
Copy link
Author

I have pushed fix for CI errors.

@wieslawsoltes, FYI - just running build.cmd locally after cloning and checking out netstandard2.0 results in those same failures.

I get this error when run build.cmd:

The property 'installationPath' cannot be found on this object. Verify that the property exists.
System.Management.Automation.PropertyNotFoundException: The property 'installationPath' cannot be found on this object. Verify that the property exists.
   w System.Management.Automation.ExceptionHandlingOps.CheckActionPreference(FunctionContext funcContext, Exception exception)
   w System.Management.Automation.Interpreter.ActionCallInstruction`2.Run(InterpretedFrame frame)
   w System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame)
   w System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame)
at InitializeVisualStudioBuild, C:\DOWNLOADS\GitHub-Forks\wpf\eng\common\tools.ps1: line 93
at InitializeTools, C:\DOWNLOADS\GitHub-Forks\wpf\eng\common\tools.ps1: line 181
at <ScriptBlock>, C:\DOWNLOADS\GitHub-Forks\wpf\eng\common\Build.ps1: line 72

@dotMorten
Copy link
Contributor

Would it be better to target 2.1, so System.Xaml can start being optimized with Span<T> ? (considering there's lots of string parsing going on in there)

@wieslawsoltes
Copy link
Author

wieslawsoltes commented Dec 4, 2018

Would it be better to target 2.1, so System.Xaml can start being optimized with Span<T> ? (considering there's lots of string parsing going on in there)

@dotMorten I think using netstandard2.0 is universal (and necessary for library developers) and you get Span<T> anyway when your app targets netcoreapp2.1.

@dotMorten
Copy link
Contributor

you get Span anyway when your app targets netcoreapp2.1.

But that's not what you're targeting. For System.Xaml itself to take advantage of the Span<T> APIs, it needs to target the version that adds Span<T>

@vatsan-madhavan
Copy link
Member

@wieslawsoltes

The property 'installationPath' cannot be found on this object. Verify that the property exists.

Do you have a full Visual Studio installation (2019 preview) on your development machine?

@wieslawsoltes
Copy link
Author

@wieslawsoltes

The property 'installationPath' cannot be found on this object. Verify that the property exists.

Do you have a full Visual Studio installation (2019 preview) on your development machine?

@vatsan-madhavan No, I use VS2017

@dotMorten
Copy link
Contributor

@vatsan-madhavan did you enable .NET Core preview SDK?
image

@vatsan-madhavan vatsan-madhavan added this to the Future milestone Dec 4, 2018
@vatsan-madhavan vatsan-madhavan added the Enhancement Requested Product code improvement that does NOT require public API changes/additions label Dec 4, 2018
@vatsan-madhavan
Copy link
Member

The way you have added ValueSerializerAttribute is perfect - the codebase already had the affordance to do what you have done - you found just the right approach!

I want to note some considerations.

  • We would want to build the same sources (modulo ValueSerializerAttribute and minor nits like variants of ThreadSafeDictionary.TryAdd) for all the targets.
  • We would like to clean up our sources and get rid of old code related to partial-trust etc. (esp. if we don't use it for netcoreapp3.0, we don't want to keep it around for other targets either).
    • Changes (large or small) will have to result in corresponding updates to tests as well.
    • The tests we have onboarded in our repo at this time (DrtXaml) are only a small percentage of total Xaml tests. We are working on continuously onboarding more tests
  • There are no plans on building and shipping non-netcore products (targets) out of this repo. The goal of this project is to bring WPF to .NET Core 3.0.
    • The practical concern is that this repo is currently set up to publish its outputs into the .NET Core 3.0 shared framework through our CI system. Any attempt to build additional targets will require a reworking of packaging and creation of a new publishing pipeline to enable a separate System.Xaml package.
    • Even to enable multitargeted builds nominally, we have to do some work to ensure that those non-netcoreapp3.0 targets do not make their way into the .NET Core 3.0 shared framework/SDK packages.

In general, I think we can continue looking at this PR, but since it will require some additional work to do builds and packaging differently, I've marked it as a future change. Our immediate/short-term goal is onboarding of additional WPF sources to the repo - for now that takes precedence over changes to build/packaging to enable System.Xaml on additional targets.

cc @dotnet/wpf-developers

@vatsan-madhavan vatsan-madhavan self-requested a review December 4, 2018 23:29
@karelz karelz added * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) and removed Enhancement Requested Product code improvement that does NOT require public API changes/additions labels Dec 5, 2018
Copy link
Member

@karelz karelz left a comment

Choose a reason for hiding this comment

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

We should remove new public APIs from this PR. That should be independent PR, which needs first an issue to follow API review process.
Note that the WPF team will likely have not bandwidth to accept new APIs during 3.0 timeframe (just setting expectations).

@grubioe grubioe added the PR metadata: Label to tag PRs, to facilitate with triage label May 28, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
* NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish System.Xaml as a separate nuget package.
10 participants