-
Notifications
You must be signed in to change notification settings - Fork 695
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
Use MSBuildProjectExtensionsPath for RestoreOutputPath #2056
Changes from 8 commits
d672fd8
8607549
cb7ce66
09f1311
b0eae2d
0e210a9
fafc168
ad8539f
b54ed69
d6814b0
5a8662e
ebf1d67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -151,9 +151,10 @@ private RestoreSummaryRequest Create( | |
{ | ||
// Set properties from the restore metadata | ||
ProjectStyle = project.PackageSpec.RestoreMetadata.ProjectStyle, | ||
// Project.json is special cased to put assets file and generated .props and targets in the project folder | ||
RestoreOutputPath = project.PackageSpec.RestoreMetadata.ProjectStyle == ProjectStyle.ProjectJson ? rootPath : project.PackageSpec.RestoreMetadata.OutputPath, | ||
AssetsCachePath = projectPackageSpec.RestoreMetadata.OutputPath, | ||
DependencyGraphSpec = projectDgSpec, | ||
BaseIntermediateOutputPath = projectPackageSpec.RestoreMetadata.OutputPath, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. set msbuildextensionspath here. |
||
ParentId = restoreArgs.ParentId | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -132,16 +132,15 @@ public RestoreRequest( | |
public ProjectStyle ProjectStyle { get; set; } = ProjectStyle.Unknown; | ||
|
||
/// <summary> | ||
/// Restore output path | ||
/// RestoreOutputPath, which is where the restore output (assets file and generated props and targets) will go | ||
/// </summary> | ||
public string RestoreOutputPath { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bring this back. |
||
|
||
/// <summary> | ||
/// Base Intermediate output path | ||
/// AssetsCachePath, which is where the no-op restore cache will go | ||
/// </summary> | ||
public string BaseIntermediateOutputPath { get; set; } | ||
public string AssetsCachePath { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
|
||
/// <summary> | ||
/// Compatibility options | ||
/// </summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ internal static bool IsNoOpSupported(RestoreRequest request) | |
} | ||
|
||
/// <summary> | ||
/// The cache file path is $(BaseIntermediateOutputPath)\$(project).nuget.cache | ||
/// The cache file path is $(project).nuget.cache in the AssetsCachePath folder | ||
/// </summary> | ||
private static string GetBuildIntegratedProjectCacheFilePath(RestoreRequest request) | ||
{ | ||
|
@@ -35,7 +35,7 @@ private static string GetBuildIntegratedProjectCacheFilePath(RestoreRequest requ | |
|| request.ProjectStyle == ProjectStyle.PackageReference | ||
|| request.ProjectStyle == ProjectStyle.Standalone) | ||
{ | ||
var cacheRoot = request.BaseIntermediateOutputPath ?? request.RestoreOutputPath; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since MsbuildExtensionsPath will always be set, it breaks this logic here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
var cacheRoot = request.AssetsCachePath; | ||
return request.Project.RestoreMetadata.CacheFilePath = GetProjectCacheFilePath(cacheRoot, request.Project.RestoreMetadata.ProjectPath); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.