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

VMRs project filtering mechanism is suboptimal #4047

Open
ViktorHofer opened this issue Jan 29, 2024 · 4 comments
Open

VMRs project filtering mechanism is suboptimal #4047

ViktorHofer opened this issue Jan 29, 2024 · 4 comments

Comments

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 29, 2024

Many repositories inside the VMR use Arcade's filtering mechanism to filter out (TFMs and) projects via the ExcludeFrom... properties. The problem with that is that it requires to evaluate the to be filtered out project. This leads to several issues:

  1. Poor performance as at least one evaluation is kick-off per to be filtered out project.
  2. Doesn't support static graph builds and only supports static graph restore with hacks.
  3. More verbose logs and binlogs as the excluded projects are part of the build graph.
  4. Hacks. Due to the evaluation of excluded projects, referenced msbuild sdks are restored and imported. This is the reason why the VMR has this big hack: https://github.com/dotnet/dotnet/tree/main/eng/tools/EmptySdk & https://github.com/dotnet/dotnet/blob/a3488dad389cab8b740939b9d2e9f49ff7dd4b43/repo-projects/Directory.Build.props#L177. If the excluded project wouldn't be evaluated, that msbuild sdk resolver would never see a request to download and import that specific msbuild sdk.

Alternatives:

  1. Solution filter files (slnf). Some repositories use solution filter files to only build a subset of their repository. There are a couple of downsides with that approach:
    • Static. Solution filter files specify the projects that shouldn't filtered out statically. Meaning, you need one file per filtering rule. For the VMR build this means that repositories need to define slnf files for the source-only build, when not building source-only but excluding tests, when not building source-build without excluding tests, etc. This becomes unmaintainable relatively quickly.
    • There's currently a defect in static graph restore that causes filtered-out projects to still be restored (and therefore, evaluated).
  2. Microsoft.Build.Traversal. This is an msbuild sdk and is heavily used by big repositories like runtime which want to have full control over their build graph based on different inputs. There's really only one downside to this approach: You can't open a project that uses that msbuild sdk directly via tools like Visual Studio or Visual Studio Code. Tools like slngen can on-the-fly generate a solution file from a Traversal project, but that requires to be inside a command prompt. You can of course check the solution file that is generated via slngen in but then you need to make sure that the traversal project (i.e. Build.proj) stays in sync with the generated sln file.
  3. Combination of solution file and Microsoft.Build.Traversal. See my POC that enables that in dotnet/xdt: dotnet/xdt@93902be. MSBuild generates a .metaproj from a given solution file which can be mutated by adding a Directory.Solution.props/targets file with custom msbuild logic. This would be the best option for small to medium sized repositories which want to continue using a solution file but with a more dynamic filtering mechanism than what slnfs offer. Unfortunately there are couple of downsides when using that filtering mechanism today:

I've been dealing with this topic for years now as we needed a solid way to filter projects in XXXL repositories like dotnet/runtime. It's clear to me that neither the ExcludeFrom... Arcade infrastructure, nor slnfs satisfy our needs. We need a highly performant filtering mechanism (=> no evaluations) with the flexibility to change the set of projects based on different inputs (i.e. DotNetBuildSourceOnly vs DotNetBuild vs DotNetBuildTests).

Repositories like runtime which already use option 2 don't need to change anything. They already declare the set of projects to be built based on various inputs. Ones that use slnfs or Arcade's infra would highly benefit from option 3. Therefore I propose that we iterate on option 3 to make it work without kicking-off evaluations (might need a small product change in msbuild's metaproj generation) and then convert the existing repositories to depend on that.

cc @mmitche @jeffkl @ericstj @MichaelSimons

Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@mmitche
Copy link
Member

mmitche commented Jan 29, 2024

Does the traversal approach work with TFM filtering? How much work would it be to implement such an approach in all the repos? In other words, the approach today does work pretty transparently for most repos without actual changes in those repos.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jan 29, 2024

Does the traversal approach work with TFM filtering?

In regards to TFM filtering, the ExcludeFrom... infrastructure is currently used when all project's TFMs are filtered out: https://github.com/dotnet/arcade/blob/cf685b45c1a9268dae9cec2478948c143a695168/src/Microsoft.DotNet.Arcade.Sdk/tools/TargetFrameworkFilters.BeforeCommonTargets.targets#L16-L17

For this to work, these projects need to be filtered out manually at the traversal point. I.e.:

<!-- Exclude .NET Framework only projects when building source-only. -->
<ItemGroup Condition="'$(DotNetBuildSourceOnly)' == 'true'">
  <ProjectReference Remove="a/b/c/some-netfx-only-project.csproj" />
</ItemGroup>

How much work would it be to implement such an approach in all the repos? In other words, the approach today does work pretty transparently for most repos without actual changes in those repos.

With the design that I have in mind, repositories would only need to add a Directory.Solution.targets file to exclude projects from the generated metaproj and declare the to be excluded projects in there. We could still provide a default set of projects to be filtered out via an eng/common file:

eng/common/RepoProjectFilter.props

<Project>

  <!-- Keep glob patterns in sync with the ones in the Arcade SDK's Tests.props file. -->
  <ItemGroup Condition="'$(DotNetBuildTests)' != 'true'">
    <ProjectReference Remove="$(MSBuildThisFileDirectory)**/*.PerformanceTests.csproj" />
    <ProjectReference Remove="$(MSBuildThisFileDirectory)**/*.IntegrationTests.csproj" />
    <ProjectReference Remove="$(MSBuildThisFileDirectory)**/*.Tests.csproj" />
    <ProjectReference Remove="$(MSBuildThisFileDirectory)**/*.UnitTests.csproj" />
  </ItemGroup>

</Project>

Directory.Solution.targets

<Project>

  <Import Project="$(MSBuildThisFileDirectory)eng/common/RepoProjectFilter.props" />

  <!-- Exclude VS components -->
  <ItemGroup Condition="'$(DotNetBuild)' == 'true'">
    <ProjectReference Remove="..." />
  </ItemGroup>

  <!-- Exclude test utility projects -->
  <ItemGroup Condition="'$(DotNetBuildTests)' != 'true'">
    <ProjectReference Remove="..." />
  </ItemGroup>  

</Project>

Basically the ExcludeFrom... properties in the respective projects would be moved and transformed into items in the Directory.Solution.targets. This is a quite simple change for most repositories. I don't expect this to be too costly.

@mmitche
Copy link
Member

mmitche commented Jan 29, 2024

For this to work, these projects need to be filtered out manually at the traversal point. I.e.:

That may end up being a pretty large set to special case. Also keep in mind that the filter is not based on DotNetBuildFromSource. It's just a generic filter X->Filter->Y. So if you had to special case all project references it would be fragile to do so based on DotNetBuildFromSource

I would prototype what this may look like in repos like roslyn.

ViktorHofer added a commit to dotnet/arcade that referenced this issue Dec 12, 2024
First of all, I hate this whole filtering model. It's super hacky and constantly causes issues. dotnet/source-build#4047 tracks this.

Here we need to also empty the `ProcessFrameworkReferences` target as it still runs as it has a Before/AfterTargets hook. Keep it running is causing issues when RuntimeIdentifier is set to a custom value even though the project should be excluded.

Unblocks dotnet/sdk#45362
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

4 participants