Skip to content

Cleanup usages of IsTestProject #49838

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Cleanup usages of IsTestProject #49838

wants to merge 10 commits into from
+178 −24

Conversation

Youssef1313
Copy link
Member

Fixes #49063

This was referenced Jul 17, 2025
Closed
Closed
Youssef1313
Youssef1313 commented Jul 17, 2025
View reviewed changes
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets Outdated
Comment on lines 1326 to 1330
<Target Name="ValidateExecutableReferencesCore"
DependsOnTargets="_CalculateIsVSTest;ValidateExecutableReferences"
AfterTargets="_GetProjectReferenceTargetFrameworkProperties">

</Target>
Copy link
Member Author

Choose a reason for hiding this comment

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

Created this extra target without condition. The reason is I want to execute _CalculateIsVSTest target unconditionally, because _CalculateIsVSTest will set the property needed for the condition in ValidateExecutableReferences.

@Youssef1313 Youssef1313 marked this pull request as ready for review July 17, 2025 10:07
@nohwnd nohwnd mentioned this pull request Jul 17, 2025
Closed
@build-analysis build-analysis bot mentioned this pull request Jul 17, 2025
Open
@Youssef1313
Copy link
Member Author

@baronfel @dsplaisted This is green 🎉

nohwnd
nohwnd reviewed Jul 17, 2025
View reviewed changes
This was referenced Jul 17, 2025
Open
Open
dsplaisted
dsplaisted reviewed Jul 17, 2025
View reviewed changes
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets
@@ -115,7 +115,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<!-- Set the IsRidAgnostic property if this project should NOT accept global RuntimeIdentifier and SelfContained
property values from referencing projects. -->
<PropertyGroup Condition="'$(IsRidAgnostic)' == ''">
<IsRidAgnostic Condition="('$(_IsExecutable)' == 'true' And '$(IsTestProject)' != 'true') Or
<IsRidAgnostic Condition="('$(_IsExecutable)' == 'true') Or
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<IsRidAgnostic Condition="('$(_IsExecutable)' == 'true') Or
<IsRidAgnostic Condition="('$(_IsExecutable)' == 'true' And ('$(IsTestProject)' != 'true' Or '$(IsTestingPlatformApplication) != 'true')) Or

Copy link
Member Author

Choose a reason for hiding this comment

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

I would try to avoid reading IsTestingPlatformApplication in evaluation if possible

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets Outdated
@@ -1322,6 +1323,12 @@ Copyright (c) .NET Foundation. All rights reserved.
UseAttributeForTargetFrameworkInfoPropertyNames="$(_UseAttributeForTargetFrameworkInfoPropertyNames)"/>
</Target>

<Target Name="ValidateExecutableReferencesCore"
Copy link
Member

Choose a reason for hiding this comment

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

I think we can collapse _CalculateIsVSTestTestProject, ValidateExecutableReferencesCore, and ValidateExecutableReferences all into one target. Currently, ValidateExecutableReferences only has a single call to a task in it. So we can move the condition from the target to the task, and then move the _CalculateIsVSTestTestProject logic to set the properties into the ValidateExecutableReferences itself.

That seems a lot simpler and cleaner to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I needed to add BeforeTargets="GetTargetFrameworksWithPlatformForSingleTargetFramework" in _CalculateIsVSTestTestProject. I'm not 100% sure if they can now be merged or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can still do a bit of simplification though. Let me try.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed a cleanup to get rid of ValidateExecutableReferencesCore, but still keeping _CalculateIsVSTestTestProject

@Youssef1313 Youssef1313 requested a review from dsplaisted July 18, 2025 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nohwnd nohwnd nohwnd left review comments

@baronfel baronfel Awaiting requested review from baronfel

@dsplaisted dsplaisted Awaiting requested review from dsplaisted

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate historical hacks for test projects that may be harmful for MTP projects
3 participants
@Youssef1313 @dsplaisted @nohwnd