dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.22k stars 1.35k forks source link

Items returned by tasks not considered for EmbedInBinlog #7665

Closed KirillOsenkov closed 2 years ago

KirillOsenkov commented 2 years ago

I'd expect this to embed C:\temp\1.txt into the binlog, but it doesn't:

<Project>

  <Target Name="Build">
    <MSBuild Projects="$(MSBuildThisFileFullPath)" Targets="GetItems">
      <Output TaskParameter="TargetOutputs" ItemName="EmbedInBinlog" />
    </MSBuild>
  </Target>

  <Target Name="GetItems" Returns="@(ResultItem)">
    <ItemGroup>
      <ResultItem Include="C:\temp\1.txt" />
    </ItemGroup>
  </Target>

</Project>

As a workaround to fix this, also need to add an explicit ItemGroup:

<Project>

  <Target Name="Build">
    <MSBuild Projects="$(MSBuildThisFileFullPath)" Targets="GetItems">
      <Output TaskParameter="TargetOutputs" ItemName="EmbedInBinlog" />
    </MSBuild>
    <ItemGroup>
      <EmbedInBinlog Include="@(EmbedInBinlog)" />
    </ItemGroup>
  </Target>

  <Target Name="GetItems" Returns="@(ResultItem)">
    <ItemGroup>
      <ResultItem Include="C:\temp\1.txt" />
    </ItemGroup>
  </Target>

</Project>

I think this line: https://github.com/dotnet/msbuild/blob/f1dae6ab690483458d37b8900f1d1e4a5fc72851/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs#L521

should also allow for TaskParameterMessageKind.TaskOutput

MeikTranel commented 2 years ago

Y'all can assign me this one - just have to do the dunkin' after ⬆️

MeikTranel commented 2 years ago

hey btw i've noticed this last week - does anyone else have issues with dependabot running on forks recently?https://github.com/MeikTranel/msbuild/pull/1/

rainersigwald commented 2 years ago

@MeikTranel it has happened on mine before but I was assuming/hoping that was a weird vestige of me having tested it there before configuring it here. Can you check this setting on your fork? https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuring-dependabot-version-updates#enabling-version-updates-on-forks

MeikTranel commented 2 years ago

Is it possible this dependabot.csproj is responsible? image

There's no disable there though.

Even under settings dependabot seems disabled: image

MeikTranel commented 2 years ago

Well good news is @KirillOsenkov was absolutely right - it really just was that TaskOutput that needed to be added to the filter.

image

On the other hand i'm having trouble writing a test for this one and i really think the binary log deserves some love there (only a single test confirming parallel and console loggers are roundtripping through binary logs - otherwise no test coverage for binary log contents).

I'd like some guidance regarding the BinaryLogs created by the TestEnvironment / ObjectModelHelper in the following code:

BinaryLogger_Tests.cs ```csharp [Fact] public void BinaryLoggerShouldEmbedFilesViaTaskOutput() { using var buildManager = new BuildManager(); var binaryLogger = new BinaryLogger() { Parameters = $"LogFile={_logFile}", CollectProjectImports = BinaryLogger.ProjectImportsCollectionMode.Embed }; var testProject = @" "; ObjectModelHelpers.BuildProjectExpectSuccess(testProject, binaryLogger); //Replay and verify file entries var logReader = new BinaryLogReplayEventSource(); var files = new List(); logReader.AnyEventRaised += (object sender, BuildEventArgs e) => { if (e is ProjectImportedEventArgs importArgs) { files.Add(importArgs.File); } }; logReader.Replay(_logFile); } ``` When i open the log written by the following code the entire thing looks like a light binlog - no parameter logging nothing. ![image](https://user-images.githubusercontent.com/6466560/171511652-11a3ba4d-9641-4f8b-87fd-ddd280ff1808.png) If i run `dotnet build /bl` over the same temporary project file everything is where it is expected - see above.
KirillOsenkov commented 2 years ago

could it be we need to set the verbosity to diagnostic inside BuildProjectExpectSuccess?

MeikTranel commented 2 years ago

I already tried setting BinaryLogger.Verbosity to Diagnostic to no avail - is there another verbosity setting somewhere i'm missing? I'm also not sure which of the APIs i'm supposed to use for running a simple project build with a binarylogger attached that i can parse for events later on. Maybe @rainersigwald has some guidance?

rainersigwald commented 2 years ago

There's no disable there though.

Even under settings dependabot seems disabled:

Asked internally, and got pointed to dependabot/dependabot-core#2804 which sure sounds related. The workaround suggested there is delete/recreate the fork, which is pretty heavyweight.

MeikTranel commented 2 years ago

Jesus somebody is pretty petty under that issue - how did they manage to link 1500 PRs to that issue :D:D

Have you looked at the problem regarding tests and binary logs ? Wanted to give it another look after work today.

Make sure to expand the spoiler section: https://github.com/dotnet/msbuild/issues/7665#issuecomment-1144206481

MeikTranel commented 2 years ago

I think i found a good solution to have some test coverage - sorry that it took so long - got super distracted and kind of forgot that i still had this laying around.