Skip to content

Conversation

@ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jan 22, 2022

To avoid additional project evaluations when a project doesn't multi-target, don't set the SetTargetFramework property and undefine the TargetFramework property. For consistency also undefine the RuntimeIdentifier property when a project is RID agnostic. This mimics the msbuild's Common.targets behavior which is disabled because the SkipGetTargetFrameworkProperties attribute is set to the annotated P2Ps.

Change

Without this change, projects like System.Runtime or System.Linq.Expressions which don't multi-target, are evaluated twice. Once without TargetFramework being passed in globally and once with it being passed in.

This change shows a drastic speed improvement when building the libs.ref projects in dotnet/runtime and changing the projects to use the TargetFramework property instead of TargetFrameworks when the project doesn't multi-target. That's because additional project evaluations are avoided:

image

42s instead of 55s in a no-op incremental build.

To avoid additional project evaluations when a project doesn't multi-target, don't set the SetTargetFramework property and undefine the TargetFramework property. For consistency also undefine the RuntimeIdentifier property when a project is RID agnostic. This mimics the msbuild's Common.targets behavior which is disabled because the SkipGetTargetFrameworkProperties attribute is set to the annotated P2Ps.

This change shows a drastic speed improvement when building the libs.ref projects in dotnet/runtime and changing the projects to use the TargetFramework property instead of TargetFrameworks when the project doesn't multi-target.
@ViktorHofer ViktorHofer requested review from ericstj and safern January 22, 2022 02:54
@ViktorHofer ViktorHofer self-assigned this Jan 22, 2022
@ViktorHofer
Copy link
Member Author

@ericstj can you please review?

projectReference.SetMetadata("UndefineProperties", undefineProperties + ";TargetFramework");
}

if (projectReference.GetMetadata("IsRidAgnostic") == "true")
Copy link
Member

Choose a reason for hiding this comment

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

What sets this metadata? I don't think we should ever flow RID between projects, it should always be inferred from the active TargetFramework value.

Copy link
Member Author

Choose a reason for hiding this comment

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

IsRidAgnostic is set by msbuild: https://github.com/dotnet/msbuild/blob/8872ed650643d12340fb22ece223c1aacf5a30a0/src/Tasks/Microsoft.Common.CrossTargeting.targets#L48.

Our projects don't set a runtime identifier so it doesn't matter but as I mimicked the SDK's behavior (see the comment with the link) I included this part as well, for completeness.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ericstj does that make sense? If there isn't any other feedback I would like to merge this in :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's interesting. I hadn't realized they touched the RuntimeIdentifier property across projects.
If a project doesn't specify a any RID it is built without RIDs. If a project specifies a RID it will be built with the RID of the parent, should that be globally specified.

@ericstj
Copy link
Member

ericstj commented Jan 25, 2022

This is definitely an improvement, just a small question about RID. TFTF!

@ericstj ericstj merged commit 93656ab into main Jan 27, 2022
@safern safern deleted the ViktorHofer-patch-1 branch January 27, 2022 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants