-
Notifications
You must be signed in to change notification settings - Fork 146
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
base: main
Are you sure you want to change the base?
Conversation
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. |
src/Microsoft.Sbom.Extensions.DependencyInjection/MSBuildLogger.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Sbom.Extensions.DependencyInjection/ServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs
Outdated
Show resolved
Hide resolved
I've attached a zipped binlog of the tool working with these changes. The detailed logs are in the binlog as expected. To the end user, it looks like this:
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 sbom-tool/src/Microsoft.Sbom.Api/Workflows/SBOMGenerationWorkflow.cs Lines 240 to 242 in 3d1a59f
|
src/Microsoft.Sbom.Api/Entities/output/ValidationResultGenerator.cs
Outdated
Show resolved
Hide resolved
/azp run |
...oft.Sbom.Extensions.DependencyInjection/Microsoft.Sbom.Extensions.DependencyInjection.csproj
Outdated
Show resolved
Hide resolved
Please feel free to change this |
8f4c123
to
843d8a3
Compare
src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Sbom.Extensions.DependencyInjection/ServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this 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
src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs
Outdated
Show resolved
Hide resolved
71efc73
to
432a948
Compare
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. |
@sfoslund the loop is coming from a circular dependency in the DI configuration. |
Alright, fixed the loop by using the strategy from I am now completely satisfied with this PR. whew! |
/azp run |
{ | ||
services | ||
.AddSingleton<IConfiguration, Configuration>() | ||
.AddTransient(_ => FileSystemUtilsProvider.CreateInstance(CreateLogger(logLevel))) | ||
.AddTransient(x => |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Looks like we're seeing some CI unit test failures with this change: https://dev.azure.com/mseng/1ES/_build/results?buildId=29662326&view=logs&j=aeb7b77f-2f5c-5c5e-fb92-f326324a83fb&t=947a9a91-a289-5b68-4a98-262ba6071be8 |
Also @DaveTryon do you know why the 'Check for API changes' commentor check is failing? |
…mTask's Logging infrastructure Also generate binlogs for the tests to allow precise investigation of the builds
…are rather frequent
85b9746
to
c9fc1a5
Compare
…cumentation comments to clarify intended usage of members.
c9fc1a5
to
649d0fc
Compare
…t assembly load failures in the target and target tests
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
EDIT: I've confirmed this happens with a nuget package built from
|
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:
TaskLoggingHelper
's logging methodsAlong the way, I've made it easier to test the post-conditions of the Task on both net472 and .NET.