Add a restore task report on whether the build files changed #1831

nkolev92 · 2017-11-29T22:58:37Z

Bug

Fixes: NuGet/Home#6122
Regression: No
If Regression then when did it last work:
If Regression then how are we preventing it in future:

Fix

Details:
Make restore task report whether the build files changed to optimize double evaluation on build /restore.

It's possible that restore did not happen, yet we save on evaluation if the build assets have not changed.

Right the result is a property that contains the project paths of projects whose assets have changed.

Chatting with @AndyGerlicher to understand if we want the property or we want an item group with more information

Testing/Validation

Tests Added: Yes, only basic tests, can't really test the full integration on our side.
Reason for not adding tests:
Validation done: Manual.

emgarten

Looks like a great way to solve this 🚀

emgarten · 2017-11-30T01:59:22Z

src/NuGet.Core/NuGet.Build.Tasks/RestoreTask.cs

+                foreach (var summary in restoreSummaries)
+                {
+                    //  Success does not matter here, as long as the files are not changed
+                    if(summary.BuildFilesChanged)


nit: spacing

emgarten · 2017-11-30T01:59:37Z

src/NuGet.Core/NuGet.Build.Tasks/RestoreTask.cs

@@ -68,6 +69,9 @@ public class RestoreTask : Microsoft.Build.Utilities.Task, ICancelableTask, IDis
        /// </summary>
        public bool HideWarningsAndErrors { get; set; }

+        [Output]
+        public string OutputRestoreChangedImportsProjectList { get; set; }


Add a summary for this

emgarten · 2017-11-30T02:01:51Z

src/NuGet.Core/NuGet.Build.Tasks/RestoreTask.cs

+                    //  Success does not matter here, as long as the files are not changed
+                    if(summary.BuildFilesChanged)
+                    {
+                        projectsWithImportsChanged.Append(summary.InputPath).Append(';');


would it be easier to just do:
string.Join(";", restoreSummaries.Where(e => e.BuildFilesChanged).Select(e => e.InputPath))

instead of the string builder and loops?

It is....didn't think too much about it, my instinct was to not add the extra linq overhead lol :)
This is called only once though :D
So I can change to make it more readable

I get avoiding Linq, it's always a trade off it seems. I'm fine with either one.

emgarten · 2017-11-30T02:03:01Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreResult.cs

@@ -65,6 +65,12 @@ public class RestoreResult : IRestoreResult
        /// </summary>
        protected string CacheFilePath { get;  }

+        /// <summary>
+        /// Indicates whether the build files have changed. It will only be updated after the restore result has been committed. It will false Commit has not be called.


It will be false if Commit

emgarten · 2017-11-30T02:04:04Z

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

@@ -89,7 +89,7 @@ Copyright (c) .NET Foundation. All rights reserved.
    Main entry point for restoring packages
    ============================================================
  -->
-  <Target Name="Restore" DependsOnTargets="_GenerateRestoreGraph">
+  <Target Name="Restore" DependsOnTargets="_GenerateRestoreGraph" Returns="$(RestoreChangedImportsProjectList)">


Consider adding a comment to the targets about this, it is covered in the task but users don't always see that source.

jainaashish · 2017-11-30T17:49:17Z

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

@@ -87,9 +87,11 @@ Copyright (c) .NET Foundation. All rights reserved.
    ============================================================
    Restore
    Main entry point for restoring packages
+    Returns a property with comma delimited projects whose imports


is it comma delimited or semicolon delimited list?

semicolon...nice catch :)

jainaashish · 2017-11-30T17:52:44Z

src/NuGet.Core/NuGet.Build.Tasks/RestoreTask.cs

+                    // Success does not matter here, as long as the files are not changed
+                    if(summary.BuildFilesChanged)
+                    {
+                        projectsWithImportsChanged.Append(summary.InputPath).Append(';');


i think it shouldn't add an additional ; at the end of the list. It will unnecessary increase load for msbuild to always ignore last item when they split on semicolon.

jainaashish · 2017-11-30T17:54:30Z

src/NuGet.Core/NuGet.Build.Tasks/RestoreTask.cs

+                foreach (var summary in restoreSummaries)
+                {
+                    // Success does not matter here, as long as the files are not changed
+                    if(summary.BuildFilesChanged)


will there be a case where restore failed but still changed the targets/props file?

@jainaashish
Yeah, restoreSuccess is part of the props.

nkolev92 · 2017-11-30T18:24:34Z

@jainaashish Addressed your feedback.

mishra14 · 2017-12-09T01:31:44Z

🔔

nkolev92 · 2017-12-09T05:35:55Z

@mishra14
Rainer needs to take a look at the changes needed on MSBuild side, so this PR is waiting on an OK from their side.

rainersigwald · 2018-01-03T21:17:46Z

Hey folks, I'm back from vacation but I've been gone for the lifetime of this. Can someone bring me up to speed?

emgarten · 2018-01-03T22:22:58Z

@rainersigwald the idea here is that restore will tell MSBuild whether or not it changed props/targets files. If it didn't make any changes then MSBuild can avoid clearing the cache.

Since then we found that this may not change much. Restore needs to run with ExcludeRestorePackageImports=true to avoid the outputs of restore becoming inputs and changing the result.

Would avoiding clearing the cache still help even if a global property changed? Is this change still useful for MSBuild?

AndyGerlicher · 2018-01-04T01:50:41Z

Unfortunately it's not really useful for MSBuild if that's always the case :(

The easy way to test this is to run msbuild /t:Restore;Build and compare the time to msbuild /restore /t:Build. If they're the same, this optimization won't work (this assumes that evaluation is a significant portion of the build to show up). If the property can be removed and /t:Restore;Build (where it reuses the cached evaluation) becomes faster we should be able to implement this fairly easily.

nkolev92 · 2018-01-04T19:17:04Z

Yeah, we did do some of those before @emgarten improved some of the target perf.

For OrchardCore:

/t:build /restore
Time Elapsed 00:00:57.75
Time Elapsed 00:00:58.31

/t:restore,build
Time Elapsed 00:00:57.81
Time Elapsed 00:00:58.82

Closing this as redundant right now.

dnfclas added the cla-already-signed label Nov 29, 2017

nkolev92 changed the title ~~[WIP, need to add tests] Add a restore task report on whether the build files changed~~ Add a restore task report on whether the build files changed Nov 30, 2017

nkolev92 requested review from emgarten, mishra14, rohit21agrawal, PatoBeltran, zhili1208 and jainaashish and removed request for emgarten November 30, 2017 01:50

emgarten approved these changes Nov 30, 2017

View reviewed changes

nkolev92 added 5 commits November 29, 2017 23:06

Simple result for the msbuild restore task

626621f

Check if the build files actually change on write

413f1a8

Add basic tests

9da669b

Update RestoreResult.cs

462e92e

address feedback

352f51e

nkolev92 force-pushed the dev-nkolev92-restoreTaskStatus branch from 572e659 to 352f51e Compare November 30, 2017 07:06

jainaashish reviewed Nov 30, 2017

View reviewed changes

address feedback

6851369

ghost removed the cla-already-signed label Dec 6, 2017

Update RestoreTask.cs

1690730

ghost mentioned this pull request Dec 31, 2017

Optimize away separate evaluation for /restore when the restore didn't actually pull down new props/targets dotnet/msbuild#2677

Closed

nkolev92 closed this Jan 4, 2018

nkolev92 deleted the dev-nkolev92-restoreTaskStatus branch January 4, 2018 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a restore task report on whether the build files changed #1831

Add a restore task report on whether the build files changed #1831

nkolev92 commented Nov 29, 2017 •

edited

Loading

emgarten left a comment

emgarten Nov 30, 2017

emgarten Nov 30, 2017

emgarten Nov 30, 2017

nkolev92 Nov 30, 2017

emgarten Nov 30, 2017

emgarten Nov 30, 2017

emgarten Nov 30, 2017

jainaashish Nov 30, 2017

nkolev92 Nov 30, 2017

jainaashish Nov 30, 2017

jainaashish Nov 30, 2017

nkolev92 Nov 30, 2017

nkolev92 commented Nov 30, 2017

mishra14 commented Dec 9, 2017

nkolev92 commented Dec 9, 2017 •

edited

Loading

rainersigwald commented Jan 3, 2018

emgarten commented Jan 3, 2018

AndyGerlicher commented Jan 4, 2018

nkolev92 commented Jan 4, 2018 •

edited

Loading

Add a restore task report on whether the build files changed #1831

Add a restore task report on whether the build files changed #1831

Conversation

nkolev92 commented Nov 29, 2017 • edited Loading

Bug

Fix

Testing/Validation

emgarten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 commented Nov 30, 2017

mishra14 commented Dec 9, 2017

nkolev92 commented Dec 9, 2017 • edited Loading

rainersigwald commented Jan 3, 2018

emgarten commented Jan 3, 2018

AndyGerlicher commented Jan 4, 2018

nkolev92 commented Jan 4, 2018 • edited Loading

nkolev92 commented Nov 29, 2017 •

edited

Loading

nkolev92 commented Dec 9, 2017 •

edited

Loading

nkolev92 commented Jan 4, 2018 •

edited

Loading