Skip to content
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

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,23 @@ private async Task<string> GetAssetsFilePathAsync(bool shouldThrow)
{
await _threadingService.JoinableTaskFactory.SwitchToMainThreadAsync();

var baseIntermediatePath = GetMSBuildProjectExtensionsPath(shouldThrow);
var msbuildProjectExtensionsPath = GetMSBuildProjectExtensionsPath(shouldThrow);

if (baseIntermediatePath == null)
if (msbuildProjectExtensionsPath == null)
{
return null;
}

return Path.Combine(baseIntermediatePath, LockFileFormat.AssetsFileName);
return Path.Combine(msbuildProjectExtensionsPath, LockFileFormat.AssetsFileName);
}

private async Task<string> GetAssetsCacheFolderAsync(bool shouldThrow)
{
await _threadingService.JoinableTaskFactory.SwitchToMainThreadAsync();

var msbuildProjectExtensionsPath = GetMSBuildProjectExtensionsPath(shouldThrow);

return msbuildProjectExtensionsPath;
}

#endregion BuildIntegratedNuGetProject
Expand Down Expand Up @@ -175,7 +184,8 @@ private string GetMSBuildProjectExtensionsPath(bool shouldThrow = true)
if (shouldThrow)
{
throw new InvalidDataException(string.Format(
Strings.MSBuildProjectExtensionsPathNotFound,
Strings.MSBuildPropertyNotFound,
ProjectBuildProperties.MSBuildProjectExtensionsPath,
_vsProjectAdapter.ProjectDirectory));
}

Expand Down Expand Up @@ -352,7 +362,7 @@ private async Task<PackageSpec> GetPackageSpecAsync(ISettings settings)
}
},
SkipContentFileWrite = true,
CacheFilePath = await GetCacheFilePathAsync(),
AssetsCacheFolder = await GetAssetsCacheFolderAsync(true),
PackagesPath = GetPackagesPath(settings),
Sources = GetSources(settings),
FallbackFolders = GetFallbackFolders(settings),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,23 +39,24 @@ public VsProjectJsonNuGetProject(
ProjectServices = projectServices;
}

protected override async Task<string> GetMSBuildProjectExtensionsPathAsync()
protected async Task<string> GetBaseIntermediateOutputPathAsync()
{
var msbuildProjectExtensionsPath = await ProjectServices.BuildProperties.GetPropertyValueAsync(ProjectBuildProperties.MSBuildProjectExtensionsPath);
var baseIntermediateOutputPath = await ProjectServices.BuildProperties.GetPropertyValueAsync(ProjectBuildProperties.BaseIntermediateOutputPath);

if (string.IsNullOrEmpty(msbuildProjectExtensionsPath))
if (string.IsNullOrEmpty(baseIntermediateOutputPath))
{
throw new InvalidDataException(string.Format(
Strings.MSBuildProjectExtensionsPathNotFound,
Strings.MSBuildPropertyNotFound,
ProjectBuildProperties.BaseIntermediateOutputPath,
MSBuildProjectPath));
}

return UriUtility.GetAbsolutePathFromFile(MSBuildProjectPath, msbuildProjectExtensionsPath);
return UriUtility.GetAbsolutePathFromFile(MSBuildProjectPath, baseIntermediateOutputPath);
}

public override async Task<string> GetCacheFilePathAsync()
{
return NoOpRestoreUtilities.GetProjectCacheFilePath(await GetMSBuildProjectExtensionsPathAsync(), MSBuildProjectPath);
return NoOpRestoreUtilities.GetProjectCacheFilePath(await GetBaseIntermediateOutputPathAsync(), MSBuildProjectPath);
}

protected override async Task UpdateInternalTargetFrameworkAsync()
Expand Down

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
Expand Up @@ -200,9 +200,9 @@
<data name="ProjectNotLoaded_RestoreFailed" xml:space="preserve">
<value>The operation failed as details for project {0} could not be loaded.</value>
</data>
<data name="MSBuildProjectExtensionsPathNotFound" xml:space="preserve">
<value>The MSBuildProjectExtensionsPath MSBuild property could not be found for project '{0}'.</value>
<comment>{0} is the full path to the project.</comment>
<data name="MSBuildPropertyNotFound" xml:space="preserve">
<value>The {0} MSBuild property could not be found for project '{1}'.</value>
<comment>{0} is the name of the property that was not set. {1} is the full path to the project.</comment>
</data>
<data name="ProjectCouldNotBeCastedToBuildPropertyStorage" xml:space="preserve">
<value>The project '{0}' could not be cast to a build property storage interface, which is required to get MSBuild properties inside Visual Studio.</value>
Expand Down
22 changes: 15 additions & 7 deletions src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets
Original file line number Diff line number Diff line change
Expand Up @@ -593,17 +593,23 @@ Copyright (c) .NET Foundation. All rights reserved.
targets import these via the MSBuildProjectExtensionsPath, so that's what the RestoreOutputPath should be. -->
<RestoreOutputPath Condition=" '$(RestoreOutputPath)' == '' " >$(MSBuildProjectExtensionsPath)</RestoreOutputPath>
</PropertyGroup>

<ConvertToAbsolutePath Paths="$(RestoreOutputPath)" Condition=" '$(PackageReferenceCompatibleProjectStyle)' == 'true' ">
<Output TaskParameter="AbsolutePaths" PropertyName="RestoreOutputAbsolutePath" />
</ConvertToAbsolutePath>

<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>
<!-- For project.json projects, the restore output path is the project directory. The assets file cache
goes in BaseIntermediateOutputPath. -->
<RestoreAssetsCacheFolder Condition=" '$(RestoreOutputPath)' == '' " >$(BaseIntermediateOutputPath)</RestoreAssetsCacheFolder>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

</PropertyGroup>

<ConvertToAbsolutePath Paths="$(RestoreOutputPath)" Condition=" '$(PackageReferenceCompatibleProjectStyle)' == 'true' OR '$(RestoreProjectStyle)' == 'ProjectJson'">
<Output TaskParameter="AbsolutePaths" PropertyName="RestoreOutputAbsolutePath" />
<ConvertToAbsolutePath Paths="$(RestoreAssetsCacheFolder)" Condition=" '$(RestoreProjectStyle)' == 'ProjectJson'">
<Output TaskParameter="AbsolutePaths" PropertyName="RestoreAssetsCacheFolder" />
</ConvertToAbsolutePath>



<!--
Determine project name for the assets file.
Highest priority: PackageId
Expand Down Expand Up @@ -653,6 +659,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<PackagesPath>$(_OutputPackagesPath)</PackagesPath>
<ProjectStyle>$(RestoreProjectStyle)</ProjectStyle>
<OutputPath>$(RestoreOutputAbsolutePath)</OutputPath>
<AssetsCacheFolder>$(RestoreOutputAbsolutePath)</AssetsCacheFolder>
<TargetFrameworks>@(_RestoreTargetFrameworksOutputFiltered)</TargetFrameworks>
<RuntimeIdentifiers>$(RuntimeIdentifiers);$(RuntimeIdentifier)</RuntimeIdentifiers>
<RuntimeSupports>$(RuntimeSupports)</RuntimeSupports>
Expand All @@ -675,7 +682,8 @@ Copyright (c) .NET Foundation. All rights reserved.
<ProjectPath>$(MSBuildProjectFullPath)</ProjectPath>
<ProjectName>$(_RestoreProjectName)</ProjectName>
<Sources>$(_OutputSources)</Sources>
<OutputPath>$(RestoreOutputAbsolutePath)</OutputPath>
<OutputPath>$(MSBuildProjectDirectory)</OutputPath>
<AssetsCacheFolder>$(RestoreAssetsCacheFolder)</AssetsCacheFolder>
<FallbackFolders>$(_OutputFallbackFolders)</FallbackFolders>
<PackagesPath>$(_OutputPackagesPath)</PackagesPath>
<ProjectJsonPath>$(_CurrentProjectJsonPath)</ProjectJsonPath>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,13 @@ private RestoreSummaryRequest Create(
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,
ParentId = restoreArgs.ParentId
ParentId = restoreArgs.ParentId,
};


request.AssetsCachePath = NoOpRestoreUtilities.GetCacheFilePath(request);


var restoreLegacyPackagesDirectory = project.PackageSpec?.RestoreMetadata?.LegacyPackagesDirectory
?? DefaultRestoreLegacyPackagesDirectory;
request.IsLowercasePackagesDirectory = !restoreLegacyPackagesDirectory;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,6 @@ public void ApplyStandardProperties(RestoreRequest request)
request.LockFilePath = ProjectJsonPathUtilities.GetLockFilePath(request.Project.FilePath);
}

if (request.Project.RestoreMetadata?.CacheFilePath == null) {
request.Project.RestoreMetadata.CacheFilePath = NoOpRestoreUtilities.GetCacheFilePath(request);
}

request.MaxDegreeOfConcurrency =
DisableParallel ? 1 : RestoreRequest.DefaultDegreeOfConcurrency;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public async Task<RestoreResult> ExecuteAsync(CancellationToken token)
_request.ExistingLockFile,
_request.ExistingLockFile.Path,
cacheFile,
_request.Project.RestoreMetadata.CacheFilePath,
_request.AssetsCachePath,
_request.ProjectStyle,
restoreTime.Elapsed);
}
Expand Down Expand Up @@ -222,9 +222,9 @@ private KeyValuePair<CacheFile, bool> EvaluateCacheFile()
NoOpRestoreUtilities.UpdateRequestBestMatchingToolPathsIfAvailable(_request);
}

if (_request.AllowNoOp && File.Exists(_request.Project.RestoreMetadata.CacheFilePath))
if (_request.AllowNoOp && File.Exists(_request.AssetsCachePath))
{
cacheFile = FileUtility.SafeRead(_request.Project.RestoreMetadata.CacheFilePath, (stream, path) => CacheFileFormat.Read(stream, _logger, path));
cacheFile = FileUtility.SafeRead(_request.AssetsCachePath, (stream, path) => CacheFileFormat.Read(stream, _logger, path));

if (cacheFile.IsValid && StringComparer.Ordinal.Equals(cacheFile.DgSpecHash, newDgSpecHash))
{
Expand Down Expand Up @@ -254,7 +254,7 @@ private KeyValuePair<CacheFile, bool> EvaluateCacheFile()
{
// Clean up to preserve the pre no-op behavior. This should not be used, but we want to be cautious.
_request.LockFilePath = null;
_request.Project.RestoreMetadata.CacheFilePath = null;
_request.AssetsCachePath = null;
}
}
return new KeyValuePair<CacheFile, bool>(cacheFile, noOp);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ public static PackageSpec GetPackageSpec(IEnumerable<IMSBuildItem> items)
result.RestoreMetadata.PackagesPath = specItem.GetProperty("PackagesPath");

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

// Read package references for netcore, tools, and standalone
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ private static string GetBuildIntegratedProjectCacheFilePath(RestoreRequest requ
|| request.ProjectStyle == ProjectStyle.PackageReference
|| request.ProjectStyle == ProjectStyle.Standalone)
{
var cacheRoot = request.AssetsCachePath;
return request.Project.RestoreMetadata.CacheFilePath = GetProjectCacheFilePath(cacheRoot, request.Project.RestoreMetadata.ProjectPath);
var cacheRoot = request.Project.RestoreMetadata.AssetsCacheFolder;

return GetProjectCacheFilePath(cacheRoot, request.Project.RestoreMetadata.ProjectPath);
}

return null;
Expand Down Expand Up @@ -89,20 +90,17 @@ internal static string GetCacheFilePath(RestoreRequest request)
/// </summary>
internal static string GetCacheFilePath(RestoreRequest request, LockFile lockFile)
{
var projectCacheFilePath = request.Project.RestoreMetadata?.CacheFilePath;
string projectCacheFilePath = null;

if (string.IsNullOrEmpty(projectCacheFilePath))
if (request.ProjectStyle == ProjectStyle.PackageReference
|| request.ProjectStyle == ProjectStyle.Standalone
|| request.ProjectStyle == ProjectStyle.ProjectJson)
{
if (request.ProjectStyle == ProjectStyle.PackageReference
|| request.ProjectStyle == ProjectStyle.Standalone
|| request.ProjectStyle == ProjectStyle.ProjectJson)
{
projectCacheFilePath = GetBuildIntegratedProjectCacheFilePath(request);
}
else if(request.ProjectStyle == ProjectStyle.DotnetCliTool)
{
projectCacheFilePath = GetToolCacheFilePath(request, lockFile);
}
projectCacheFilePath = GetBuildIntegratedProjectCacheFilePath(request);
}
else if(request.ProjectStyle == ProjectStyle.DotnetCliTool)
{
projectCacheFilePath = GetToolCacheFilePath(request, lockFile);
}
return projectCacheFilePath != null ? Path.GetFullPath(projectCacheFilePath) : null;
}
Expand Down Expand Up @@ -296,7 +294,7 @@ public static void UpdateRequestBestMatchingToolPathsIfAvailable(RestoreRequest

if (toolDirectory != null) // Only set the paths if a good enough match was found.
{
request.Project.RestoreMetadata.CacheFilePath = NoOpRestoreUtilities.GetToolCacheFilePath(toolDirectory, ToolRestoreUtility.GetToolIdOrNullFromSpec(request.Project));
request.AssetsCachePath = NoOpRestoreUtilities.GetToolCacheFilePath(toolDirectory, ToolRestoreUtility.GetToolIdOrNullFromSpec(request.Project));
request.LockFilePath = toolPathResolver.GetLockFilePath(toolDirectory);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ namespace NuGet.ProjectManagement
public static class ProjectBuildProperties
{
public const string MSBuildProjectExtensionsPath = "MSBuildProjectExtensionsPath";
public const string BaseIntermediateOutputPath = "BaseIntermediateOutputPath";
public const string PackageTargetFallback = "PackageTargetFallback";
public const string AssetTargetFallback = "AssetTargetFallback";
public const string PackageVersion = "PackageVersion";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,13 @@ public override async Task<IEnumerable<PackageReference>> GetInstalledPackagesAs
return packages;
}

protected virtual Task<string> GetMSBuildProjectExtensionsPathAsync()
protected virtual Task<string> GetBaseIntermediateOutputPathAsync()
{
// Extending class will implement the functionality (but this method can't be abstract as tests create it directly)
return Task.FromResult((string)null);
// Extending class will implement the functionality (but tests will use this implementation)
string baseIntermediateOutputPath = Path.Combine(Path.GetDirectoryName(MSBuildProjectPath), "obj");
return Task.FromResult((string)baseIntermediateOutputPath);
}

public override async Task<IReadOnlyList<PackageSpec>> GetPackageSpecsAsync(DependencyGraphCacheContext context)
{
PackageSpec packageSpec = null;
Expand All @@ -172,12 +173,12 @@ public override async Task<IReadOnlyList<PackageSpec>> GetPackageSpecsAsync(Depe
packageSpec.RestoreMetadata = metadata;

metadata.ProjectStyle = ProjectStyle.ProjectJson;
metadata.OutputPath = await GetMSBuildProjectExtensionsPathAsync();
metadata.OutputPath = Path.GetDirectoryName(MSBuildProjectPath);
metadata.ProjectPath = MSBuildProjectPath;
metadata.ProjectJsonPath = packageSpec.FilePath;
metadata.ProjectName = packageSpec.Name;
metadata.ProjectUniqueName = MSBuildProjectPath;
metadata.CacheFilePath = await GetCacheFilePathAsync();
metadata.AssetsCacheFolder = await GetBaseIntermediateOutputPathAsync();

// Reload the target framework from csproj and update the target framework in packageSpec for restore
await UpdateInternalTargetFrameworkAsync();
Expand Down
8 changes: 4 additions & 4 deletions src/NuGet.Core/NuGet.ProjectModel/ProjectRestoreMetadata.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public class ProjectRestoreMetadata : IEquatable<ProjectRestoreMetadata>

/// <summary>
/// Restore Output Path.
/// In NuGet.targets, this is set to MSBuildProjectExtensionsPath.
/// In NuGet.targets, this is set to MSBuildProjectExtensionsPath for PackageReference projects.
/// For PackageReference, this is where the assets file, generated .props and .targets files, and assets file cache will be stored.
/// For project.json projects, the assets file and generated .props and .targets will go into the project folder, and this
/// property will only affect where the assets cache is stored.
Expand Down Expand Up @@ -58,9 +58,9 @@ public class ProjectRestoreMetadata : IEquatable<ProjectRestoreMetadata>
public string PackagesPath { get; set; }

/// <summary>
/// Cache file path
/// Cache file folder
/// </summary>
public string CacheFilePath { get; set; }
public string AssetsCacheFolder { get; set; }

/// <summary>
/// Fallback folders.
Expand Down Expand Up @@ -189,7 +189,7 @@ public ProjectRestoreMetadata Clone()
clonedObject.ProjectName = ProjectName;
clonedObject.ProjectUniqueName = ProjectUniqueName;
clonedObject.PackagesPath = PackagesPath;
clonedObject.CacheFilePath = CacheFilePath;
clonedObject.AssetsCacheFolder = AssetsCacheFolder;
clonedObject.CrossTargeting = CrossTargeting;
clonedObject.LegacyPackagesDirectory = LegacyPackagesDirectory;
clonedObject.SkipContentFileWrite = SkipContentFileWrite;
Expand Down