Remove System.Drawing.Common from runtime #83356
Conversation
System.Drawing.Common was merged into winforms with dotnet/winforms#8633. Removing its sources and using PackageReferences to reference it.
|
Tagging subscribers to this area: @dotnet/area-system-drawing Issue DetailsSystem.Drawing.Common was merged into winforms with dotnet/winforms#8633. Remove its sources and use PackageReferences to reference it the latest available build from winforms.
|
| @@ -9,6 +9,6 @@ | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <ProjectReference Include="$(LibrariesProjectRoot)System.Drawing.Common\ref\System.Drawing.Common.csproj" /> | |||
| <PackageReference Include="System.Drawing.Common" Version="$(SystemDrawingCommonVersion)" /> | |||
There was a problem hiding this comment.
Is this going to create a build cycle between dotnet/runtime and dotnet/winforms? Is this build cycle going to break source build?
There was a problem hiding this comment.
Maybe System.Windows.Extensions should be moved over to WinForms repo as well?
There was a problem hiding this comment.
Good question. I will reach out to @MichaelSimons and @mmitche to get clarity on that. Presumably, we could make System.Windows.Extensions not build during source build.
The other place that "depends on" System.Drawing.Common are our compatibility shims. I don't know what the plan is here but AFAIK the source-build team wants to remove .NET Framework targeting packs from source build anyway and that would result in the compiler not being able to generate the type forwards as the reference assembly is missing. We could disable the shim generation during source build or alternatively check in IL to generate the shims. That would also remove the cyclic dependency in the shared framework build on the out-of-band project build.
Regarding moving System.Windows.Extensions over to winforms as well, cc @JeremyKuhne and @ericstj. No other library references System.Windows.Extensions except for the Microsoft.Windows.Compatibility package but that one is excluded from source-build as it depends on other prebuilts already.
There was a problem hiding this comment.
The facades and System.Windows.Extensions only need a Drawing reference to include type-forwards - we could use a SBRP or a non-shipping local reference to facilitate that during source build. I'm not sure where Microsoft.Win32.SystemEvents came into this discussion?
There was a problem hiding this comment.
winforms isn't included in SB, so the reference would have to be resolved with an SBRP or from excluding it from the build altogether.
However, based on the addition of a product winforms dependency below, I have concerns overall that this move will cause normal product build breaks and is introducing a product construction layering issue.
There was a problem hiding this comment.
Assuming you mean System.Windows.Extensions? What would this mean for the package that ships in 8.0 GA? Would it have a PackageReference to an old System.Drawing.Common?
There was a problem hiding this comment.
Would it have a PackageReference to an old System.Drawing.Common?
Yes, it would point to and bring along the 7.0.0 version of System.Drawing.Common. Same for the Microsoft.Windows.Compatibility package. Alternatively, we could discuss porting M.W.C and S.W.E to winforms as well. cc @JeremyKuhne
There was a problem hiding this comment.
For S.W.E I can't imagine that it would ever need new features in S.D.C. For projects consuming M.W.C. it might be a little odd to not get 8.0 surface area for S.D.C? Pulling in a direct reference to S.D.C. would be necessary, correct?
I'd presume the hardest core users of S.D.C. are using WinForms. The answers here don't impact them as WinForms is pulling in S.D.C. already, correct?
There was a problem hiding this comment.
In any case, if we do move them up I would expect that Libraries would still own things aligned with the other libraries (notably the x509 code). I would presume that there isn't much overhead with these?
There was a problem hiding this comment.
For projects consuming M.W.C. it might be a little odd to not get 8.0 surface area for S.D.C? Pulling in a direct reference to S.D.C. would be necessary, correct?
Yes that would be odd. We could move M.W.C into windowsdesktop. That would make sense anyway as it already depends on prebuilts outside of the stack and could then also reference the live version of System.Drawing.Common (which it gets from winforms).
System.Windows.Extensions is harder though as at least System.Security.Permissions depends on it. @ericstj and I are currently chatting offline what we could do about it.
as WinForms is pulling in S.D.C. already, correct?
Yes. The live version of System.Drawing.Comomn is already part of the WinForms and WindowsDesktop targeting packs.
There was a problem hiding this comment.
Thanks for this PR, @ViktorHofer!
I left mostly questions to understand the removal process.
I see there's a JIT test that is failing due to not finding the Bitmap type anymore: src/tests/JIT/Regression/JitBlue/GitHub_25468/GitHub_25468.cs
I assume the package reference would have to be added to the csproj?:
src/tests/JIT/Regression/JitBlue/GitHub_25468/GitHub_25468.csproj
src/libraries/System.IO.FileSystem.AccessControl/System.IO.FileSystem.AccessControl.sln
Show resolved
Hide resolved
|
Can we include an update to https://github.com/dotnet/runtime/blob/main/docs/area-owners.md to point people to winforms? |
| @@ -124,7 +124,7 @@ | |||
| <SystemComponentModelAnnotationsVersion>5.0.0</SystemComponentModelAnnotationsVersion> | |||
| <SystemDataSqlClientVersion>4.8.5</SystemDataSqlClientVersion> | |||
| <SystemDataDataSetExtensionsVersion>4.5.0</SystemDataDataSetExtensionsVersion> | |||
| <SystemDrawingCommonVersion>8.0.0-preview.3.23163.8</SystemDrawingCommonVersion> | |||
| <SystemDrawingCommonVersion>7.0.0</SystemDrawingCommonVersion> | |||
There was a problem hiding this comment.
@oleksandr-didyk This will require a new SBRP I think.
There was a problem hiding this comment.
Yes and we agree that this isn't ideal but we doubt that we can come up with a better solution before Preview 3 code-complete. We will likely need to add System.Drawing.Common/7.0.0 to SBRP but we don't need the net7.0 asset in it (to avoid bringing in the net7.0 targeting pack) as we can just use the net6.0 asset instead.
| @@ -45,6 +44,7 @@ | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="System.Drawing.Common" Version="$(SystemDrawingCommonVersion)" /> | |||
There was a problem hiding this comment.
Come GA time we'll want this to be 8.0.0, but I'm not sure how we can make that happen. Might need to move this meta-package to a higer repo.
|
CI failures for the latest commit c4a3861 are all unrelated:
@ericstj I think this is good to merge if you don't think there are any blocking unresolved comments. |
|
My feedback was non-blocking and for follow-up issues. |
ghost
locked as resolved and limited conversation to collaborators
System.Drawing.Common was merged into winforms with dotnet/winforms#8633.
Remove its sources and use PackageReferences to reference it the latest available build from winforms.