Add extension point to import msbuild logic after Directory.Packages.props is imported

[ViktorHofer]

Bug

Fixes: NuGet/Home#13743
Contributes to dotnet/source-build#4467

Description

Define an msbuild extension point to be able to import msbuild logic directly after the Directory.Packages.props file gets imported.

This mimics the extension point that already exists for Directory.Packages.props: https://github.com/dotnet/msbuild/blob/07a6d0d9cd3515987587b34dd8171c5c7372dcd9/src/Tasks/Microsoft.Common.props#L36

This is useful as source-build updates the Version metadata on the PackageVersion items to lift dependencies to a live built version.

PR Checklist

• Meaningful title, helpful description and a linked NuGet/Home issue
• Added tests
• Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

[ViktorHofer]

@jeffkl

[nkolev92]

Might be a good idea to add a test.
Examples: https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Core.FuncTests/Msbuild.Integration.Test/MsbuildRestoreTaskTests.cs

[ViktorHofer]

I was hoping that we don't need a test for such a trivial change. I.e. we don't have tests for such extension points in the SDK / MSBuild Common targets. If you feel strongly about it, I'm fine with adding one.

[jeffkl]

I think a unit test in this case would just make sure it was regressed in the future

[ViktorHofer]

Do we have a unit test for the import in L35 above that I could use as a template?

[AraHaan]

I would love also a way to extract a version from PackageVersion nodes as well from within msbuild itself.

[jeffkl]

This probably also needs an existence check.

Suggested change

	<Import Project="$(CustomAfterDirectoryPackagesProps)" Condition="'$(CentralPackageVersionsFileImported)' == 'true' and '$(CustomAfterDirectoryPackagesProps)' != ''" />
	<Import Project="$(CustomAfterDirectoryPackagesProps)" Condition="'$(CentralPackageVersionsFileImported)' == 'true' and '$(CustomAfterDirectoryPackagesProps)' != '' and Exists('$(CustomAfterDirectoryPackagesProps)')" />

[ViktorHofer]

Do we need that? I.e. see https://github.com/dotnet/msbuild/blob/07a6d0d9cd3515987587b34dd8171c5c7372dcd9/src/Tasks/Microsoft.Common.props#L36

That doesn't have an Exists check either. If I set this property, I would want msbuild to tell me that the import fails when the file doesn't exist.

[jeffkl]

I think a unit test in this case would just make sure it was regressed in the future

[zivkan]

I feel like building .NET projects is already too slow, and adding in extra "check if this file name exists in every single parent directory up to the root" is not helping performance. Sure, the operating system will (hopefully) cache directory listings, but even checking cached data takes more time than not checking.

There's already a Directory.Build.targets file that will get imported after Directory.Packages.props file. Yes, it will get imported much later. Even the contents of the csproj will be evaluated after the Directory.Packages.props file.

So, I don't understand the benefit or use-case, and I'm wondering if we as .NET more broadly should think more about build performance?

[zivkan]

Maybe I care about this more than I should?

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

public class Program
{
    private static void Main(string[] args)
    {
        BenchmarkRunner.Run<Program>();
    }

    [Benchmark]
    public string? FindFile()
    {
        string? directory = Directory.GetCurrentDirectory();
        while (!string.IsNullOrEmpty(directory))
        {
            string file = Path.Combine(directory, "Directory.Whatever.props");
            if (File.Exists(file))
            {
                return file;
            }

            directory = Path.GetDirectoryName(directory);
        }

        return null;
    }
}

Method	Mean	Error	StdDev	Gen0	Allocated
FindFile	124.4 us	0.70 us	0.66 us	0.2441	4.56 KB

This is just one file, so multiple this by every file that the .NET build search for (at least 3, but maybe more), multiplied by the number of projects in the solution. For huge repos with 1000+ projects, that's going to be hundreds of milliseconds (maybe even over a second?) doing nothing more than searching for files during project evaluation.

This is also an extensibility point to load a customer provided file after loading another customer provided file. Why can't the customer just add the <Import at the end of their own Directory.Packages.props?

[zivkan]

I just noticed this PR is NOT using [MSBuild]::GetDirectoryNameOfFileAbove, so it's not anywhere near as bad as my initial gut reaction.

I still don't understand why any of these before/after imports extensibility points exist. External customers can put the <Import in their own Directory.*.[props|targets] files. If this is only needed for the .NET virtual mono repo, then it seems more appropriate to me to enforce all products that the VRM consists of to add the imports, rather than change the product which affects all customers. 🤷

[ViktorHofer]

This feature request isn't tied to the VMR effort. We want to make use of this in individual repo builds as well. Note that there's also a customer asking for this.

I took a look and couldn't find an extension point that allows to import logic after D.P.props gets imported and before the project file gets read. That's the hook point that we need.

Perf isn't a reason to reject this. It is completely opt-in and we already have 100+ imports in a normal .NET project.

[AraHaan]

This PR not stale, just waiting for merge after reviews.

[ViktorHofer]

@jeffkl can this be merged now?

[jeffkl]

I'd like to make sure @zivkan has no objections.

[zivkan]

You probably don't want to consider it a blocking objection, I just don't see the point of this. Anyone who wants this can trivially add the same <Import into their Directory.Packages.props. I don't understand why this should be part of the product.

I understand that other files have a before/after imports extensibility point, but I similarly don't understand why they exist. To use an analogy, NuGet has a lot of code patterns that are now considered bad and are causing us technical debt. We absolutely should not be repeating those just to keep things consistent. So, I don't find "Directory.Build.props" already does this as convincing. Anyone wanting it could add the <Import into their Directory.Build.props. Why does this exist as a feature?

[ViktorHofer]

Anyone who wants this can trivially add the same <Import into their Directory.Packages.props. I don't understand why this should be part of the product.

This doesn't need to be a product feature but I don't see a way how to import an msbuild file directly after a Directory.Packages.props file is imported and before the project file is evaluated. In the VMR, we mirror the sources of each repo into it and we can't modify those sources, they are just a mirror and should be treated as read-only. Meaning, we can't touch each repo's Directory.Packages.props file.

Please let me know how that is achievable without adding an extension point (which as I mentioned, a customer is asking for as well).

[zivkan]

.NET/VMR already puts a bunch of requirements on component repos. For example, NuGet.Client would have no reason to have, and maintain, https://github.com/NuGet/NuGet.Client/blob/dev/eng/SourceBuildPrebuiltBaseline.xml if it wasn't for VMR.

So, I think telling dotnet/* and NuGet/NuGet.Client repos to add this <Import into their Directory.Package.props is acceptable.

Alternatively, since everyone (except NuGet.Client) is using the Arcade SDK, is there something in the Arcade SDK already that can be used to detect when it's in the VMR, and then import ..\something.props? (NuGet.Client repo already has a lot of MSBuild conditions on the BuildFromSource property, or a property with a similar name to that)

Also, could VMR achieve the same outcome with Directory.Build.targets or CustomAfterDirectoryBuildTargets? Those already get imported after Directory.Packages.props.

which as I mentioned, a customer is asking for as well

You said a customer asked for it, but didn't provide a link. the "XY Problem" is common enough to have a wikipedia article about it, so without more details about what the customer actually wants to achieve, it's impossible to know if this is a good solution to whatever the problem is.

Looking at the linked issues, I see someone mention that it can help clean up a Directory.Packages.props file for VMR, so I think another option is <Target Name="??" AfterTargets="CollectPackageVersions" in the Arcade SDK (and a once-off update to NuGet.Client's build scripts, not product code), which could have the same outcome.

[ViktorHofer]

Again, what I'm asking here is to add an extension point to NuGet.props that is fully opt-in, has zero impact on performance and doesn't need to be documented if you don't want to expose that property to customers. It's a net benefit for the VMR project, source build and the .NET stack as the extension point provides more flexibility in composing the product.

---

So, I think telling dotnet/* and NuGet/NuGet.Client repos to add this <Import into their Directory.Package.props is acceptable.

That unfortunately doesn't work (and doesn't scale with 30+ repos) with nested Directory.Packages.props which some repos use. When you have inner D.P.props files which import the repo root's D.P.props file and you add the import to the repo root's one, then the import would happen too early. Only with an extension point in NuGet.props after the "nearest" D.P.props is imported we can be certain that all inner/outer D.P.props files are imported.

Also, could VMR achieve the same outcome with Directory.Build.targets or CustomAfterDirectoryBuildTargets? Those already get imported after Directory.Packages.props.

No, we need to import msbuild logic after the Directory.Packages.props file is imported but before the project file is evaluated. Directory.Build.targets gets imported after the project file is evaluated.

Alternatively, since everyone (except NuGet.Client) is using the Arcade SDK, is there something in the Arcade SDK already that can be used to detect when it's in the VMR, and then import ..\something.props? (NuGet.Client repo already has a lot of MSBuild conditions on the BuildFromSource property, or a property with a similar name to that)

We are doing that today and that works well for Arcade's eng/Versions.props extension point: https://github.com/dotnet/arcade/blob/31624193093a13f765ab5382509e693911264509/src/Microsoft.DotNet.Arcade.Sdk/tools/DefaultVersions.props#L109-L110

We can't leverage the Arcade SDK here because it gets imported before Directory.Packages.props (Arcade.Sdk props files) and after the project file is evaluated (Arcade.Sdk targets files).

Now, here we would like to better leverage CPM and support it in all build environments (repo build, unified-build and source-build). We believe that CPM makes tracking our dependencies much easier but without the mentioned extension point supporting it in all build environments is very hard.

Because we don't have that extension point, we need to do the following today: https://github.com/dotnet/templating/blob/5e4bdc5bb02e0fe73e4b962b85bc0f2dd97607de/Directory.Packages.props#L45-L57

[ViktorHofer]

@jeffkl and I just talked offline and I will talk with the msbuild team to see if we can move their existing extension point after NuGet.props gets imported in Microsoft.Common.props so that NuGet doesn't need to expose a new one.

[jeffkl]

dotnet/sdk#1696 seems like a better solution or just moving the import of NuGet.props in MSBuild to be before $(CustomAfterMicrosoftCommonProps) is imported.