Refactor build traversal #15093
Refactor build traversal #15093
Conversation
| </Target> | ||
|
|
||
| <!-- The initial shaking target for trimming down applicable projects for specified vertical --> | ||
| <Target Name="DetermineProjectsConfiguration" |
There was a problem hiding this comment.
We really weren't using this target? That's surprising.
There was a problem hiding this comment.
Oh, it was being used, but after your refactors, it is now handled by AnnotateAndFilterProjects
There was a problem hiding this comment.
Yeah, you're right. I'll fix up that commit message. I think I got half way through and realized it was no longer needed.
Change contract of cross-project target to return configuration rather than trying to construct a project item. This preserves the metadata of the original Project/ProjectReference. When the MSBuild task creates the TargetOutputs it will copy all the original metadata from the Project items and create the OriginalItemSpec metadata. From this we can simply transform back to a Project/ProjectReference and prepend Configuration to the AdditionalProperties. This fixes a case where projects were setting AdditionalProperties and we were squashing it. This exposed an issue in traversal where we were still setting TestTFM (previously this was squashed, only with other AdditionalProperties). To make it clearer how this worked I made the targets only return a property so that we don't accidentally squash any metadata set on the project. I've also cleaned up the targets to make things a bit more consistent and added comments.
ericstj
force-pushed
the
refactorTraversal
branch
from
77ac8bb
to
47b56ae
Compare
This was regressed by 8d70b2c.
| </Project> | ||
|
|
||
| <!-- Include all projects which have TestTFM from the supported set --> | ||
| <ProjectsToTest Include="@(Project)" Condition="'%(Project.Extension)'=='.csproj' And $([System.String]::new('%(Project.Identity)').Contains('.Tests'))"> |
There was a problem hiding this comment.
Previously we were only testing projects that ended with "tests" but I don't think it makes sense to filter this in multiple places. Let just let the builds file define the set of projects. I fixed the one case that wasn't working, RemoteExecutorConsoleApp: it was failing because we hardcoded .dll as the test extension. I now use TargetFileName. This will run xunit against the assembly which is a noop since it contains no tests. If we want to bring back this filtering let me know. /cc @weshaggard @mellinoe @joperezr
There was a problem hiding this comment.
Just as an FYI since it is related, I recently changed (in dev/eng) the way we calculate the property for IsTestProject to instead check if the project has a tests folder in it's path, and that is what we are using today to determine which projects to test.
There was a problem hiding this comment.
Yeah, I understand that, it still gets a false-positive for RemoteExecutorConsoleApp since that is src/Common/tests/System/Diagnostics/RemoteExecutorConsoleApp/RemoteExecutorConsoleApp.csproj.
There was a problem hiding this comment.
This is causing a lot more than just RemoteExecutorConsoleApp to "run" now (about 20 more assemblies based on my primitive ctrl-F searching), which I don't think is desirable. Before, the only assemblies we were unnecessarily running were things like "Performance" or "Stress" test assemblies, which technically do have test cases but are filtered via Xunit traits.
There was a problem hiding this comment.
Ok, don't you think we should filter those from Tests.builds rather than just filtering them from test execution.
There was a problem hiding this comment.
Or at least in one place rather than a side effect of many conditions in different places?
There was a problem hiding this comment.
Yes, I understand the idea; we should move the logic from here into tests.builds.
There was a problem hiding this comment.
Ok, I've opened https://github.com/dotnet/corefx/issues/15125
|
@dotnet-bot test Innerloop Ubuntu14.04 Debug Build and Test |
| <!-- Runs during traversal to select which projects and configurations of those projects to build --> | ||
| <Target Name="AnnotateAndFilterProjects" | ||
| BeforeTargets="BuildAllProjects;TestAllProjects" | ||
| Condition="!$(MSBuildProjectFullPath.EndsWith('.proj'))"> |
There was a problem hiding this comment.
I'm not sure I understand this condition. Don't you want this to work on proj files as well?
There was a problem hiding this comment.
I'd assume that Eric carried this over from the previous version. The only ".proj" file in the traversal at the time was build.proj and we didn't want to annotate / trim it. I'd think that it's now safe to remove that condition given other change recently.
|
|
||
| <ItemGroup> | ||
| <_Configuration Condition="'$(BuildConfigurations)' == ''" Include="$(BuildConfiguration)" /> | ||
| <!-- transform back to project --> | ||
| <ProjectWithConfiguration Include="@(_ProjectBestConfigurations->'%(OriginalItemSpec)')"> |
There was a problem hiding this comment.
Where does the filtering occur?
| </ItemGroup> | ||
| </Target> | ||
|
|
||
| <!-- TestAllProjects will run all tests according to TestTFM value we are filtering with --> | ||
| <Target Name="TestAllProjects" | ||
| AfterTargets="BuildAllProjects" | ||
| Condition="$(MSBuildProjectName.EndsWith('tests'))"> |
There was a problem hiding this comment.
@mellinoe we should make this conditions consistent with what do in tests.builds. I assume that tests.builds is the only project we have now that matches this pattern from a traversal prospective.
Refactor build traversal Commit migrated from dotnet/corefx@f3e8043
Change contract of cross-project target to return configuration rather
than trying to construct a project item. This preserves the metadata of
the original Project/ProjectReference. When the MSBuild task creates
the TargetOutputs it will copy all the original metadata from the
Project items and create the OriginalItemSpec metadata. From this
we can simply transform back to a Project/ProjectReference and prepend
Configuration to the AdditionalProperties. This fixes a case where
projects were setting AdditionalProperties and we were squashing it.
To make it clearer how this worked I made the targets only return a
property so that we don't accidentally squash any metadata set on the
project.
I found that we were never using the DetermineProjectsConfiguration
target so I deleted it.
I've also cleaned up the targets to make things a bit more consistent
and added comments.