C#: Implement correct behavior for dotnet build tracing

[criemen]

For proper C# tracing, dotnet build needs the parameter
/p:UseSharedCompilation=false. However, we can't pass that to the other
subcommands of dotnet, therefore we need to figure out which subcommand
of dotnet is being invoked.

[tamasvajk]

Looks plausible to me.

[michaelnebel]

Looks plausible to me!

[michaelnebel]

Do you know, if this will cause any problems if '/p:UseSharedCompilation=false' is already set as a part of the command line arguments?

[criemen]

I believe the last specified option wins, and I just tried this and it worked:

 ✘ criemen@Corneliuss-MacBook-Pro  ~/test  dotnet new console
The template "Console App" was created successfully.

Processing post-creation actions...
Running 'dotnet restore' on /Users/criemen/test/test.csproj...
  Determining projects to restore...
  Restored /Users/criemen/test/test.csproj (in 120 ms).
Restore succeeded.

 criemen@Corneliuss-MacBook-Pro  ~/test  dotnet clean
Microsoft (R) Build Engine version 17.1.1+a02f73656 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 07/21/2022 11:00:30.
     1>Project "/Users/criemen/test/test.csproj" on node 1 (Clean target(s)).
     1>CoreClean:
         Creating directory "obj/Debug/net6.0/".
     1>Done Building Project "/Users/criemen/test/test.csproj" (Clean target(s)).

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.79
 criemen@Corneliuss-MacBook-Pro  ~/test  dotnet build /p:UseSharedCompilation=false /p:UseSharedCompilation=false
Microsoft (R) Build Engine version 17.1.1+a02f73656 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
  test -> /Users/criemen/test/bin/Debug/net6.0/test.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:03.00
 criemen@Corneliuss-MacBook-Pro  ~/test 

[michaelnebel]

Thank you! 😄
Great work!

[hvitved]

Thanks a lot for doing all the work that lead to this, @criemen .

[jaredpar]

Using UseSharedCompilation=false will significantly slow down builds. On large projects this can cause a 2-5X increase in build time. Pushing this on every CodeQL consumer is likely causing a substantial increase in build cost. This is further compounded by the CodeQL documentation recommending customers set this property without providing context to the downsides of doing this.

Further the manner in which this is done is causing downstream implications to other teams. This is being added as a global property which means it filters down to tasks, targets and sub-builds which do not anticipate seeing this. It's presently causing a number of internal teams errors because it's filtering into commands that do not support it.

Can we get some explanation of why this was done? Did you reach out to the compiler team to get a more supported way of working around whatever problem you were hitting?

[tamasvajk]

@jaredpar CodeQL is intercepting calls to csc to record the arguments that are used for the call. These arguments are then passed to our extractor to create a CSharpCompilation. If UseSharedCompilation=false is not added to the build command and the compilation server instance is already started, then we’re not able to intercept the csc calls. (If the server is not yet started, then we might end up intercepting more csc calls than we should have.)

We used to have a .NET profiler based solution, but that’s no longer available. Our old build tracer wasn’t compatible with newer dotnet versions and the new tracer gives us greater flexibility over the command line.

You’re raising a valid point regarding performance. We’re aware of this issue, and investigating other approaches that could potentially improve the analysis times. We’ve created an internal issue to improve the documentation around UseSharedCompilation=false. Note that this PR ensures that UseSharedCompilation=false is always injected into builds, prior to this change, the CodeQL documentation has always recommended this for C# projects using dotnet build or msbuild for the reasons above.

Could you give more detail on failures that are caused by UseSharedCompilation=false? We’ve recently received feedback on other channels regarding failures with dotnet test. This issue is being fixed in #13387. Are there more use-cases that we should look into?

[baronfel]

Consider attaching a logger to the build instead to gather the CommandLineArgs passed to the Csc task. I would suggest writing and attaching your own MSBuild Logger (adding logger CLI arguments automatically in the same way you've added SharedCompilation arguments). Your logger could listen to TaskStarted/TaskFinished events and wait for invocations of the Csc task, then write out the values of the CscCommandLineArgs MSBuild Items in whatever format/location you need without otherwise negatively impacting the users chosen build. Other teams in Microsoft (e.g. AzDo CI system task authors) have done this to seamlessly gather build telemetry again without impacting the users builds.

[jaredpar]

These arguments are then passed to our extractor to create a CSharpCompilation

Curious: how do you all create and use this? Just based on the code here it would seemingly need to be on the same machine as the build. In that case, given you're running an analyisis engine, why are you not just writing a Roslyn based analyzer? There are always risks and issues that come with recreating a Compilation after the fact so curious why you chose the current approach over just hooking into build where most of the heavy lifting is done for you.

As @baronfel mentioned command line arguments are easy to pull from a build. There are several different ways to get command line arguments out of say a binary log that don't change the build performance so dramatically.

Could you give more detail on failures that are caused by UseSharedCompilation=false? We’ve recently received feedback on other channels regarding failures with dotnet test.

This is source of problems I'm seeing right now.

[tamasvajk]

Thank you for the pointers towards MsBuild loggers. I created an internal issue to explore this approach.

Curious: how do you all create and use this? Just based on the code here it would seemingly need to be on the same machine as the build. In that case, given you're running an analyisis engine, why are you not just writing a Roslyn based analyzer?

Yes, the build and the analysis (extraction) need to happen on the same machine. I think the current approach was chosen based on its ease of maintenance, simplicity and robustness. We need to support most (all) versions of csc, which can be achieved by shipping a single variant of the extractor that depends on the latest version of Roslyn.

[jaredpar]

Is the code where you do the analysis available anywhere? Interested in looking how you're creating the Compilation instances.

[tamasvajk]

You can find the main entry point here and the compilation is created here.

If you open this folder in VS Code, you can use the below launch.json to debug the application:

{
    "version": "0.2.0",
    "configurations": [
        {
            "name": ".NET Core Launch (console)",
            "type": "coreclr",
            "request": "launch",
            "preLaunchTask": "dotnet build",
            "program": "${workspaceFolder}/extractor/Semmle.Extraction.CSharp.Driver/bin/Debug/net7.0/Semmle.Extraction.CSharp.Driver.dll",
            "args": [
                "-target:library",
                "/noconfig",
                "/r:System.Private.CoreLib.dll",
                "/r:System.dll",
                "/r:System.Core.dll",
                "/r:System.Runtime.dll",
                "/r:System.Console.dll",
                "/r:System.Linq.dll",
                "${workspaceFolder}/ql/test/library-tests/csharp7/CSharp7.cs"
            ],
            "cwd": "${workspaceFolder}/ql/test/library-tests/csharp7/",
            "console": "internalConsole",
            "stopAtEntry": true
        }
    ]
}