Skip to content

Clean-up System.Drawing.Common and remove the Unix code. - #64623

Merged
ViktorHofer merged 16 commits into
dotnet:mainfrom
teo-tsirpanis:sdc-remove-unix
Apr 20, 2022
Merged

Clean-up System.Drawing.Common and remove the Unix code.#64623
ViktorHofer merged 16 commits into
dotnet:mainfrom
teo-tsirpanis:sdc-remove-unix

Conversation

@teo-tsirpanis

@teo-tsirpanisteo-tsirpanis commented Feb 1, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Since #64084 got merged, System.Drawing.Common is supported only on Windows. This PR actually removes all Unix-specific code and tests, and merges the .Windows.cs files into their formerly cross-platform counterparts, drastically simplifying the codebase.

I also cleaned-up the code a little bit (used a [ThreadStatic] instead of a named data slot, and removed unused files).

@ghostghost added community-contribution Indicates that the PR has been added by a community member area-System.Drawing labels Feb 1, 2022
@ghost

ghost commented Feb 1, 2022

Copy link
Copy Markdown

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

Issue Details

Since #64084 got merged, System.Drawing.Common is supported only on Windows. This PR actually removes all Unix-specific code and tests, and merges the .Windows.cs files into their formerly cross-platform counterparts, drastically simplifying the codebase.

Author:teo-tsirpanis
Assignees:-
Labels:

area-System.Drawing, community-contribution

Milestone:-

@teo-tsirpanisteo-tsirpanis mentioned this pull request Feb 1, 2022
Closed
@safern

safern commented Feb 1, 2022

Copy link
Copy Markdown
Member

Thanks, @teo-tsirpanis -- please let me know when this is ready for review (CI is Red). I'd like to ask if you need any help to get CI green or a review, or just wait?

teo-tsirpanis
teo-tsirpanis commented Feb 1, 2022
View reviewed changes

@teo-tsirpanisteo-tsirpanis left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for offering to help @safern. CI failures are so far unrelated (they occur on non-Windows platforms). I have two questions.

Comment threadsrc/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj Outdated
Comment threadsrc/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj Outdated
safern
safern reviewed Feb 2, 2022
View reviewed changes
Comment threadsrc/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj Outdated
safern
safern reviewed Feb 2, 2022
View reviewed changes
Comment threadsrc/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj Outdated
@ViktorHoferViktorHofer mentioned this pull request Feb 8, 2022
Merged
@teo-tsirpanis

This comment was marked as resolved.

Sign in to view
@teo-tsirpanis
teo-tsirpanisforce-pushed the sdc-remove-unix branch 2 times, most recently from 3f4fe59 to 966d6bdCompareFebruary 11, 2022 20:27
@ViktorHofer

ViktorHofer commented Feb 12, 2022

Copy link
Copy Markdown
Member

@teo-tsirpanis#64500 which got just merged caused a conflict which I just resolved. Hope that's ok.

@teo-tsirpanis

teo-tsirpanis commented Feb 12, 2022

Copy link
Copy Markdown
ContributorAuthor

Thanks @ViktorHofer.

@teo-tsirpanis
teo-tsirpanisforce-pushed the sdc-remove-unix branch 2 times, most recently from 2893c32 to 608394eCompareFebruary 15, 2022 13:24
@teo-tsirpanis

teo-tsirpanis commented Feb 15, 2022

Copy link
Copy Markdown
ContributorAuthor

CI is green (failures are non-Windows and unrelated). @safern is there anything else to do? This PR is getting more and more conflicts as it stays open.

@teo-tsirpanis

teo-tsirpanis commented Feb 22, 2022

Copy link
Copy Markdown
ContributorAuthor

Conflicts are resolved.

@danmoseley

danmoseley commented Feb 23, 2022

Copy link
Copy Markdown
Contributor

@dotnet/area-system-drawing as @safern changed teams, this will need a different one of you to sign off.

@runfoapprunfoappBot mentioned this pull request Feb 23, 2022
Closed
@teo-tsirpanis
teo-tsirpanisforce-pushed the sdc-remove-unix branch 3 times, most recently from 34379c8 to 1c0ca40CompareMarch 15, 2022 10:46
@teo-tsirpanis

teo-tsirpanis commented Mar 16, 2022

Copy link
Copy Markdown
ContributorAuthor

Great, CI now passes. Can someone review?

BinaryFormatter on some enum types breaks on non-Windows because the TypeForwardedFromAttribute is missing, but I made these test cases run only on Windows. I hope there's no problem.

@danmoseley
danmoseley requested a review from ericstjMarch 23, 2022 04:14
@danmoseley

danmoseley commented Mar 23, 2022

Copy link
Copy Markdown
Contributor

we will need a new reviewer with Santi gone.

@teo-tsirpanis
teo-tsirpanisforce-pushed the sdc-remove-unix branch 2 times, most recently from 9a2312f to c699429CompareMarch 27, 2022 18:02
@teo-tsirpanisteo-tsirpanis changed the title Remove the Unix code from System.Drawing.Common.Clean-up System.Drawing.Common and remove the Unix code.Apr 19, 2022
ViktorHofer
ViktorHofer reviewed Apr 19, 2022
View reviewed changes
Comment threadsrc/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj Outdated
teo-tsirpanisand others added 2 commits April 19, 2022 14:10
@teo-tsirpanis@ViktorHofer
Remove TargetsAnyOS.
df7a067
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@ViktorHofer
SDC project file clean-up
b61661f
ViktorHofer
ViktorHofer reviewed Apr 19, 2022
View reviewed changes
Comment threadsrc/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.cs Outdated
ViktorHofer
ViktorHofer reviewed Apr 19, 2022
View reviewed changes
Comment threadsrc/libraries/System.Drawing.Common/src/System/Drawing/Gdiplus.cs
ViktorHofer
ViktorHofer reviewed Apr 19, 2022
View reviewed changes
Comment threadsrc/libraries/System.Drawing.Common/src/System/Drawing/Gdiplus.cs
ViktorHofer
ViktorHofer reviewed Apr 19, 2022
View reviewed changes

@ViktorHoferViktorHofer left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Went over all the changes. This looks REALLY good. Thanks for the great work. The remaining work now is to:

  1. Switch all the !! null checks to their previous state. For most, you could probably use ArgumentNullException.ThrowIfNull(myString);.
  2. Make sure that the remaining questions are answered.

After that we can get the PR merged :)

PS: I hope you don't mind that I pushed one commit to your branch to clean-up the SDC project file further.

@ViktorHoferViktorHofer mentioned this pull request Apr 19, 2022
Closed
ViktorHofer
ViktorHofer approved these changes Apr 20, 2022
View reviewed changes

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Congratulations and thank you for your contribution 🎉🎉🎉 We very much appreciate the time and effort that you put into this change.

@ViktorHofer
ViktorHofer merged commit 5b55b06 into dotnet:mainApr 20, 2022
@teo-tsirpanis
teo-tsirpanis deleted the sdc-remove-unix branch April 20, 2022 08:00
@ViktorHofer

ViktorHofer commented Apr 20, 2022

Copy link
Copy Markdown
Member

If you want to pick up another task, there are plenty ones in the backlog that are marked as up-for-grabs. And I will promise that future changes won't take us that long to react and will hopefully cause less merge conflicts (😥) on your side.

directhex pushed a commit to directhex/runtime that referenced this pull request Apr 21, 2022
@teo-tsirpanis@ViktorHoferJo Shields
Clean-upSystem.Drawing.Commonand remove the Unix code. (dotnet#64623
f73c6aa
)
* Remove all Unix-specific files.
* Throw PNSE on non-Windows.
* Merge the Windows-specific files into their formerly cross-platform counterparts.
* Remove all mentions of Unix in the tests.
* Remove two always-on defines.
* Merge two item groups in the project file.
No reason to sort them; the list is already unsorted.
And rename a remaining formerly Windows-specific file.
* Remove the NoCOMWrappers files.
* Fail on unsupported platforms when a library is trying to be loaded.
* Fix compile errors.
* Small changes in the project file.
* Remove two meaningless asserts.
* Run BinaryFormatter tests on SDC types only on Windows.
* Use `[ThreadStatic]` in Gdip.ThreadData.
* Remove `TargetsAnyOS`.
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
* SDC project file clean-up
* Remove `!!` from System.Drawing.Common.
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@danmoseley

danmoseley commented May 10, 2022

Copy link
Copy Markdown
Contributor

I also wanted to add my belated thanks here @teo-tsirpanis. This was a lot of work.

@ghostghost locked as resolved and limited conversation to collaborators Jun 9, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@safernsafernsafern left review comments
@ViktorHoferViktorHoferViktorHofer approved these changes
@ericstjericstjAwaiting requested review from ericstj
@carlossanlopcarlossanlopAwaiting requested review from carlossanlop
+1 more reviewer
@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@ViktorHoferViktorHofer

Labels

area-System.Drawingcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@teo-tsirpanis@safern@ViktorHofer@danmoseley@carlossanlop@stephentoub