Skip to content

Always set RestoreUseStaticGraphEvaluation to true#52117

Merged
sharwell merged 1 commit intodotnet:mainfrom
sharwell:static-graph
Mar 26, 2021
Merged

Always set RestoreUseStaticGraphEvaluation to true#52117
sharwell merged 1 commit intodotnet:mainfrom
sharwell:static-graph

Conversation

@sharwell
Copy link
Contributor

@sharwell sharwell commented Mar 24, 2021 •
edited
Loading

Opening so we can discuss why this would or would not be desirable.

/cc @tmat @jmarolf

@sharwell sharwell requested a review from a team as a code owner March 24, 2021 15:14
@ghost ghost added the Area-Infrastructure label Mar 24, 2021
@jmarolf
Copy link
Contributor

jmarolf commented Mar 24, 2021 •
edited
Loading

@sharwell there is no reason to not do this. The VS version we were building against in the past was not always compatible with this option. However, we should already be doing this for all restores via build.ps1 here

@sharwell
Copy link
Contributor Author

sharwell commented Mar 24, 2021

@jmarolf If we're already setting this flag for improved restore performance in command line builds:

  1. Why are we not also setting this flag for restore inside the IDE?
  2. If this mode is faster and produces correct results, why is it an option?
  3. As a follow-up to (2), why is the option disabled by default?

@Youssef1313
Copy link
Member

Youssef1313 commented Mar 24, 2021

@sharwell NuGet/Home#9803 tracks enabling it by default. Probably it was disabled by default as it's experimental feature.

@jmarolf
Copy link
Contributor

jmarolf commented Mar 24, 2021

  1. I forget that the VS restore is different :( my main situation was trying to improve CI times
  2. and 3. ¯_(ツ)_/¯ up to nuget team

@sharwell
Copy link
Contributor Author

sharwell commented Mar 25, 2021

@nkolev92 any reason for us to not merge this?

@nkolev92
Copy link

nkolev92 commented Mar 25, 2021

Nope, as long as you do the equivalency test mentioned in the body of NuGet/Home#9803 and it passes, you are all good.

@sharwell sharwell merged commit a4ab8e5 into dotnet:main Mar 26, 2021
@ghost ghost added this to the Next milestone Mar 26, 2021
@sharwell sharwell deleted the static-graph branch March 26, 2021 00:46
@sharwell
Copy link
Contributor Author

sharwell commented Mar 26, 2021

Verified that equivalency test passed.

jaredpar
jaredpar reviewed Mar 28, 2021
View reviewed changes
eng/targets/Settings.props
<CommonExtensionInstallationRoot>CommonExtensions</CommonExtensionInstallationRoot>
<LanguageServicesExtensionInstallationFolder>Microsoft\ManagedLanguages\VBCSharp\LanguageServices</LanguageServicesExtensionInstallationFolder>


Copy link
Member

@jaredpar jaredpar Mar 28, 2021

Choose a reason for hiding this comment

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

Can we avoid blank lines before after properties in property blocks? Unless there is a comment prefer we just group them together.

@jaredpar
Copy link
Member

jaredpar commented Mar 28, 2021

Why didn't we delete this when merging this change?

https://github.com/dotnet/roslyn/blob/main/eng/build.ps1#L270

@sharwell
Copy link
Contributor Author

sharwell commented Mar 29, 2021

Why didn't we delete this when merging this change?

The line in build.ps1 applies to pseudo-projects with broader scope than what gets applied for restore operations that occur within Visual Studio.

@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaredpar jaredpar jaredpar left review comments

+1 more reviewer

@jmarolf jmarolf jmarolf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Infrastructure

Projects

None yet

Milestone

16.10.P2

Development

Successfully merging this pull request may close these issues.

6 participants

@sharwell @jmarolf @Youssef1313 @nkolev92 @jaredpar @allisonchou