Skip to content

Conversation

@rainersigwald
Copy link
Member

Reverts #8498 which should no longer be necessary now that 17.6 has rolled out broadly--it's everywhere I checked (official build, hosted agents, the pre queue).

@rainersigwald
Copy link
Member Author

I think the failure may be a bad interaction with #8488. @JanKrivanek can you investigate as a kitten task (but super low pri, and feel free to delegate back to me if you're swamped :)).

@JanKrivanek
Copy link
Member

I think the failure may be a bad interaction with #8488. @JanKrivanek Jan Krivanek FTE can you investigate as a kitten task (but super low pri, and feel free to delegate back to me if you're swamped :)).

Adding to the kitten queue

@JaynieBai
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@GangWang01
Copy link
Member

GangWang01 commented Jun 21, 2023

The error in this PR as below ever appeared in #8488 (comment). It's supposed src\MSBuild.Bootstrap\RedirectNuGetConsoleProcess.After.Microsoft.Common.targets could redirect the dotnet path properly. But from binlog Build.zip the task was not executed. No idea why.

##[error]stage1\bin\bootstrap\net7.0\MSBuild\NuGet.RestoreEx.targets(19,5): error : (NETCORE_ENGINEERING_TELEMETRY=Restore) An error occurred trying to start process 'D:\a\1\s\stage1\bin\bootstrap\dotnet' with working directory 'D:\a\1\s'. The system cannot find the file specified.

@GangWang01
Copy link
Member

@dfederm is #8488 missing calling the target RedirectNuGetConsoleProcess in src\MSBuild.Bootstrap\RedirectNuGetConsoleProcess.After.Microsoft.Common.targets?

@dfederm
Copy link
Contributor

dfederm commented Jun 21, 2023

@GangWang01 The target is defined as <Target Name="RedirectNuGetConsoleProcess" BeforeTargets="Restore">, so it should be executing.

@Forgind
Copy link
Contributor

Forgind commented Jun 21, 2023

What is importing that .targets file?

@dfederm
Copy link
Contributor

dfederm commented Jun 21, 2023

What is importing that .targets file?

It's copied to Current\Microsoft.Common.targets\ImportAfter, so the common targets import it via wildcard

@JanKrivanek
Copy link
Member

Interesting that the build seems to fail in the non-bootstrapped legs - which should be independent on the other change (adressing bootstrap).
So I guess something is flipped with RestoreUseStaticGraphEvaluation that makes the restore fail here?

@JanKrivanek
Copy link
Member

JanKrivanek commented Jun 21, 2023

This: https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj#L215-L232 is getting skipped in the failing legs
Red herring

The difference is in the Restore target that's being executed. For failed case it's comming from D:\a\1\s\stage1\bin\bootstrap\net7.0\MSBuild\NuGet.RestoreEx.targets, for analogous build without the change it comes from D:\a\1\s\stage1\bin\bootstrap\net7.0\MSBuild\NuGet.targets

@rainersigwald
Copy link
Member Author

What is importing that .targets file?

It's copied to Current\Microsoft.Common.targets\ImportAfter, so the common targets import it via wildcard

That pulls it in for projects that import common.targets, but not for solutions. @GangWang01 can you please try putting a copy of that file in bin/bootstrap/net7.0/MSBuild/Current/SolutionFile/ImportAfter/Microsoft.NuGet.ImportAfter.targets in addition to its current location?

@rainersigwald rainersigwald added this to the VS 17.8 milestone Jun 22, 2023
@GangWang01
Copy link
Member

What is importing that .targets file?

It's copied to Current\Microsoft.Common.targets\ImportAfter, so the common targets import it via wildcard

That pulls it in for projects that import common.targets, but not for solutions. @GangWang01 can you please try putting a copy of that file in bin/bootstrap/net7.0/MSBuild/Current/SolutionFile/ImportAfter/Microsoft.NuGet.ImportAfter.targets in addition to its current location?

It worked. #8960 fixed the failure. Please help to merge it to the work branch in this PR.

@rainersigwald
Copy link
Member Author

Thanks @GangWang01. LGTM (but it's my change too, so I'm not going to hit merge until somebody else looks at the current state).

@JanKrivanek
Copy link
Member

LGTM. It has enough approvals - I'm merging it now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants