Skip to content

Support nuget static graph eval in Build.props#5226

Merged
safern merged 1 commit into
masterfrom
ViktorHofer-patch-2
Apr 8, 2020
Merged

Support nuget static graph eval in Build.props#5226
safern merged 1 commit into
masterfrom
ViktorHofer-patch-2

Conversation

@ViktorHofer
Copy link
Copy Markdown
Member

@ViktorHofer ViktorHofer commented Apr 7, 2020

This allows to set RestoreUseStaticGraphEvaluation in Build.props so that projects invoked via -projects can use the new nuget static graph evaluation restore.

@ViktorHofer ViktorHofer requested a review from safern April 7, 2020 22:29
<_SolutionRestoreProps Include="@(_SolutionBuildProps)" />
<_SolutionRestoreProps Include="__BuildPhase=SolutionRestore" />
<_SolutionRestoreProps Include="_NETCORE_ENGINEERING_TELEMETRY=Restore" />
<_SolutionRestoreProps Include="MSBuildRestoreSessionId=$([System.Guid]::NewGuid())" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We were not passing this property to _IsProjectRestoreSupported target, will it change anything?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That target doesn't run for static graph evaluation it doesn't need to be passed in.

Copy link
Copy Markdown
Member

@tmat tmat Apr 7, 2020

Choose a reason for hiding this comment

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

What's are the pros/cons of setting RestoreUseStaticGraphEvaluation to true? Would setting it by default for all repos be good/bad?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That would be SUPER bad. That mode doesn't support all feature that the current nuget restore supports. Ie hook targets are removed, item property conversion doesn't work, etc. We should move the main repos over to use it (as restore and no-op restore are faster) but this definitely needs to continue being an opt-in for some time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, then I suggest we file an issue in Arcade that tracks the progress and link if from here. Let's mention the requirements there, so that repo owners have some info based on which they could decide to enable or not. Also I'd name the property name like so: RestoreUsingStaticGraphEvaluation

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok, then I suggest we file an issue in Arcade that tracks the progress and link if from here.

Sounds good but I would prefer doing this in a follow-up issue. I will be OOF rest of the week and I want to get this in for dotnet/runtime to be able to use the switch, ie for projects passed in via the -projects switch.

Also I'd name the property name like so: RestoreUsingStaticGraphEvaluation

I'm hesitant renaming the switch to something else than the nuget property's name. That would easily confuse consumers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This feels like something that should be documented in Arcade as well. Perhaps in the SDK doc?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm hesitant renaming the switch to something else than the nuget property's name. That would easily confuse consumers.

Makes sense.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds good but I would prefer doing this in a follow-up issue.

Sure.

@safern
Copy link
Copy Markdown
Member

safern commented Apr 8, 2020

Merging as @ViktorHofer is OOF and builds are green and seems like @tmat agreed on his statements above, he can follow up for the issue and docs once he's back.

@safern safern merged commit c2875f0 into master Apr 8, 2020
@safern safern deleted the ViktorHofer-patch-2 branch April 8, 2020 18:22
dougbu pushed a commit that referenced this pull request Jul 13, 2021
dougbu pushed a commit that referenced this pull request Jul 29, 2021
ghost pushed a commit that referenced this pull request Aug 4, 2021
* Support nuget static graph eval in Build.props (#5226)
- backport of c2875f0

* Fix Break in build.proj (#5249)

Move from PropertyGroup to ItemGroup

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
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.

4 participants