Enable solution filtering with -graph option

[Forgind]

Fixes #7058

Context

Solution filters operate in MSBuild by modifying the list of projects that "should build." This list is normally constructed in SolutionProjectGenerator, but for -graph builds, it's in GraphBuilder instead. This was hard to notice because it didn't actually cause a build failure because the slnf tries to get out of the way as quickly as it can when you try to build it, which meant the graph build simply built the full sln. This corrects the slnf filtering behavior with -graph.

Changes Made

Filter the list of projects that "should build" when using -graph.

Testing

I built MSBuild.dev.slnf before and after the change as well as with and without graph. Eyeballing it, there were about the same number of projects built with the change and -graph as without the change and without -graph. There were substantially more with -graph but without the change.

[jeffkl]

You’ll need to leave this as-is until your change ships in a build used by Source Build right?

[Forgind]

If Source Build gets this change in Build.props, wouldn't it also get the fix?

[jeffkl]

Yeah but source build doesn’t use current MSBuild to build (as far as I know). I think it uses our build.cmd which uses a pretty recent version but will lag behind a little until this change is committed and inserted to VS and .NET SDK. Right?

[Forgind]

I know very little about source build, so I have no idea. Pushed the change 🙂 Thanks!

[jeffkl]

It looks like it runs build.sh:

msbuild/eng/common/templates/steps/source-build.yml

Line 71 in 764b54b

${{ coalesce(parameters.platform.buildScript, './build.sh') }} --ci \

build.sh installs the .NET SDK version based on what's in global.json:

msbuild/eng/Versions.props

Line 34 in 764b54b

<DotNetCliVersion>$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(MSBuildThisFileDirectory)..\global.json')), '"dotnet": "([^"]*)"').Groups.get_Item(1))</DotNetCliVersion>

So until we're using a version of .NET SDK with this change, we'll need to leave Static Graph restore off for Source Build. Might be worth opening a tracking issue to do that if you haven't already.

[rainersigwald]

The version of MSBuild used in source-build isn't directly controlled by our repo at all--it's a property of sourcebuild itself. The job that runs in our PR checks is only a simulation and I don't think it's sufficient on this front to be a good canary for when we're ready to remove this.

[jeffkl]

Thanks for fixing this!

[rainersigwald]

Please add a test to cover this case. Also, does this preserve the property of our solution filter handling that projects referenced by projects in the .slnf are built?

[Forgind]

Seems to. I modified MSBuild.Dev.slnf to be:

{
  "solution": {
    "path": "MSBuild.sln",
    "projects": [
      "src\\Build.OM.UnitTests\\Microsoft.Build.Engine.OM.UnitTests.csproj",
      "src\\Build.UnitTests\\Microsoft.Build.Engine.UnitTests.csproj"
    ]
  }
}

and it built fine (including things other than M.B.E.OM.UT and M.B.E.UT).

[rainersigwald]

Please don't move code and edit code in the same commit if at all possible; it makes the diff very hard to parse. I manually reassembled this real diff:

--- 
+++ 
@@ -1,15 +1,51 @@
         /// <summary>
         /// Test that a solution filter file excludes projects not covered by its list of projects or their dependencies.
         /// </summary>
         [Fact]
         public void SolutionFilterFiltersProjects()
         {
             using (TestEnvironment testEnvironment = TestEnvironment.Create())
             {
-                TransientTestFolder folder = testEnvironment.CreateFolder(createFolder: true);              
+                TransientTestFolder folder = testEnvironment.CreateFolder(createFolder: true);
                 TransientTestFolder classLibFolder = testEnvironment.CreateFolder(Path.Combine(folder.Path, "ClassLibrary"), createFolder: true);
                 TransientTestFolder classLibSubFolder = testEnvironment.CreateFolder(Path.Combine(classLibFolder.Path, "ClassLibrary"), createFolder: true);
                 TransientTestFile classLibrary = testEnvironment.CreateFile(classLibSubFolder, "ClassLibrary.csproj",
                     @"<Project>
                   <Target Name=""ClassLibraryTarget"">
                       <Message Text=""ClassLibraryBuilt""/>
@@ -23,12 +59,13 @@
                     @"<Project DefaultTargets=""SimpleProjectTarget"">
                   <Target Name=""SimpleProjectTarget"">
                       <Message Text=""SimpleProjectBuilt""/>
                   </Target>
                   </Project>
                     ");
+
 
                 // Slashes here (and in the .slnf) are hardcoded as backslashes intentionally to support the common case.
                 TransientTestFile solutionFile = testEnvironment.CreateFile(simpleProjectFolder, "SimpleProject.sln",
                     @"
                     Microsoft Visual Studio Solution File, Format Version 12.00
                     # Visual Studio Version 16
@@ -82,16 +119,22 @@
                 SolutionFile solution = SolutionFile.Parse(filterFile.Path);
                 ILoggingService mockLogger = CreateMockLoggingService();
                 ProjectInstance[] instances = SolutionProjectGenerator.Generate(solution, null, null, _buildEventContext, mockLogger);
                 instances.ShouldHaveSingleItem();
 
                 // Check that dependencies are built, and non-dependencies in the .sln are not.
-                MockLogger logger = new MockLogger(output);
+                MockLogger logger = new(output);
                 instances[0].Build(targets: null, new List<ILogger> { logger }).ShouldBeTrue();
                 logger.AssertLogContains(new string[] { "SimpleProjectBuilt" });
                 logger.AssertLogDoesntContain("ClassLibraryBuilt");
+
+                // Test that the same works for graph builds
+                string log = RunnerUtilities.ExecMSBuild(filterFile.Path + " -graph", out bool success);
+                success.ShouldBeTrue();
+                log.ShouldContain("SimpleProjectBuilt");
+                log.ShouldNotContain("ClassLibraryBuild");
             }
         }
 
         [Theory]
         [InlineData(@"
                 {

[rainersigwald]

I think it would be fairly difficult to understand why this test fails when it fails for only the graph case. Did you find that to be true when you ran this without the fix?

There's a bunch of setup code here so a totally separate test doesn't seem justified, but at the very least a graphBuild: true/false theory would help.

[Forgind]

I was waffling on this a little. We have some tests in the theory model and some more like this where, after setting up, it runs multiple tests in a row on the same thing. I thought this was more appropriate for the latter because most of it is boring setup. It's hard for me to say how debuggable it is when I know the answer ahead of time. My first priority is getting something that will fail if someone breaks this. I honestly doubt anyone other than me will be trying to debug this in the near future...

[rainersigwald]

Why does this invoke msbuild.exe rather than using the API like the one above does?

[Forgind]

Using the API is better, but this was something I mostly knew how to do already. If you feel strongly, I can try to figure out the graph API. I would guesstimate an hour or two.