Use MSBuildProjectExtensionsPath for RestoreOutputPath #2056

dsplaisted · 2018-02-27T23:52:25Z

This is part of a solution to dotnet/msbuild#1603. The MSBuildProjectExtensionsPath is where MSBuild will automatically import project .props and .targets files from. This PR changes the RestoreOutputPath to default to MSBuildProjectExtensionsPath instead of BaseIntermediateOutputPath. By default, both of these properties will default to the obj folder, but this change will make the behavior more correct if one of the properties is overridden.

This PR also adds an error message if RestoreOutputPath doesn't match MSBuildProjectExtensionsPath. This is because if they don't match, Restore will write .props and .targets to one location, while MSBuild will look in another, so the .props and .targets that Restore writes won't be used in the build.

I could use some help figuring out how to add a test for this behavior. @rohit21agrawal @nkolev92 To test it I think you really need to restore a project that sets RestoreOutputPath to something different than MSBuildProjectExtensionsPath, and verify that an NU1004 error is generated. Would the EndToEnd tests that are implemented in PowerShell be the right place for this? How do you run those tests?

Included in this PR is also a commit that replaces new string[0] with Array.Empty<string>(), which was something I noticed when browsing the code investigating this change.

…they don't match

nkolev92 · 2018-02-28T00:06:06Z

This is not a complete fix actually.
There's VS work that needs done as well.
This include SDk based package ref, legacy package and project.json.
So potentially there's project system work as well.

This is a breaking change, doesn't matter which way we implement it.
We need to consider the impact to people that use different versions on command-line vs VS and their CI setups etc.

I think we need to analyze this, and discussing internally before committing to a fix.

Personally, I'd like us to get this in, but the road is thorny.

nkolev92 · 2018-02-28T00:09:27Z

//cc @rrelyea for visibility.

Related issue here:
NuGet/Home#6186

dsplaisted · 2018-02-28T02:11:35Z

Per discussion with @nkolev92, here are some outstanding items for this:

Update VS to use RestoreOutputPath instead of BaseIntermediateOutputPath
- VsProjectJsonNuGetProject (for project.json projects)
- VsProjectAdapter (for non-CPS PackageReference)
- NuGetRestore.xaml in dotnet/project-system (for CPS PackageReference)
Update Microsoft.NET.Sdk to use RestoreOutputPath to determine assets file path
Investigate how much of a breaking change this is (scenarios where this could be breaking may already be broken in other ways)
Investigate whether there are valid scenarios for MSBuildProjectExtensionsPath and RestoreOutputPath to be different, and if so either remove the error or make it a warning instead
Investigate what would happen if some NuGet tooling (for example, the .NET Core SDK) has been updated, but other tooling (ie VS) hasn't, and how the changes would inter-operate.

nkolev92 · 2018-02-28T02:18:12Z

Related:
dotnet/sdk#803
https://github.com/Microsoft/msbuild/blob/f9183fddd15e728d52dbeb4716d1d43f2f5541d4/src/Tasks/Microsoft.Common.props#L38-L63
https://github.com/dotnet/sdk/blob/6945e3694c918eea4c8c4fb6217e1485b179994b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets#L28

nkolev92 · 2018-02-28T02:23:40Z

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

+
+namespace NuGet.Build.Tasks
+{
+    public class ErrorForMismatchRestoreOutputPathTask : Task


As we discussed in chat, can you please remove this error message, because it will break the advanced scenarios (namely dotnetclitool and global tools).

…ectExtensionsPath

dsplaisted · 2018-03-01T00:17:34Z

@nkolev92 I've removed the error message and modified VsProjectJsonNuGetProject and VsProjectAdapter to use the RestoreOutputPath property.

nkolev92 · 2018-03-01T01:22:39Z

...NuGet.Clients/NuGet.PackageManagement.VisualStudio/Projects/LegacyPackageReferenceProject.cs

        {
            ThreadHelper.ThrowIfNotOnUIThread();

-            var baseIntermediatePath = _vsProjectAdapter.BaseIntermediateOutputPath;
+            var restoreOutputPath = _vsProjectAdapter.RestoreOutputPath;


I think this should be MSBuildProjectExtensionsPath, or did we misunderstand each other?

RestoreOutputPath is only meant to be used in non-VS/non-build scenarios. Namely the tool restore.

The property here should be the same as the one we read in MsBuild.

nkolev92 · 2018-03-01T01:24:35Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/NoOpRestoreUtilities.cs

@@ -35,7 +35,7 @@ private static string GetBuildIntegratedProjectCacheFilePath(RestoreRequest requ
                || request.ProjectStyle == ProjectStyle.PackageReference
                || request.ProjectStyle == ProjectStyle.Standalone)
            {
-                var cacheRoot = request.BaseIntermediateOutputPath ?? request.RestoreOutputPath;


Since MsbuildExtensionsPath will always be set, it breaks this logic here.

This breaks project.json.

Don't remove RestoreOutputPath as from restoreRequest as the assets/lock and msbuild properties go to the project directory, but the cache file still goes into obj.

dsplaisted · 2018-03-06T04:06:09Z

@nkolev92 We discussed today that we want MSBuildProjectExtensionsPath to be the canonical property that NuGet uses to determine where it restores to, and that we would like to remove support for RestoreOutputPath if possible so people don't get confused.

However, since the .NET CLI uses RestoreOutputPath for tool restore scenarios, removing support for it entirely would require synchronizing this update with an update to the CLI.

Because of this, how would you feel about the following?

Keep the logic pretty much as is currently in this PR. That is, update the logic to be based on MSBuildProjectExtensionsPath instead of BaseIntermediateOutputPath, while RestoreOutputPath will still be there as an implementation detail and a way for the CLI to restore tools
Add an error that is generated if RestoreOutputPath is set when building a project. As I understand it, the current use cases for setting this property involve running the Restore target, but not actually building the project. So this should preserve the functionality for CLI tool restore, while alerting anyone using the RestoreOutputPath property in their build that it's not the right one to set.

nkolev92 · 2018-03-06T06:52:37Z

@dsplaisted
Yeah we don't want to remove RestoreOutputPath definitely. I didn't want us to remove that anyways, it has it's usage.

I'd like us to have a minimal fix, that being replace BaseIntermediateOutputPath with MSBuildProjectExtensionsPath.

The error could be something that we add later on.

dsplaisted · 2018-03-06T19:37:45Z

@nkolev92 OK, so what else do you think needs to be done with this PR?

nkolev92 · 2018-03-06T20:26:39Z

Basically, it the diff should be changing the BaseIntermediateOutputPath calls to MsBuildExtensionsPath.

Don't replace GetBaseIntermediate with GetRestoreOutputPath because the cache path calculatio is a bit different.

nkolev92 · 2018-03-07T00:57:13Z

After a discussion offline:
The net change will be: Change NuGet to use MSBuildProjectsExtensionsPath instead of BaseIntermediateOutputPath.

Tracking bug for follow-up 15.7 work.
NuGet/Home#6644

We will consider a warning if MSBuildProjectsExtensionsPath and BaseIntermediateOutputPath are not equivalent. Consider non-NuGet restore scenarios (meaning only do it in PR potentially)
Consider an error if someone is building with RestoreOutputPath set (or using RestoreOutputPath in a restore for build). Consider how dotnet self-contained publish scenarios might affect this.
Change the UWP/csproj PR build tasks to reflect this.
Consider cross-versions complications (From the brainstorming me, the only sideeffect will be that the targets and props will now be imported, thus fixing a bug, and if someone is using cross-versions in their CI/VS, they'll get the assets file in an unexpected folder)

…h in VS project adapters

jeffkl · 2018-03-07T20:53:29Z

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

@@ -577,6 +578,12 @@ Copyright (c) .NET Foundation. All rights reserved.
      <Output TaskParameter="AbsolutePaths" PropertyName="RestoreOutputAbsolutePath" />
    </ConvertToAbsolutePath>

+    <WarnForMismatchedBaseIntermediateOutputPathTask 


I'm not convinced that this makes sense. I could set just MSBuildProjectExtensionsPath and leave BaseIntermediateOutputPath alone if I wanted. With this warning, I would feel like I need to set them both always to the same location. Why even have a different property?

If NuGet only reads MSBuildProjectExtensionsPath, why would it care what the value of BaseIntermediateOutputPath is?

I agree with Jeff on this, that's why I added it in the follow-up task, becuase I'm not convinced we should add it.

@jeffkl @nkolev92 My reasoning is that the common case will be that BaseIntermediateOutputPath is set in the body of the project, and the result is not what was expected (ie the expectation was that nothing would be written to obj in the project folder. So the warning helps guide people to the right way of doing what they want to do.

I forgot to allow the warning message to be disabled by setting a property, which is what I was thinking you would do if you actually wanted the paths to be different.

I think we should rely on the error in MSBuild we're adding: dotnet/msbuild#3059

Is that not enough? I'd be okay with this warning if it was only displayed when BaseIntermediateOutputPath is different and it was modified. But I don't think it should happen anytime they're different after evaluation...

…ct uses PackageReferenc

dsplaisted · 2018-03-13T23:35:53Z

@nkolev92 I think this is ready for review now. It enables the warning being added in dotnet/msbuild#3059.

/cc @jeffkl

…projects

dsplaisted · 2018-03-15T18:34:47Z

@nkolev92 I think I've fixed the behavior for project.json. Please take a look.

nkolev92 · 2018-03-16T23:58:58Z

//cc
@rrelyea
This is the finalized PR on this issue.

nkolev92 · 2018-03-19T23:27:15Z

...uGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/DependencyGraphSpecRequestProvider.cs

                DependencyGraphSpec = projectDgSpec,
-                BaseIntermediateOutputPath = projectPackageSpec.RestoreMetadata.OutputPath,
+                RestoreOutputPath = projectPackageSpec.RestoreMetadata.OutputPath,


Should this be:
RestoreOutputPath = project.PackageSpec.RestoreMetadata.ProjectStyle == ProjectStyle.ProjectJson ? rootPath : project.PackageSpec.RestoreMetadata.OutputPath,

Why did it change?

I feel like the assetscachepath and the restoreoutputpath conditions are swapped here, unless I am missing something.

It seems to me that this is going to be null in the project.json case from commandline.

I think you are right about the conditions being swapped. Thanks for catching that!

It seems to me that this is going to be null in the project.json case from commandline.

Looking at MSBuildRestoreUtility, it looks like the RestoreOutputPath will be set for project.json projects:

result.RestoreMetadata.OutputPath = specItem.GetProperty("OutputPath");

…projects

nkolev92 · 2018-03-20T22:10:14Z

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

+    <PropertyGroup Condition=" '$(RestoreProjectStyle)' == 'ProjectJson' ">
+      <!-- For project.json projects, only the assets file cache goes in the RestoreOutputPath.  The assets file and the
+           generated .props and .targets go in the project folder.  So in that case use the BaseIntermediateOutputPath,
+           so that the assets file cache will respect a BaseIntermediateOutputPath set in the project body. -->
      <RestoreOutputPath Condition=" '$(RestoreOutputPath)' == '' " >$(BaseIntermediateOutputPath)</RestoreOutputPath>


This is now different between VS and commandline for Project.Json?
One cache file goes BaseIntermediate, the other one goes to MsBuildExtensionsOutputPath.

Why not use MSBuildProjectExtensionsPath here as well?

I didn't mean to make it inconsistent, so if it is that should be fixed.

But the idea is that for project.json, the generated props and targets go in the project folder and are imported via other means, so the whole reason we needed to switch to MSBuildProjectExtensionsPath doesn't exist. So we might as well continue to use BaseIntermediateOutputPath so that we don't change where the assets cache goes if BaseIntermediateOutputPath has been overridden.

Valid.

I think I'll just change what we read in VS for PJ then.

Other than that, this change, looks fine to me.

I'll get someone else to review it and merge it to a feature branch, and fix the tests.

What is the issue if we change it for project.json as well? at least that way, it will be a consistent behavior across project formats...

@jainaashish
We can do either one.
The argument would be, no need to risk changing the experience in PJ, because it's not fixing a bug there.

I personally don't care too much either way.

in the time when we're still supporting pj for legacy projects, I'd recommend we keep the code same across formats instead of making it conditional.

jainaashish · 2018-03-21T00:45:47Z

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

        /// </summary>
-        public string BaseIntermediateOutputPath { get; set; }
+        public string AssetsCachePath { get; set; }


This is also a NuGet restore artifact just like assets file or target file, so I think this should also go in the same location.

jainaashish · 2018-03-21T00:52:52Z

...Get.PackageManagement.VisualStudio.Test/ProjectSystems/LegacyPackageReferenceProjectTests.cs

@@ -43,17 +43,17 @@ public LegacyPackageReferenceProjectTests(DispatcherThreadFixture fixture)
        }

        [Fact]
-        public async Task GetAssetsFilePathAsync_WithValidBaseIntermediateOutputPath_Succeeds()
+        public async Task GetAssetsFilePathAsync_WithValidMSBuildProjectExtensionsPath_Succeeds()


before merging this PR, we have to add func tests to cover this scenario. So either we add as part of this PR itself or open a workitem to add them subsequently

@jainaashish
My plan was to merge this in a feature branch....work on some tests, PR those against the feature branch, and then merge that into dev.

sounds good!

dsplaisted · 2018-03-21T23:13:54Z

@nkolev92 I've pushed some updates which should fix the test failures (or at least some of them). Can you kick off another test pass? Thanks!

dsplaisted · 2018-03-22T04:40:14Z

@nkolev92 I've pushed some updates that should at least fix the compile errors. I think the tests that were failing before should pass now, although I might have broken other stuff.

nkolev92 · 2018-03-22T18:50:18Z

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

-      <RestoreOutputPath Condition=" '$(RestoreOutputPath)' == '' " >$(BaseIntermediateOutputPath)</RestoreOutputPath>
+      <!-- For project.json projects, the restore output path is the project directory.  The assets file cache
+           goes in BaseIntermediateOutputPath. -->
+      <RestoreAssetsCacheFolder Condition=" '$(RestoreOutputPath)' == '' " >$(BaseIntermediateOutputPath)</RestoreAssetsCacheFolder>


Why change this?
I fear this is becoming a bigger refactoring than it needs to be.
I'd like the change to be more targeted so we can actually merge it, and solve the initial problem.

@nkolev92 I just keep hitting issues where I'm having trouble following how all the data about the paths to use flows through, so I try to refactor it to make it more understandable to have more confidence that the change is correct.

Additionally I decided to use the BaseIntermediateOutputPath for project.json projects. I'm fine with just using MSBuildProjectExtensionsPath in both cases, but I don't think that would significantly affect the other changes.

I don't think the refactoring is necesarry, because in the end, this is a very simple change on our side.

Can you please revert these refactoring and I'll merge it in a feature branch and clean it up there?

dsplaisted · 2018-03-28T23:01:54Z

Closing in favor of #2121

dsplaisted added 2 commits February 27, 2018 12:38

Use Array.Empty<string>() instead of new string[0]

d672fd8

Use MSBuildProjectExtensionsPath for RestoreOutputPath, and error if …

8607549

…they don't match

rohit21agrawal added the Approved for CI label Feb 28, 2018

nkolev92 reviewed Feb 28, 2018

View reviewed changes

nkolev92 mentioned this pull request Feb 28, 2018

project.assets.json does not work with shared intermediates NuGet/Home#4463

Open

rohit21agrawal added Approved for CI and removed Approved for CI labels Feb 28, 2018

Remove error message when RestoreOutputPath doesn't match MSBuildProj…

cb7ce66

…ectExtensionsPath

nkolev92 reviewed Mar 1, 2018

View reviewed changes

nkolev92 requested a review from jainaashish March 6, 2018 06:52

Use MSBuildProjectExtensionsPath instead of BaseIntermediateOutputPat…

09f1311

…h in VS project adapters

dsplaisted force-pushed the dev-dsplaisted-restore-output-path branch from ea78d16 to 09f1311 Compare March 7, 2018 18:45

dsplaisted mentioned this pull request Mar 7, 2018

Don't set a default ProjectAssetsFile path in the SDK dotnet/sdk#2010

Merged

jeffkl reviewed Mar 7, 2018

View reviewed changes

This was referenced Mar 7, 2018

Log an error if MSBuildProjectExtensionsPath is modified after it is used dotnet/msbuild#3059

Merged

Speed up incremental package asset resolution dotnet/sdk#2020

Merged

Enable MSBuild warning for mismatched IntermediateOutputPath if proje…

b0eae2d

…ct uses PackageReferenc

dsplaisted force-pushed the dev-dsplaisted-restore-output-path branch from 491c25f to b0eae2d Compare March 13, 2018 23:33

Fix assets cache path and restore output path for Project.json style …

0e210a9

…projects

nkolev92 added Approved for CI and removed Approved for CI labels Mar 15, 2018

nkolev92 requested review from rohit21agrawal, PatoBeltran and zhili1208 March 16, 2018 23:58

nkolev92 added Approved for CI and removed Approved for CI labels Mar 19, 2018

nkolev92 reviewed Mar 19, 2018

View reviewed changes

dsplaisted added 2 commits March 19, 2018 17:55

Fix restore paths for project.json

fafc168

Use BaseIntermediateOutputPath for assets cache file in project.json …

ad8539f

…projects

nkolev92 added Approved for CI and removed Approved for CI labels Mar 20, 2018

Update tests to look in obj folder for assets file

b54ed69

nkolev92 reviewed Mar 20, 2018

View reviewed changes

jainaashish reviewed Mar 21, 2018

View reviewed changes

Refactor restore output and cache path flow, fix tests

d6814b0

nkolev92 added Approved for CI and removed Approved for CI labels Mar 21, 2018

dsplaisted added 2 commits March 21, 2018 17:33

Fix misspelled filename

5a8662e

Fix compile errors

ebf1d67

nkolev92 reviewed Mar 22, 2018

View reviewed changes

dsplaisted mentioned this pull request Mar 24, 2018

Use MSBuildProjectExtensionsPath for RestoreOutputPath #2121

Closed

dsplaisted closed this Mar 28, 2018

Use MSBuildProjectExtensionsPath for RestoreOutputPath #2056

Use MSBuildProjectExtensionsPath for RestoreOutputPath #2056

Conversation

dsplaisted commented Feb 27, 2018

nkolev92 commented Feb 28, 2018 • edited Loading

nkolev92 commented Feb 28, 2018

dsplaisted commented Feb 28, 2018 • edited Loading

nkolev92 commented Feb 28, 2018

Choose a reason for hiding this comment

dsplaisted commented Mar 1, 2018

Choose a reason for hiding this comment

nkolev92 Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted commented Mar 6, 2018

nkolev92 commented Mar 6, 2018

dsplaisted commented Mar 6, 2018

nkolev92 commented Mar 6, 2018

nkolev92 commented Mar 7, 2018 • edited Loading

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

dsplaisted commented Mar 13, 2018

dsplaisted commented Mar 15, 2018

nkolev92 commented Mar 16, 2018

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 Mar 20, 2018 • edited Loading

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

dsplaisted commented Mar 21, 2018

dsplaisted commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted commented Mar 28, 2018

nkolev92 commented Feb 28, 2018 •

edited

Loading

dsplaisted commented Feb 28, 2018 •

edited

Loading

nkolev92 Mar 1, 2018 •

edited

Loading

nkolev92 commented Mar 7, 2018 •

edited

Loading

nkolev92 Mar 20, 2018 •

edited

Loading