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

Always set RestoreUseStaticGraphEvaluation to true #52117

Merged
merged 1 commit into from
Mar 26, 2021

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Mar 24, 2021

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
@jmarolf
Copy link
Contributor

jmarolf commented Mar 24, 2021

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

@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

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

@nkolev92 any reason for us to not merge this?

@nkolev92
Copy link

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

Verified that equivalency test passed.

@@ -12,7 +12,9 @@

<CommonExtensionInstallationRoot>CommonExtensions</CommonExtensionInstallationRoot>
<LanguageServicesExtensionInstallationFolder>Microsoft\ManagedLanguages\VBCSharp\LanguageServices</LanguageServicesExtensionInstallationFolder>


Copy link
Member

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

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

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

@sharwell
Copy link
Member Author

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants