[release/6.0] Move 2 Drawing APIs that are not implemented in netfx to net6.0 or later #60371

safern · 2021-10-13T21:22:38Z

This was discovered with the new Microsoft.DotNet.Compatibility tool: #60306 (comment)

Customer Impact

In 6.0 we added two new graphics APIs for better performance, but these were added to the netstandard2.0 implementation which in NuGet packages that's the ref assembly as well. These APIs are not part of the netfx implementation and that could cause runtime problems if for example a library targeting netstandard2.0, used these APIs and then a net472 application consumed this library, it would fail with MissingMethodException at runtime.

Testing

Unit tests, PackageValidation and API Compat.

Risk

Low, the I'm just bringing the old behavior before the APIs were added and conditioning these APIs to .NET Core 3.1 or greater.

ghost · 2021-10-13T21:22:44Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-10-13T21:22:47Z

Tagging subscribers to this area: @safern, @tarekgh
See info in area-owners.md if you want to be subscribed.

Issue Details

This was discovered with the new Microsoft.DotNet.Compatibility tool: #60306 (comment)

Customer Impact

In 6.0 we added two new graphics APIs for better performance, but these were added to the netstandard2.0 implementation which in NuGet packages that's the ref assembly as well. These APIs are not part of the netfx implementation and that could cause runtime problems if for example a library targeting netstandard2.0, used these APIs and then a net472 application consumed this library, it would fail with MissingMethodException at runtime.

Testing

Unit tests, PackageValidation and API Compat.

Risk

Low, the I'm just bringing the old behavior before the APIs were added and conditioning these APIs to NET 6 or greater.

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

safern · 2021-10-13T21:23:28Z

src/libraries/System.Drawing.Common/ref/System.Drawing.Common.cs

        public static System.Drawing.Graphics FromImage(System.Drawing.Image image) { throw null; }
        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
        [System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]
+#if NET6_0_OR_GREATER


decided to use ifdefs on the ref assembly for simplicity of the change and because ref assemblies sources shouldn't change during servicing, so this doesn't harm any possible experience with GenAPI.

src/libraries/System.Drawing.Common/src/misc/GDI/WindowsGraphics.cs

src/libraries/System.Drawing.Common/ref/System.Drawing.Common.cs

danmoseley · 2021-10-14T14:29:41Z

Thanks.@ericstj could you please also review before @safern sends the tactics mail? Since were being extra careful now

danmoseley · 2021-10-14T14:29:58Z

And @joperezr perhaps

ericstj · 2021-10-14T15:59:49Z

src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Windows.cs

        /// </summary>
        [EditorBrowsable(EditorBrowsableState.Never)]
+#if NET6_0_OR_GREATER
        [Obsolete(Obsoletions.GetContextInfoMessage, DiagnosticId = Obsoletions.GetContextInfoDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]


Did we consider making this change for netcoreapp3.1 and later instead of net6.0 and later? That could make it so the Obsolete message reaches people earlier: those that update the package without retargeting. The package builds for netcoreapp3.1 as well as net6.0 and moving this change from netstandard2.0 to netcoreapp3.1 instead of net6.0 might reach more folks (and be a smaller difference from what we've already shipped in previews).

I'm fine either way, but I'd like this to be considered.

I'm fine with including it in netcoreapp3.1. I'll update the PR accordingly.

Done 😄 Thanks for the suggestion.

danmoseley · 2021-10-14T20:49:36Z

Feel free to send the tactics mail today, you might cc me and Eric.

ericstj

Lgtm

ViktorHofer · 2021-10-15T14:02:08Z

Presumably this will also go into main?

safern · 2021-10-15T15:16:45Z

Yes. I will port it once we merge.

…1 or later (dotnet#60371)

…60599) * Move 2 Drawing APIs that are not implemented in netfx to netcoreapp3.1 or later (#60371) * Don't use ifdefs on ref file

Move 2 Drawing APIs that are not implemented in netfx to net6.0 or later

2cabe9c

safern added the Servicing-consider Issue for next servicing release review label Oct 13, 2021

safern requested review from JeremyKuhne, danmoseley and ericstj October 13, 2021 21:22

ghost added area-System.Drawing new-api-needs-documentation labels Oct 13, 2021

safern commented Oct 13, 2021

View reviewed changes

tarekgh reviewed Oct 13, 2021

View reviewed changes

src/libraries/System.Drawing.Common/src/misc/GDI/WindowsGraphics.cs Show resolved Hide resolved

tarekgh reviewed Oct 13, 2021

View reviewed changes

src/libraries/System.Drawing.Common/ref/System.Drawing.Common.cs Outdated Show resolved Hide resolved

safern removed the new-api-needs-documentation label Oct 13, 2021

tarekgh approved these changes Oct 13, 2021

View reviewed changes

ericstj reviewed Oct 14, 2021

View reviewed changes

Include the APIs on netcoreapp3.1 or later

8f071a1

ericstj approved these changes Oct 15, 2021

View reviewed changes

safern added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 15, 2021

safern merged commit f94d65e into dotnet:release/6.0 Oct 15, 2021

safern deleted the MoveDrawingApisNet60 branch October 15, 2021 16:48

runfoapp bot mentioned this pull request Oct 18, 2021

Error in CI Mono llvmfullaot Pri0 Runtime Tests Run Linux arm64 release #60234

Closed

safern added a commit to safern/runtime that referenced this pull request Oct 19, 2021

Move 2 Drawing APIs that are not implemented in netfx to netcoreapp3.…

c2e08aa

…1 or later (dotnet#60371)

safern mentioned this pull request Oct 19, 2021

Move 2 Drawing APIs that are not implemented in netfx to netcoreapp #60599

Merged

safern added a commit that referenced this pull request Oct 19, 2021

Move 2 Drawing APIs that are not implemented in netfx to netcoreapp (#…

21bdf48

…60599) * Move 2 Drawing APIs that are not implemented in netfx to netcoreapp3.1 or later (#60371) * Don't use ifdefs on ref file

ghost locked as resolved and limited conversation to collaborators Nov 14, 2021

[release/6.0] Move 2 Drawing APIs that are not implemented in netfx to net6.0 or later #60371

[release/6.0] Move 2 Drawing APIs that are not implemented in netfx to net6.0 or later #60371

Uh oh!

Conversation

safern commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer Impact

Testing

Risk

Uh oh!

ghost commented Oct 13, 2021

Uh oh!

ghost commented Oct 13, 2021

Customer Impact

Testing

Risk

Uh oh!

safern Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Oct 14, 2021

Uh oh!

danmoseley commented Oct 14, 2021

Uh oh!

ericstj Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

safern Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

safern Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Oct 14, 2021

Uh oh!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Oct 15, 2021

Uh oh!

safern commented Oct 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

safern commented Oct 13, 2021 •

edited

Loading