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.

Sign up for GitHub

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

Jump to bottom

Use MSBuild Static Graph to evaluate projects #3109

Merged
merged 14 commits into from
Jan 9, 2020
Merged

Use MSBuild Static Graph to evaluate projects #3109

merged 14 commits into from
Jan 9, 2020

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Nov 4, 2019

Bug

Fixes: NuGet/Home#8791
Regression: No

  • Last working version:
  • How are we preventing it in future:

Fix

Details: This change adopts Static Graph to speed up command-line based restores. There is a new EXE that does the evaluation and restore out of proc so that server GC can be enabled. The feature is off by default and users must set a property to enable it.

Testing/Validation

Tests Added: Yes/No
Reason for not adding tests:
Validation:

@jeffkl
Copy link
Contributor Author

jeffkl commented Nov 4, 2019

/cc @AndyGerlicher

@jeffkl jeffkl force-pushed the optimized-restore branch 2 times, most recently from 8dbe2e1 to a545ae2 Compare November 5, 2019 16:41
dtivel
dtivel reviewed Nov 5, 2019
View reviewed changes
Copy link
Contributor

@dtivel dtivel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't finished reviewing this but posting comments so far anyway. Some may be outdated.

NuGet.sln Outdated Show resolved Hide resolved
build/config.props Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Build.Tasks.Console/ExtensionMethods.cs Show resolved Hide resolved
src/NuGet.Core/NuGet.Build.Tasks/RestoreTaskEx.cs Show resolved Hide resolved
src/NuGet.Core/NuGet.Build.Tasks/RestoreTaskEx.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Build.Tasks.Console/Program.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Build.Tasks.Console/Program.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Build.Tasks.Console/Program.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Build.Tasks/ConsoleOutLogMessage.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Build.Tasks/ConsoleOutLogMessage.cs Outdated Show resolved Hide resolved
@jeffkl jeffkl force-pushed the optimized-restore branch 2 times, most recently from 1aa1be5 to f40de95 Compare December 4, 2019 18:12
@jeffkl
Copy link
Contributor Author

jeffkl commented Dec 4, 2019

This is now ready for a more final review. I have merged a few changes to reduce code duplication and have updated setup logic so that the new EXE is distributed in the VSIX.

I'll need to rebase this on Kat's change that adds support for packages.config and then I'll have a final review.

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 9, 2020

@nkolev92 CI appears to be broken again, how can I get a green build?

@zivkan
Copy link
Member

zivkan commented Jan 9, 2020

#3184 should fix the most common Apex & E2E test failures. It was merged a few hours ago, so please rebase and try again.

@jeffkl jeffkl merged commit 40e01c3 into NuGet:dev Jan 9, 2020
@jeffkl jeffkl deleted the optimized-restore branch January 9, 2020 22:56
jeffkl added a commit to jeffkl/NuGet.Client that referenced this pull request Jan 16, 2020
@jeffkl
Add new NuGet.Build.Tasks.Console.Test assembly
59bbdb5 
This is a follow-up to NuGet#3109
This was referenced Jan 16, 2020
Closed
Merged
Closed
@karann-msft karann-msft mentioned this pull request Feb 21, 2020
Open
@ViktorHofer
Copy link
Contributor

ViktorHofer commented Mar 7, 2020 •
edited
Loading

@jeffkl I was trying to use RestoreTaskEx but in my sdk (5.0.100-preview.2.20155.14) the NuGet.Build.Tasks.Console.dll isn't present. Also it seems that errors are swallowed as that command invoked the RestoreTaskEx task but no output was logged and no assets files were generated, this was basically a no-op:

C:\git\runtime4\tools-local\tasks\installer.tasks>..\..\..\dotnet.cmd msbuild /t:Restore /p:RestoreUseStaticGraphEvaluation=true

Microsoft (R) Build Engine version 16.6.0-preview-20105-01+5d872c945 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.


C:\git\runtime4\tools-local\tasks\installer.tasks>

Am I missing something here? cc @nkolev92

@ViktorHofer
Copy link
Contributor

OK found NuGet/Home#9267. Still we should follow-up why errors aren't propagated.

@nkolev92
Copy link
Member

nkolev92 commented Mar 9, 2020

@ViktorHofer Hey, can you please file an issue for the error propagation?

@ViktorHofer
Copy link
Contributor

Sure: NuGet/Home#9282

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

@cristinamanum cristinamanum cristinamanum left review comments

@dtivel dtivel dtivel approved these changes

@nkolev92 nkolev92 nkolev92 approved these changes

@dominoFire dominoFire Awaiting requested review from dominoFire

@donnie-msft donnie-msft Awaiting requested review from donnie-msft

@heng-liu heng-liu Awaiting requested review from heng-liu

@kartheekp-ms kartheekp-ms Awaiting requested review from kartheekp-ms

@zivkan zivkan Awaiting requested review from zivkan

@zkat zkat Awaiting requested review from zkat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make noop restore faster - speed up evaluations by calling MSBuild Static Graph apis
7 participants
@jeffkl @nkolev92 @zivkan @ViktorHofer @dominoFire @dtivel @cristinamanum