Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Remove UAP target #41759

Merged
merged 19 commits into from
Oct 17, 2019
Merged

Remove UAP target #41759

merged 19 commits into from
Oct 17, 2019

Conversation

ViktorHofer
Copy link
Member

cc @safern @danmosemsft

@ViktorHofer
Copy link
Member Author

I still need to figure out the harvesting story for the uap assets.

@ViktorHofer ViktorHofer added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 13, 2019
@ViktorHofer ViktorHofer force-pushed the RemoveUapTarget branch 3 times, most recently from 524bce5 to 0dfa300 Compare October 15, 2019 20:24
@ViktorHofer
Copy link
Member Author

OK this should be ready now. Anybody wants to review the 1.4k file changes? 😫😫😫 @danmosemsft @stephentoub @ericstj @safern

@davidsh
Copy link
Contributor

davidsh commented Oct 15, 2019

OK this should be ready now. Anybody wants to review the 1.4k file changes?

How did you modify 1.4k files in the first place? Did you use an automated tool of some kind? That's a lot of files to hand edit.

I would suggest that if it builds ok, then it should be merged. Then we can sort out issues (if any) later.

@ViktorHofer
Copy link
Member Author

How did you modify 1.4k files in the first place? Did you use an automated tool of some kind? That's a lot of files to hand edit.

I did most of the changes by hand + a few regex replace patterns for the Configurations.props files and the <Configurations /> element in the csproj files.

@davidsh
Copy link
Contributor

davidsh commented Oct 15, 2019

I did most of the changes by hand

Impressive!

I still think we should merge once the build looks good.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Oct 15, 2019

I think 50% of the changes were automated so it's probably not that impressive. But that proves how badly I want this change in ;)

@danmoseley
Copy link
Member

danmoseley commented Oct 15, 2019

For changes like this I usually pull them down and fiddle with grep to try to spot anomalous changes. Eg.,

git diff -b -U0 --diff-filter=d upstream/master..viktorhofer/removeuaptarget | \t\grep  -E "(^- |^\+ )" > out

Shows just the add/remove lines in files that weren't deleted, with no context. Then I will grep away in my editor diffs that I don't care about, like .*\<configurations.*\n or whatever until it's small enough to read.

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

Other than my questions/comments, overall looks good.

@ViktorHofer
Copy link
Member Author

@jkotas @stephentoub and I agreed offline that we remove all WinRT related files with this PR as the chance to bring them in their current state back is close to zero. One of the reasons for that is that more Win32 APIs are now allowed than back in 2017.

@ViktorHofer
Copy link
Member Author

Number of tests before and after the change. The results match my assumption of having ~70k less test invocations because of the removal of the UWP leg.

Before

image

After

image

@ViktorHofer ViktorHofer merged commit 3b6ec2b into dotnet:master Oct 17, 2019
@ViktorHofer ViktorHofer deleted the RemoveUapTarget branch October 17, 2019 13:04
@karelz karelz added this to the 5.0 milestone Dec 19, 2019
macrogreg pushed a commit to open-telemetry/opentelemetry-dotnet-instrumentation that referenced this pull request Sep 24, 2020
* Remove Uap target

* Remove RemoteInvokeForUap wrapper calls entirely

* Harvest S.D.SqlClient on UWP

* Disable UWP PInvoke analyzer

* Enable WindowsRuntime* tests on netcoreapp

Commit migrated from dotnet/corefx@3b6ec2b
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Remove Uap target

* Remove RemoteInvokeForUap wrapper calls entirely

* Harvest S.D.SqlClient on UWP

* Disable UWP PInvoke analyzer

* Enable WindowsRuntime* tests on netcoreapp

Commit migrated from dotnet/corefx@3b6ec2b
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants