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

Use MSBuild Logging APIs for Task output instead of StdOut #933

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

baronfel
Copy link
Member

Fixes #712
Fixes #616

This is a refresh of #730 that I've gotten the tests to green up on. Along the way I think I've also addressed #616, which was a consequence of way the dependencies of the task were initialized in the .NET task.

Since the majority of the tool uses Serilog, the strategy here is to integrate MSBuild's logging APIs into Serilog's logging configuration where possible. This means, broadly, two things:

  • creating and using an MSBuild Logger that converts Serilog's messages to calls to the TaskLoggingHelper's logging methods
  • ensuring that the Verbosity selected in the Task is used when configuring Serilog

Along the way, I've made it easier to test the post-conditions of the Task on both net472 and .NET.

@baronfel baronfel requested a review from a team as a code owner February 16, 2025 20:22
@baronfel
Copy link
Member Author

One big note - I'm using .NET SDK 9.0.200, where some of the Roslyn analyzers have been tweaked/fixed and so are reporting things that I had to fix in order to get the repo to compile.

@baronfel
Copy link
Member Author

baronfel commented Feb 17, 2025

I've attached a zipped binlog of the tool working with these changes. The detailed logs are in the binlog as expected.

sbom-pack.zip

To the end user, it looks like this:

> dotnet pack -bl
Restore complete (0.3s)
  sbom-tests succeeded with 1 warning(s) (5.6s) → bin\Release\net9.0\sbom-tests.dll
    E:\code\nuget-packages\microsoft.sbom.targets\3.1.1-preview.0.6\build\Microsoft.Sbom.Targets.targets(57,5): warning : Deleting pre-existing folder E:\Code\Scratch\sbom-tests\bin\Release\sbom-tests.1.0.0.4b5fcd2f.temp\_manifest as DeleteManifestDirIfPresent is 'true'.

Build succeeded with 1 warning(s) in 6.2s

I'd like to dig in a bit more about the spurious warning, I expect that's a mismatched warning regex or something on MSbuild's side.

This warning is coming from the SBOM targets - specifically from

log.Warning(
$"Deleting pre-existing folder {rootManifestFolderPath} as {Constants.DeleteManifestDirBoolVariableName}" +
$" is 'true'.");
. Is this really a warning? A user asked to delete a manifest if it was already there, and we're doing what they asked? This feels more like a debug/diagnostic to me. The big problem here is twofold:

  • many users run MSBuild with TreatWarningsAsErrors enabled
  • this particular warning, because it's coming from deep in the Serilog stack and not from direct use of MSBuild Logging APIs, doesn't have a Code associated with it, so users can't use MSbuild's knobs to disable/hide this warning when it does appear

@sfoslund
Copy link
Member

/azp run

@DaveTryon DaveTryon added the .NET Pull requests that update .net code label Feb 18, 2025
@DaveTryon
Copy link
Contributor

log.Warning(
$"Deleting pre-existing folder {rootManifestFolderPath} as {Constants.DeleteManifestDirBoolVariableName}" +
$" is 'true'.");

. Is this really a warning? A user asked to delete a manifest if it was already there, and we're doing what they asked? This feels more like a debug/diagnostic to me. The big problem here is twofold:

  • many users run MSBuild with TreatWarningsAsErrors enabled
  • this particular warning, because it's coming from deep in the Serilog stack and not from direct use of MSBuild Logging APIs, doesn't have a Code associated with it, so users can't use MSbuild's knobs to disable/hide this warning when it does appear

Please feel free to change this log.Warning to log.Info

@baronfel baronfel force-pushed the msbuild-based-logging branch from 8f4c123 to 843d8a3 Compare February 23, 2025 23:04
@baronfel
Copy link
Member Author

I've taken another pass at this, addressing the feedback (thanks folks!) and I'm much happier with how this integration looks now. There's only one actually functional gap, and I need a bit of direction on where I should go with that.

Copy link
Member

@sfoslund sfoslund left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, other than the 2 comments I added + those that others have already added

@baronfel
Copy link
Member Author

I've just rebased against main - the major conflict was that there's now SPDX3.0 configurations in the DI registration.

I've addressed all outstanding feedback I think - the last thing is to check out some hangs that @sfoslund reported to me today in the CLI tool itself.

@baronfel
Copy link
Member Author

@sfoslund the loop is coming from a circular dependency in the DI configuration. ILogger requires knowing the verbosity (from InputConfiguration) in order to initialize, but reading the InputConfiguration requires IConfigurationBuilder<T>, which requires ConfigFileParser, which requires IFileSystemUtils, all implementations of which require ILogger.

@baronfel
Copy link
Member Author

Alright, fixed the loop by using the strategy from main - the FileSystemUtils get a default logger with an expected verbosity.
Also fixed more warnings coming from component detection, but in a way that remaps all of them cleanly to MSBuild Information without impacting the tool experience.

I am now completely satisfied with this PR. whew!

@sfoslund
Copy link
Member

/azp run

{
services
.AddSingleton<IConfiguration, Configuration>()
.AddTransient(_ => FileSystemUtilsProvider.CreateInstance(CreateLogger(logLevel)))
.AddTransient(x =>
Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that this will create a breaking change for API users, as they will now have to inject a logger similarly to how we are now doing it in Program.cs, is that right? This is okay with me as our next release will be a breaking change anyways, but tagging @DaveTryon for visibility

Copy link
Member Author

Choose a reason for hiding this comment

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

Oof, good call - I didn't realize that the dependency injection project was public API surface area. Let me noodle on how that could be fixed.

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've pushed a commit that minimizes the API surface area changes, and adds a ton of xmldoc that instructs users on the new requirement of registering their own ILogger. That won't stop someone that just updates and then runs tests from failing, but maybe that will help mitigate somewhat.

@sfoslund
Copy link
Member

@sfoslund
Copy link
Member

Also @DaveTryon do you know why the 'Check for API changes' commentor check is failing?

@baronfel baronfel force-pushed the msbuild-based-logging branch from 85b9746 to c9fc1a5 Compare March 1, 2025 19:16
…cumentation comments to clarify intended usage of members.
@baronfel baronfel force-pushed the msbuild-based-logging branch from c9fc1a5 to 649d0fc Compare March 1, 2025 19:23
@baronfel
Copy link
Member Author

baronfel commented Mar 2, 2025

The last failures I think are real - I've seen them when packing my own projects using the newly-built nuget package manually. I'll test with a package from main to compare. The failures are a double-access to the manifest.spdx.json - I've only seen them happen for the spdx_3.0 pathway, but it's possible the 2.2 pathway could also be impacted? The stack looks like this:

System.IO.IOException: The process cannot access the file 'E:\Code\sbom-tool\test\Microsoft.Sbom.Tool.Tests\bin\Debug\net8.0\TestResults\Deploy_chusk 20250301T213843_15624\E2E_GenerateManifest_GeneratesManifest_ReturnsZeroExitCode\_manifest\spdx_3.0\manifest.spdx.json' because it is being used by another process.
   at Microsoft.Win32.SafeHandles.SafeFileHandle.CreateFile(String fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.Open(String fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
   at System.IO.Strategies.OSFileStreamStrategy..ctor(String path, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
   at System.IO.Strategies.FileStreamHelpers.ChooseStrategyCore(String path, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
   at System.IO.FileInfo.OpenRead()
   at Microsoft.ComponentDetection.Common.LazyComponentStream.SafeOpenFile()

EDIT: I've confirmed this happens with a nuget package built from main too (version 3.1.1-preview.0.8 as of this comment). I believe there's a bug in the tool itself independent of my PR.

dotnet pack -bl
Restore complete (0.8s)
  sbom-tests                                                                                  GenerateSbomTarget (7.1s)
information]Finding components...                                                                                (7.3s)
##[information]No instructions received to scan docker images.                                                   (7.4s)
##[information]Starting enumeration of "E:\\Code\\Scratch\\sbom-tests"
##[information]Enumerated 50 files and 23 directories in 00:00:00.0149485
##[warning]Unhandled exception caught when trying to open "E:\\Code\\Scratch\\sbom-tests\\bin\\Release\\sbom-tests.1.0.0.98080a7c.temp\\_manifest\\spdx_3.0\\manifest.spdx.json"
System.IO.IOException: The process cannot access the file 'E:\Code\Scratch\sbom-tests\bin\Release\sbom-tests.1.0.0.98080a7c.temp\_manifest\spdx_3.0\manifest.spdx.json' because it is being used by another process.
   at Microsoft.Win32.SafeHandles.SafeFileHandle.CreateFile(String fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options)
   at Microsoft.Win32.SafeHandles.SafeFileHandle.Open(String fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
   at System.IO.Strategies.OSFileStreamStrategy..ctor(String path, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
   at System.IO.Strategies.FileStreamHelpers.ChooseStrategyCore(String path, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
   at System.IO.FileInfo.OpenRead()
   at Microsoft.ComponentDetection.Common.LazyComponentStream.SafeOpenFile()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Pull requests that update .net code
Projects
None yet
4 participants