Skip to content

Conversation

dfederm
Copy link
Contributor

@dfederm dfederm commented Dec 21, 2021

Fixes #5190

Context

$(MSBuildAssemblyVersion) is a built-in property which returns the MSBuild Assembly version as you may expect. In various props/targets it's used to detect whether MSBuild v4 is running, and at this point that's never the case that MSBuild v4 (or any MSBuild for that matter) would be using these props/targets.

My understanding is that the props/targets which ship with the .NET Framework and do ship with MSBuild v4 are distributed separately and not part of this repo.

Changes Made

Removed all conditions where the existence of $(MSBuildAssemblyVersion) was used.

<!--
Import wildcard "ImportBefore" props files if we're actually in a 12.0+ project (rather than a project being
treated as 4.0)
Wildcard imports come from $(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props\ folder.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the reference to the "Microsoft.Common.props.d folder" since as far as I can tell, it never actually existed? Even in the PR which added this comment, it didn't seem to apply then either.

Copy link
Contributor

@Nirmal4G Nirmal4G Dec 22, 2021

Choose a reason for hiding this comment

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

The reason for the weird naming is given the comments itself.

Unfortunately, there is already a file named "Microsoft.Common.props" in this directory so we have to have a slightly different directory name to hold extensions.

But the actual path is $(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Imports\$(MSBuildThisFile)\ for Microsoft.Common.props.

@Nirmal4G
Copy link
Contributor

Nirmal4G commented Dec 22, 2021

Should fix #5190. We could also remove the xmlns too!

This could be a part of the larger refactoring that'll lead into #1686.

@dfederm
Copy link
Contributor Author

dfederm commented Dec 22, 2021

Should fix #5190. We could also remove the xmlns too!

Sure, I'll take care of xmlns in a separate PR :)

Copy link
Contributor

@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.

Love all the deleted code!

Copy link
Contributor

@Nirmal4G Nirmal4G left a comment

Choose a reason for hiding this comment

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

LGTM 👌

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

This is delightful and I can't wait to merge it. I do think that should wait until we have a 17.2 branch, though, so I'm not going to hit "approve" quite yet. We'll work that out shortly I think.

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.

Microsoft.Common.tasks cleanup

4 participants