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.

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

Code cleanup in Microsoft.Common.tasks #8657

Merged
merged 4 commits into from
May 1, 2023
Merged

Conversation

jrdodds
Copy link
Contributor

@jrdodds jrdodds commented Apr 12, 2023

Fixes # (Issue to be created)

Context

Minor code cleanup in Microsoft.Common.tasks.

Changes Made

Removed duplicate UsingTask for the ResolveSDKReference task. The redundant ResolveSDKReference doesn't seem to create harm but is not useful.

Alphabetized the UsingTask elements. Retained the separate grouping of Roslyn tasks. Retained the blank lines around tasks that have different Runtimes and/or Conditions (i.e. GenerateResource, RegisterAssembly, and UnregisterAssembly). Ordering the UsingTask elements is intended to aid inspection and maintenance of tasks.

Testing

Tested on Windows 11 and macOS 12. Tested by running unit tests and by having this change in several development branches where msbuild has been run on project files.

Notes

This change is included in the implementation for #8613, which is PR #8614.

@jrdodds
Copy link
Contributor Author

jrdodds commented Apr 12, 2023

Note: There seem to be some issues with the msbuild-pr pipeline. Build 20230412.4 failed with restore issues and with a failed unit test. I reverted the branch specific commits and the build succeeded. I reverted the reversion and the build (20230412.10) succeeded.

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Looks like a safe change. Thank you!

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Thanks!

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Apr 24, 2023
@Forgind Forgind merged commit 768cad1 into dotnet:main May 1, 2023
@Forgind
Copy link
Member

Forgind commented May 1, 2023

Thanks @jrdodds!

@danmoseley
Copy link
Member

as an aside, I hope/assume XmlTextReader interns all the duplicate "Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" strings for us. So there is no memory advantage in pulling it out into a property.

@jrdodds jrdodds deleted the Commontasks branch May 3, 2023 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants