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

Reënable static graph restore #8856

Merged
merged 2 commits into from Jul 13, 2023

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
Contributor

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
Contributor

@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
Member

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
Contributor

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

@JanKrivanek JanKrivanek merged commit 4598629 into main Jul 13, 2023
8 checks passed
@JanKrivanek JanKrivanek deleted the revert-8498-turn-off-static-graph-restore branch July 13, 2023 05:49
@JanKrivanek JanKrivanek mentioned this pull request Jul 13, 2023
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.

None yet

6 participants