-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fix System.Drawing.Configurations to be netstandard compatible #24758
Conversation
<IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'netfx'">true</IsPartialFacadeAssembly> | ||
<GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetGroup)' == 'netstandard'">SR.PlatformNotSupported_Drawing</GeneratePlatformNotSupportedAssemblyMessage> | ||
<!-- Although we have a netstandard configuration, we know we are not currently UAP compatible--> | ||
<UWPCompatible>false</UWPCompatible> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually needed? It should pass pinvoke checks with the netstandard build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it's not needed now because it won't scan netcoreapp ocnfiguration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I will remove it.
@@ -3,7 +3,7 @@ | |||
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" /> | |||
<ItemGroup> | |||
<ProjectReference Include="..\ref\System.Drawing.Common.csproj"> | |||
<SupportedFramework>netcoreapp2.0</SupportedFramework> | |||
<SupportedFramework>net461;netcoreapp2.0</SupportedFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weshaggard should I add any other supported frameworks? Like AllXamarinFrameworks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so. We generally only add those when the Xamarin folks ask us to because they put this assembly inbox. @marek-safar @akoeplinger can correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding @ericstj as well because your CI error does seem to be pointing out the fact that you didn't list those target frameworks but they are supported now by your netstandard2.0 builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weshaggard is this a new netstandard assembly/package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is a new package that targets netstandard. We have a few new packages that we should provide you a list of so you know what is coming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've talked to @marek-safar offline and I shared a list of what packages that target netstandard are coming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weshaggard @marek-safar I've added AllXamarinFrameworks
to the pkgproj and added them as inbox in the package index as well. Please let me know if looks good.
f51ec74
to
ceea736
Compare
@weshaggard looks good? |
thanks @weshaggard @danmosemsft |
…t/corefx#24758) * Fix System.Drawing.Configurations to be netstandard compatible * PR Feedback and Add net461 to pkgproj supported frameworks * Add xamarin frameworks as inbox * Change InboxOnTargetFramework to include $(AllXamarinFrameworks) Commit migrated from dotnet/corefx@46ea605
This depends on: #24736 to be merged.
Fixes: https://github.com/dotnet/corefx/issues/24675
cc: @danmosemsft @weshaggard