-
Notifications
You must be signed in to change notification settings - Fork 168
Use MSBuild Logging APIs for Task output instead of StdOut #933
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
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.
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.
@Forgind is there a way we can avoid making a breaking change here?
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.
baronfel didn't come up with a way, and he knows this part much better than I do. I can try to look around, but I have little hope of success.
Commenter does not have sufficient privileges for PR 933 in repo microsoft/sbom-tool |
@@ -1,45 +1,76 @@ | |||
steps: | |||
- task: UseDotNet@2 | |||
displayName: 'Use .NET Core' | |||
displayName: Use .NET Core |
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.
Please revert changes in this file
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.
Why do you dislike these changes? Is it just because they're unrelated to the PR? They eliminated a bunch of warnings in the CI pipeline, which seems like a plus to me.
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'm seeing a lot of commented code and unnecessary changes (like quotes and dotnet --info) which I don't like so it's hard to tell which of these changes are actually functional. Can you revert the unnecessary changes/ commented code at a minimum and I'll consider whatever's left over?
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.
Yeah; I can probably do 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.
I think it's about as small a change as I can make it. I considered combining restore/build or even restore/build/test, but there isn't much harm to keeping them separate.
@@ -24,17 +24,11 @@ | |||
<PackageReference Include="Serilog.Extensions.Hosting" /> | |||
<PackageReference Include="Serilog.Sinks.Console" /> | |||
<PackageReference Include="Spectre.Console.Cli" /> | |||
<PackageReference Include="System.IO.FileSystem.AccessControl" /> |
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.
What is the reason for the changes in this file?
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.
When I built locally, I got an error that some package references might not be needed, and it pointed here. I deleted it, and it still built, so it seems to have been right.
@@ -10,7 +10,6 @@ | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="Serilog.Sinks.Console" /> | |||
<PackageReference Include="System.IO.FileSystem.AccessControl" /> |
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.
What is the reason for the change in this file?
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.
Same as previous
{ | ||
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.
@Forgind is there a way we can avoid making a breaking change here?
@@ -146,80 +149,6 @@ private void InspectPackageIsWellFormed(bool isManifestPathGenerated = true) | |||
} | |||
} | |||
|
|||
[TestMethod] | |||
public void SbomGenerationSucceedsForDefaultProperties() |
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.
@Forgind can you please provide your reasoning for deleting this tests here for visibility for the whole team?
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.
Some of the test code multitargets to net8.0 and net472. There's a target there that only executes for net472. It builds another project that only targets net8.0—and it passed net8.0 as the targetframework with which to build that project—then copies some of its assets over and tries to use them. That isn't a supported scenario and never has been. End-to-end tests should only exist if they're testing supported scenarios. It now fails because it copies in baronfel's new thing (that uses APIs that don't exist in net472). I don't think it really needs to execute them (might be wrong on that) just pack properly, but that involves loading the assembly, which causes a missing method exception.
If we wanted to avoid deleting the tests, what I'd do is either:
- Make the relevant project multitarget to net8.0.
- It also depends on a lot of other projects (and packages) that target net8.0, so we'd either need to make those conditional or have them also target net472.
- This would be reasonably robust to additional changes.
- Choose specific files to copy or not copy to the net472 project's output directory.
- Figuring out which files would be...tricky...and it would still be an unsupported scenario.
- This would be very fragile. A small change could break it and lead to just as much confusion as I faced.
I'm not terribly fond of either option. The second would just be kicking the can down the road, as far as I'm concerned, and the first would likely require major (infrastructure-y) changes.
Waiting on .NET team to get back to this PR |
Last I checked (in April), this was just waiting on approval. |
And I just resolved the merge conflict. |
I don't think that's accurate, some of my previous comments have not been resolved (like reverting changes to build-test-tool-template.yaml) and I believe this PR also contains a breaking change, which would need to be addressed |
The breaking change part was discussed already in this thread. It sounded like you wanted to avoid the breaking change, but baronfel did try to mitigate as much as possible, so I don't think there's much more to do there unless you'd rather just close this PR. As to the build-test-tool-template.yaml changes, I minimized those, as you can see from my last two commits. It's been a couple months, but my vague recollection is that the previous version chose the wrong version of the .NET SDK, and this PR revealed that. I'd rather not un-fix a bug. |
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.