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.

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

Enable central package management implicitly when Directory.Packages.props exists #5540

Closed
wants to merge 3 commits into from
Closed

Enable central package management implicitly when Directory.Packages.props exists #5540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c5e013
Implementation
jeffkl Oct 6, 2023
0c81512
Update existing tests
jeffkl Oct 6, 2023
e1a22e8
Add test
jeffkl Oct 9, 2023
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
12 changes: 9 additions & 3 deletions src/NuGet.Clients/NuGet.CommandLine/MsBuildUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public static class MsBuildUtility
{
internal const int MsBuildWaitTime = 2 * 60 * 1000; // 2 minutes in milliseconds

private const string NuGetProps = "NuGet.CommandLine.NuGet.props";
private const string NuGetTargets = "NuGet.CommandLine.NuGet.targets";
private static readonly XNamespace MSBuildNamespace = XNamespace.Get("http://schemas.microsoft.com/developer/msbuild/2003");

Expand Down Expand Up @@ -103,10 +104,12 @@ public static async Task<DependencyGraphSpec> GetProjectReferencesAsync(
}

using (var inputTargetPath = new TempFile(".nugetinputs.targets"))
using (var nugetPropsPath = new TempFile(".nugetrestore.props"))
using (var entryPointTargetPath = new TempFile(".nugetrestore.targets"))
using (var resultsPath = new TempFile(".output.dg"))
{
// Read NuGet.targets from nuget.exe and write it to disk for msbuild.exe
// Read NuGet.props and NuGet.targets from nuget.exe and write it to disk for msbuild.exe
ExtractResource(NuGetProps, nugetPropsPath);
ExtractResource(NuGetTargets, entryPointTargetPath);

// Build a .targets file of all restore inputs, this is needed to avoid going over the limit on command line arguments.
Expand All @@ -115,15 +118,15 @@ public static async Task<DependencyGraphSpec> GetProjectReferencesAsync(
{ "RestoreUseCustomAfterTargets", "true" },
{ "RestoreGraphOutputPath", resultsPath },
{ "RestoreRecursive", recursive.ToString(CultureInfo.CurrentCulture).ToLowerInvariant() },
{ "RestoreProjectFilterMode", "exclusionlist" }
{ "RestoreProjectFilterMode", "exclusionlist" },
};

var inputTargetXML = GetRestoreInputFile(entryPointTargetPath, properties, projectPaths);

inputTargetXML.Save(inputTargetPath);

// Create msbuild parameters and include global properties that cannot be set in the input targets path
var arguments = GetMSBuildArguments(entryPointTargetPath, inputTargetPath, nugetExePath, solutionDirectory, solutionName, restoreConfigFile, sources, packagesDirectory, msbuildToolset, restoreLockProperties, EnvironmentVariableWrapper.Instance);
var arguments = GetMSBuildArguments(entryPointTargetPath, nugetPropsPath, inputTargetPath, nugetExePath, solutionDirectory, solutionName, restoreConfigFile, sources, packagesDirectory, msbuildToolset, restoreLockProperties, EnvironmentVariableWrapper.Instance);

var processStartInfo = new ProcessStartInfo
{
Expand Down Expand Up @@ -221,6 +224,7 @@ public static async Task<DependencyGraphSpec> GetProjectReferencesAsync(

public static string GetMSBuildArguments(
string entryPointTargetPath,
string nugetPropsPath,
string inputTargetPath,
string nugetExePath,
string solutionDirectory,
Expand Down Expand Up @@ -253,6 +257,8 @@ public static string GetMSBuildArguments(
args.Add($"/v:{msbuildVerbosity} ");
}

AddProperty(args, "NuGetPropsFile", nugetPropsPath);

// Override the target under ImportsAfter with the current NuGet.targets version.
AddProperty(args, "NuGetRestoreTargets", entryPointTargetPath);
AddProperty(args, "RestoreUseCustomAfterTargets", bool.TrueString);
Expand Down
4 changes: 4 additions & 0 deletions src/NuGet.Clients/NuGet.CommandLine/NuGet.CommandLine.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@
</ItemGroup>

<ItemGroup>
<EmbeddedResource Include="..\..\NuGet.Core\NuGet.Build.Tasks\NuGet.props">
<Link>NuGet.props</Link>
<SubType>Designer</SubType>
</EmbeddedResource>
<EmbeddedResource Include="..\..\NuGet.Core\NuGet.Build.Tasks\NuGet.targets">
<Link>NuGet.targets</Link>
<SubType>Designer</SubType>
Expand Down
33 changes: 16 additions & 17 deletions src/NuGet.Core/NuGet.Build.Tasks/NuGet.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,33 +9,32 @@ WARNING: DO NOT MODIFY this file unless you are knowledgeable about MSBuild and
Copyright (c) .NET Foundation. All rights reserved.
***********************************************************************************************
-->

<Project>

<!--
Import 'Directory.Packages.props' which will contain centralized packages for all the projects and solutions under
the directory in which the file is present. This is similar to 'Directory.Build.props/targets' logic which is present
in the common props/targets which serve a similar purpose.
Determine the path to the 'Directory.Packages.props' file, if the user did not:
1. Set $(ManagePackageVersionsCentrally) to false
2. Set $(ImportDirectoryPackagesProps) to false
3. Already specify the path to a 'Directory.Packages.props' file via $(DirectoryPackagesPropsPath)
-->

<PropertyGroup>
<ImportDirectoryPackagesProps Condition="'$(ImportDirectoryPackagesProps)' == ''">true</ImportDirectoryPackagesProps>
<PropertyGroup Condition="'$(ManagePackageVersionsCentrally)' != 'false' And '$(ImportDirectoryPackagesProps)' != 'false' And '$(DirectoryPackagesPropsPath)' == ''">
Copy link
Contributor

Choose a reason for hiding this comment

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

I found it difficult to understand the XML syntax while reviewing this pull request, so I have pasted the formatted XML here in case it helps to quickly grasp the changes.

<PropertyGroup Condition="'$(ManagePackageVersionsCentrally)' != 'false' And '$(ImportDirectoryPackagesProps)' != 'false' And '$(DirectoryPackagesPropsPath)' == ''">
  <_DirectoryPackagesPropsFile Condition="'$(_DirectoryPackagesPropsFile)' == ''">
    Directory.Packages.props
  </_DirectoryPackagesPropsFile>
  <_DirectoryPackagesPropsBasePath Condition="'$(_DirectoryPackagesPropsBasePath)' == ''">
    $([MSBuild]::GetDirectoryNameOfFileAbove('$(MSBuildProjectDirectory)', '$(_DirectoryPackagesPropsFile)'))
  </_DirectoryPackagesPropsBasePath>
  <DirectoryPackagesPropsPath Condition="'$(_DirectoryPackagesPropsBasePath)' != '' and '$(_DirectoryPackagesPropsFile)' != ''">
    $([MSBuild]::NormalizePath('$(_DirectoryPackagesPropsBasePath)', '$(_DirectoryPackagesPropsFile)'))
  </DirectoryPackagesPropsPath>
</PropertyGroup>

Copy link
Member

Choose a reason for hiding this comment

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

@kartheekp-ms if you have github configured to use "unified" diffs, consider changing to "split":
image

<_DirectoryPackagesPropsFile Condition="'$(_DirectoryPackagesPropsFile)' == ''">Directory.Packages.props</_DirectoryPackagesPropsFile>
<_DirectoryPackagesPropsBasePath Condition="'$(_DirectoryPackagesPropsBasePath)' == ''">$([MSBuild]::GetDirectoryNameOfFileAbove('$(MSBuildProjectDirectory)', '$(_DirectoryPackagesPropsFile)'))</_DirectoryPackagesPropsBasePath>
<DirectoryPackagesPropsPath Condition="'$(_DirectoryPackagesPropsBasePath)' != '' and '$(_DirectoryPackagesPropsFile)' != ''">$([MSBuild]::NormalizePath('$(_DirectoryPackagesPropsBasePath)', '$(_DirectoryPackagesPropsFile)'))</DirectoryPackagesPropsPath>
</PropertyGroup>

<!--
Determine the path to the 'Directory.Packages.props' file, if the user did not disable $(ImportDirectoryPackagesProps) and
they did not already specify an absolute path to use via $(DirectoryPackagesPropsPath)
Default $(ManagePackageVersionsCentrally) to true, import Directory.Packages.props, and set $(CentralPackageVersionsFileImported) to true if the user did not:
1. Set $(ManagePackageVersionsCentrally) to false
2. Set $(ImportDirectoryPackagesProps) to false
3. The path specified in $(DirectoryPackagesPropsPath) exists
-->
<PropertyGroup Condition="'$(ImportDirectoryPackagesProps)' == 'true' and '$(DirectoryPackagesPropsPath)' == ''">
<_DirectoryPackagesPropsFile Condition="'$(_DirectoryPackagesPropsFile)' == ''">Directory.Packages.props</_DirectoryPackagesPropsFile>
<_DirectoryPackagesPropsBasePath Condition="'$(_DirectoryPackagesPropsBasePath)' == ''">$([MSBuild]::GetDirectoryNameOfFileAbove('$(MSBuildProjectDirectory)', '$(_DirectoryPackagesPropsFile)'))</_DirectoryPackagesPropsBasePath>
<DirectoryPackagesPropsPath Condition="'$(_DirectoryPackagesPropsBasePath)' != '' and '$(_DirectoryPackagesPropsFile)' != ''">$([MSBuild]::NormalizePath('$(_DirectoryPackagesPropsBasePath)', '$(_DirectoryPackagesPropsFile)'))</DirectoryPackagesPropsPath>
<PropertyGroup Condition="'$(ManagePackageVersionsCentrally)' != 'false' And '$(ImportDirectoryPackagesProps)' != 'false' And Exists('$(DirectoryPackagesPropsPath)')">
<ManagePackageVersionsCentrally Condition="'$(ManagePackageVersionsCentrally)' == ''">true</ManagePackageVersionsCentrally>
</PropertyGroup>

<Import Project="$(DirectoryPackagesPropsPath)" Condition="'$(ImportDirectoryPackagesProps)' == 'true' and '$(DirectoryPackagesPropsPath)' != '' and Exists('$(DirectoryPackagesPropsPath)')"/>
<Import Project="$(DirectoryPackagesPropsPath)" Condition="'$(ManagePackageVersionsCentrally)' != 'false' And '$(ImportDirectoryPackagesProps)' != 'false' And Exists('$(DirectoryPackagesPropsPath)')" />

<PropertyGroup Condition="'$(ImportDirectoryPackagesProps)' == 'true' and '$(DirectoryPackagesPropsPath)' != '' and Exists('$(DirectoryPackagesPropsPath)')">
<PropertyGroup Condition="'$(ManagePackageVersionsCentrally)' != 'false' And '$(ImportDirectoryPackagesProps)' != 'false' And Exists('$(DirectoryPackagesPropsPath)')">
Copy link
Contributor

@kartheekp-ms kartheekp-ms Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

How about just checking <PropertyGroup Condition="'$(ManagePackageVersionsCentrally)' == 'true' > because ManagePackageVersionsCentrally is only true only when '$(ImportDirectoryPackagesProps)' != 'false' And Exists('$(DirectoryPackagesPropsPath)')?

<CentralPackageVersionsFileImported>true</CentralPackageVersionsFileImported>
</PropertyGroup>

</Project>
6 changes: 5 additions & 1 deletion test/EndToEnd/Directory.Packages.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
<Project />
<Project>
<PropertyGroup>
<ManagePackageVersionsCentrally>false</ManagePackageVersionsCentrally>
</PropertyGroup>
</Project>
Loading