Skip to content

Support nuget static graph eval in Build.props #5226

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

Merged
safern merged 1 commit into from
Apr 8, 2020
Merged

Support nuget static graph eval in Build.props #5226

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,12 @@
-->

<PropertyGroup>
<_SolutionRestoreProps Include="@(_SolutionBuildProps)" />
<_SolutionRestoreProps Include="__BuildPhase=SolutionRestore" />
<_SolutionRestoreProps Include="_NETCORE_ENGINEERING_TELEMETRY=Restore" />
<_SolutionRestoreProps Include="MSBuildRestoreSessionId=$([System.Guid]::NewGuid())" />
Copy link
Copy Markdown
Member

@safern safern Apr 7, 2020

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

@ViktorHofer ViktorHofer Apr 7, 2020

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 •
edited
Loading

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

@ViktorHofer ViktorHofer Apr 7, 2020

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

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

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

@ViktorHofer ViktorHofer Apr 7, 2020

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

@markwilkie markwilkie Apr 8, 2020

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

@tmat tmat Apr 8, 2020

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

@tmat tmat Apr 8, 2020

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.

<_SolutionRestoreProps Include="RestoreUseStaticGraphEvaluation=true" Condition="'$(RestoreUseStaticGraphEvaluation)' == 'true'" />

<!-- This can be set to false as an optimization for repos that don't use NuGet. -->
<RestoreUsingNuGetTargets Condition="'$(RestoreUsingNuGetTargets)' == ''">true</RestoreUsingNuGetTargets>
</PropertyGroup>
Expand All @@ -212,7 +218,7 @@
the new msbuild static graph APIs (RestoreUseStaticGraphEvaluation=true).
-->
<MSBuild Projects="@(ProjectToBuild)"
Properties="@(_SolutionBuildProps);__BuildPhase=SolutionRestore;_NETCORE_ENGINEERING_TELEMETRY=Restore"
Properties="@(_SolutionRestoreProps)"
RemoveProperties="$(_RemoveProps)"
Targets="_IsProjectRestoreSupported"
SkipNonexistentTargets="true"
Expand All @@ -238,7 +244,7 @@
</ItemGroup>

<MSBuild Projects="@(_ProjectToRestore)"
Properties="@(_SolutionBuildProps);__BuildPhase=SolutionRestore;_NETCORE_ENGINEERING_TELEMETRY=Restore;MSBuildRestoreSessionId=$([System.Guid]::NewGuid())"
Properties="@(_SolutionRestoreProps)"
RemoveProperties="$(_RemoveProps);TreatWarningsAsErrors"
Targets="Restore"
SkipNonexistentTargets="true"
Expand Down