Remove System.Drawing.Common from runtime#83356

ViktorHofer · 2023-03-13T22:02:41Z

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.

System.Drawing.Common was merged into winforms with dotnet/winforms#8633. Removing its sources and using PackageReferences to reference it.

ghost · 2023-03-13T22:03:07Z

Tagging subscribers to this area: @dotnet/area-system-drawing
See info in area-owners.md if you want to be subscribed.

Issue Details

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.

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	`area-System.Drawing`, `new-api-needs-documentation`
Milestone:	-

jkotas · 2023-03-13T22:35:26Z

src/libraries/System.Windows.Extensions/ref/System.Windows.Extensions.csproj

@@ -9,6 +9,6 @@
  </ItemGroup>

  <ItemGroup>
-    <ProjectReference Include="$(LibrariesProjectRoot)System.Drawing.Common\ref\System.Drawing.Common.csproj" />
+    <PackageReference Include="System.Drawing.Common" Version="$(SystemDrawingCommonVersion)" />


Is this going to create a build cycle between dotnet/runtime and dotnet/winforms? Is this build cycle going to break source build?

Maybe System.Windows.Extensions should be moved over to WinForms repo as well?

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.

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?

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.

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?

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

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?

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?

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.

carlossanlop

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

eng/Version.Details.xml

src/libraries/System.IO.FileSystem.AccessControl/System.IO.FileSystem.AccessControl.sln

src/libraries/shims/Directory.Build.props

src/libraries/tests.proj

src/libraries/System.Drawing.Common/README.md

carlossanlop · 2023-03-13T23:36:10Z

Can we include an update to https://github.com/dotnet/runtime/blob/main/docs/area-owners.md to point people to winforms?

mmitche · 2023-03-14T17:37:02Z

eng/Versions.props

    <SystemDataSqlClientVersion>4.8.5</SystemDataSqlClientVersion>
    <SystemDataDataSetExtensionsVersion>4.5.0</SystemDataDataSetExtensionsVersion>
-    <SystemDrawingCommonVersion>8.0.0-preview.3.23163.8</SystemDrawingCommonVersion>
+    <SystemDrawingCommonVersion>7.0.0</SystemDrawingCommonVersion>


@oleksandr-didyk This will require a new SBRP I think.

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.

ericstj · 2023-03-15T20:49:35Z

src/libraries/Microsoft.Windows.Compatibility/src/Microsoft.Windows.Compatibility.csproj

  </ItemGroup>

  <ItemGroup>
+    <PackageReference Include="System.Drawing.Common" Version="$(SystemDrawingCommonVersion)" />


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.

src/tests/Common/test_dependencies/test_dependencies.csproj

carlossanlop · 2023-03-16T00:07:25Z

CI failures for the latest commit c4a3861 are all unrelated:

browser-wasm linux Release AllSubsets_Mono_RuntimeTests monointerpreter
- IOException running NuGet-Migrations during tests in dotnet CLI first run #80619
Libraries Test Run release mono linux arm64 Debug
- Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests segfault in Mono #81249
Libraries Test Run release mono linux x64 Debug
- System.Net native crash in mono linux x64 #83481
Libraries Test Run release coreclr Linux x64 Debug
- Segmentation fault while executing System.Net tests in linux x64 coreclr #83482
Libraries Test Run release coreclr windows x64 Debug
- System.Net.Http.WinHttpHandler.StartRequestAsync assertion failed #83486
wasi-wasm windows Release LibraryTests_Smoke and wasi-wasm linux Release LibraryTests_Smoke:
- [trimming] Test failures only when trimming #73961

@ericstj I think this is good to merge if you don't think there are any blocking unresolved comments.

ericstj · 2023-03-16T02:35:34Z

My feedback was non-blocking and for follow-up issues.

Remove System.Drawing.Common from runtime

2c34b7b

System.Drawing.Common was merged into winforms with dotnet/winforms#8633. Removing its sources and using PackageReferences to reference it.

ViktorHofer requested review from a team, JeremyKuhne and ericstj March 13, 2023 22:02

ViktorHofer self-assigned this Mar 13, 2023

ghost added area-System.Drawing new-api-needs-documentation labels Mar 13, 2023

ViktorHofer removed the new-api-needs-documentation label Mar 13, 2023

dotnet deleted a comment Mar 13, 2023

jkotas reviewed Mar 13, 2023

View reviewed changes

carlossanlop reviewed Mar 13, 2023

View reviewed changes

teo-tsirpanis reviewed Mar 13, 2023

View reviewed changes

src/libraries/System.Drawing.Common/README.md Show resolved Hide resolved

This was referenced Mar 14, 2023

IOException running NuGet-Migrations during tests in dotnet CLI first run #80619

Closed

[release/6.0] Doublelinklist GC failures on Mono #83245

Closed

[jitstress] HardwareIntrinsics_ro fails with "process cannot access the file" error #83298

Closed

Remove dependency and fix tests

08edb01

mmitche reviewed Mar 14, 2023

View reviewed changes

Fix shim S.D.C reference

607cce2

ericstj approved these changes Mar 15, 2023

View reviewed changes

Remove runtime test which causes failures

c4a3861

build-analysis bot mentioned this pull request Mar 16, 2023

Roslyn source generator crash on mono/linux/arm64 #81123

Closed

carlossanlop merged commit 99aa25f into main Mar 16, 2023

carlossanlop deleted the RemoveSystemDrawingCommon branch March 16, 2023 02:58

build-analysis bot mentioned this pull request Mar 16, 2023

Crashes when initializing System.Net.Security.Native #83540

Closed

MichaelSimons mentioned this pull request Mar 20, 2023

Add System.Common.Drawing prebuilt dotnet/installer#15868

Merged

LoopedBard3 mentioned this pull request Mar 20, 2023

Windows Micro Runs failing dotnet/performance#2931

Closed

MichaelSimons mentioned this pull request Mar 20, 2023

[SBRP] add System.Drawing.Common 7.0.0 ref pack dotnet/source-build#3338

Closed

LoopedBard3 mentioned this pull request Mar 21, 2023

Removed failing System.Drawing tests from net8.0+ runs. dotnet/performance#2936

Merged

ghost locked as resolved and limited conversation to collaborators Apr 15, 2023

Comments

Conversation

ViktorHofer commented Mar 13, 2023

Uh oh!

ghost commented Mar 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlossanlop commented Mar 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlossanlop commented Mar 16, 2023

Uh oh!

ericstj commented Mar 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ViktorHofer Mar 14, 2023 •

edited

Loading

ericstj Mar 14, 2023 •

edited

Loading

ViktorHofer Mar 15, 2023 •

edited

Loading