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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Oct 15, 2021
Merged

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

merged 2 commits into from
Oct 15, 2021

Conversation

safern
Copy link
Member

@safern safern commented Oct 13, 2021 •
edited
Loading

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.

@safern safern added the Servicing-consider Issue for next servicing release review label Oct 13, 2021
@dotnet-issue-labeler
Copy link

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
Copy link

ghost commented Oct 13, 2021

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
safern commented Oct 13, 2021
View reviewed changes
src/libraries/System.Drawing.Common/ref/System.Drawing.Common.cs Outdated
@@ -596,12 +596,16 @@ public sealed partial class Graphics : System.MarshalByRefObject, System.Drawing
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
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@danmoseley
Copy link
Member

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

@danmoseley
Copy link
Member

And @joperezr perhaps

ericstj
ericstj reviewed Oct 14, 2021
View reviewed changes
src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Windows.cs
@@ -684,7 +684,9 @@ public void DrawIconUnstretched(Icon icon, Rectangle targetRect)
/// WARNING: This method is for internal FX support only.
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
#if NET6_0_OR_GREATER
[Obsolete(Obsoletions.GetContextInfoMessage, DiagnosticId = Obsoletions.GetContextInfoDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 😄 Thanks for the suggestion.

@danmoseley
Copy link
Member

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

ericstj
ericstj approved these changes Oct 15, 2021
View reviewed changes
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Lgtm

@ViktorHofer
Copy link
Member

Presumably this will also go into main?

@safern
Copy link
Member Author

safern commented Oct 15, 2021

Yes. I will port it once we merge.

@safern safern added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 15, 2021
@safern safern merged commit f94d65e into dotnet:release/6.0 Oct 15, 2021
@safern safern deleted the MoveDrawingApisNet60 branch October 15, 2021 16:48
@runfoapp runfoapp bot mentioned this pull request Oct 18, 2021
Closed
safern added a commit to safern/runtime that referenced this pull request Oct 19, 2021
@safern
Move 2 Drawing APIs that are not implemented in netfx to netcoreapp3.…
c2e08aa 
…1 or later (dotnet#60371)
@safern safern mentioned this pull request Oct 19, 2021
Merged
safern added a commit that referenced this pull request Oct 19, 2021
@safern
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 ghost locked as resolved and limited conversation to collaborators Nov 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tarekgh tarekgh tarekgh approved these changes

@ericstj ericstj ericstj approved these changes

@danmoseley danmoseley Awaiting requested review from danmoseley

@JeremyKuhne JeremyKuhne Awaiting requested review from JeremyKuhne

Assignees
No one assigned
Labels
area-System.Drawing Servicing-approved Approved for servicing release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@safern @danmoseley @ViktorHofer @ericstj @tarekgh