DTB only returns top-level package references #11358

drewnoakes · 2020-04-21T01:17:57Z

Historically the full graph of package references has been returned in design-time builds and used to populate the Dependencies node in Solution Explorer.

This graph can be very large. Many sessions won't expand the Dependencies node, so we would like to avoid this up-front cost during solution load. This will be achieved by lazily retrieving transitive dependency information from the assets file directly when needed.

This PR modifies PreprocessPackageDependenciesDesignTime in the following ways:

Only return items of type Package (exclude others like diagnostics, targets, assemblies)
Don't return items for all target (only those in the current target framework are required, as we run a DTB per target)
Simplify the IDs used for returned items to reduce allocations and simplify processing on the Project System side
Remove intermediate objects (via the private ItemMetadata type hierarchy) to reduce allocations

The corresponding Project System changes are able to identify when the SDK is operating with this new behaviour or the old (legacy) behaviour and adapt accordingly.

Questions:

In Microsoft.PackageDependencyResolution.targets:

Can we remove the ReportAssetsLogMessages task? It no longer seems necessary for our purposes, but someone theoretically may depend upon it.
In this task, outputs TargetDefinitions, FileDefinitions and FileDependencies appear to be no longer used. Again, is it safe to remove them and the code that allocates them?

    <ResolvePackageDependencies
      ProjectPath="$(MSBuildProjectFullPath)"
      ProjectAssetsFile="$(ProjectAssetsFile)"
      ProjectLanguage="$(Language)"
      ContinueOnError="ErrorAndContinue">
      <Output TaskParameter="TargetDefinitions" ItemName="TargetDefinitions" />
      <Output TaskParameter="PackageDefinitions" ItemName="PackageDefinitions" />
      <Output TaskParameter="FileDefinitions" ItemName="FileDefinitions" />
      <Output TaskParameter="PackageDependencies" ItemName="PackageDependencies" />
      <Output TaskParameter="FileDependencies" ItemName="FileDependencies" />
    </ResolvePackageDependencies>

wli3

Some nit in the beginning. And then I am lost at 1baaca2 .

Looks like tests are start from fresh. So I don't know what is actually removed. We should fine a time to look into this change together

src/Tasks/Microsoft.NET.Build.Tasks/PreprocessPackageDependenciesDesignTime.cs

wli3

Had an about 1 hour meeting with Drew. Now I understand this change. The only consumer of this code is project system. So removing all these information should be safe. The code left is also straightforward. The new tests around the code are also sufficient to cover and to understand what the code is doing.

wli3 · 2020-05-07T05:27:12Z

@dsplaisted do you want to have a look at this PR since this it is rather big? I think it is good after talking to Drew. We plan to merge this in a day so we have more time for an another team

wli3 · 2020-05-08T04:59:18Z

Just learnt Daniel is OOF today. Could we wait until end of the Friday?

dsplaisted · 2020-05-08T21:52:25Z

We should also update the minimum MSBuild version to 16.7 for this release, right?

dsplaisted

Looks good.

Should we also update the minimum MSBuild version to 16.7 for this release? Otherwise using this SDK with an older VS will have a broken dependency tree, right?

To Drew's questions:

Let's remove the ReportAssetsLogMessages task.

I think the TargetDefinitions, FileDefinitions, and FileDependencies may be used by some of our build infrastructure (in dotnet/installer, for example). However, I would propose that by default we don't produce them, with a flag that can be used to turn them back on.

@KathleenDollard, the two previous changes are ones we should track / document as part of our "possibly breaking changes for MSBuild logic" process.

src/Tasks/Microsoft.NET.Build.Tasks/PreprocessPackageDependenciesDesignTime.cs

Changes to the design-time build targets used to populate the dependency node mean that this SDK will only work with VS 16.7 or later. See dotnet/sdk#11358

drewnoakes · 2020-05-11T10:53:10Z

@dsplaisted thanks for your review.

Should we also update the minimum MSBuild version to 16.7 for this release? Otherwise using this SDK with an older VS will have a broken dependency tree, right?

Yes, using this SDK with older VS versions will break. Submitted dotnet/toolset#4542 with the update.

Let's remove the ReportAssetsLogMessages task.

Removed in f868606. Please review.

I think the TargetDefinitions, FileDefinitions, and FileDependencies may be used by some of our build infrastructure (in dotnet/installer, for example). However, I would propose that by default we don't produce them, with a flag that can be used to turn them back on.

Added flag EmitLegacyAssetsFileItems in the most recent commit, but it's causing test failures in CI and I cannot get tests to run locally. I'll pick this up tomorrow, but any guidance would be appreciated. Or we can split those changes out into a follow up PR.

String.Replace(char, char) allocates a new string even if no replacement is made.

These are no longer being consumed by the Dependencies node, which now sources them directly from the assets file.

From now on, this task will only return top-level items, so we do not need to output assemblies, nor do we need to accept FileDefinitions.

…nTime Performance work on the Dependencies node in dotnet/project-system mean that only top-level package data is needed from the design-time build. This commit removes items and metadata related to transitive dependencies and hierarchy. - Remove metadata Dependencies, which used to list child IDs - Remove metadata IsTopLevelDependency, which is now implicitly true - Remove 'target' items - Remove object cloning, now that instances are immutable

Previously PreprocessPackageDependenciesDesignTime returned package dependencies for all targets, and PackageRuleHandler in the project system would filter based on TargetFrameworkMoniker. This change applies the filtering in this task instead. As part of this change we are now able to remove the target prefix from the item spec of output items. This reduces string allocation, improves the chance of pooling and reduces string manipulation by the consumer.

This metadata always has 'true' as its value. We can infer this in the project system.

This metadata always has 'Package' as its value. We can infer this in the project system.

Since VS 16.7, design-time builds no longer consume assets file diagnostics as items. This commit removes their generation and supporting code.

From VS 16.7 onwards, the DTB no longer requires items of type `TargetDefinitions`, `FileDefinitions`, `FileDependencies`. This commit stops producing them by default. To restore the previous behaviour, set the `EmitLegacyAssetsFileItems` property to `true`.

drewnoakes · 2020-05-12T03:47:40Z

Fixed test failures and believe this is good to merge.

Please review f868606, 28e7900 and dotnet/toolset#4542.

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets

Co-authored-by: Daniel Plaisted <[email protected]>

After dotnet/sdk#11358, EmitLegacyAssetsFileItems needs to be set to true in order to generate FileDefinitions items.

* Update dependencies from https://github.com/dotnet/sdk build 20200513.9 Microsoft.NET.Sdk From Version 3.1.400-preview.20263.1 -> To Version 3.1.400-preview.20263.9 * Update stage 0 * Stage 0 update for LKG version * Bump MSBuild version to 16.7 * Update NuGet clien version to 6609 * Fix deployment of hostfxr for SDK Resolver After dotnet/sdk#11358, EmitLegacyAssetsFileItems needs to be set to true in order to generate FileDefinitions items. Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Jacques Eloff <[email protected]> Co-authored-by: Rainer Sigwald <[email protected]> Co-authored-by: Daniel Plaisted <[email protected]>

drewnoakes mentioned this pull request Apr 21, 2020

Design-time build only provides top-level package/project dependencies, sourcing transitive references from project.assets.json dotnet/project-system#3435

Closed

drewnoakes mentioned this pull request May 4, 2020

Populate Dependency tree transitive items via the assets file dotnet/project-system#6155

Merged

marcpopMSFT requested a review from wli3 May 6, 2020 21:22

wli3 reviewed May 7, 2020

View reviewed changes

wli3 approved these changes May 7, 2020

View reviewed changes

dsplaisted approved these changes May 8, 2020

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/PreprocessPackageDependenciesDesignTime.cs Show resolved Hide resolved

drewnoakes mentioned this pull request May 11, 2020

Increase minimum required MSBuild to 16.7.0 dotnet/toolset#4542

Merged

drewnoakes force-pushed the design-time-package-references branch from 26f3e8f to 17c60d0 Compare May 11, 2020 06:04

drewnoakes added 17 commits May 12, 2020 10:39

Remove unused property setters

c65e15f

Update documentation

4148ba5

Simplify lazy initialization

ce14bf9

Make methods static

1e77c0a

Reduce string allocations

e71eec1

String.Replace(char, char) allocates a new string even if no replacement is made.

Avoid array covariance

6a54585

Remove redundant assignments

69d73a1

Array cannot be null or have negative item length

046b8e9

Inline out var declarations

4b9f980

Reduce constructor visibilities

2c9da2b

Use local function

b556fab

Formatting

4f69a00

Remove diagnostics from PreprocessPackageDependenciesDesignTime

23256d3

These are no longer being consumed by the Dependencies node, which now sources them directly from the assets file.

Documentation

4535ef6

Make local functions static

dfcbaf3

Convert property to local variable

a3971a1

Replace O(N) with O(1) lookup

0343967

drewnoakes added 14 commits May 12, 2020 10:39

Reduce allocation when applying item metadata

10fce0c

Remove assemblies from PreprocessPackageDependenciesDesignTime

78088a7

From now on, this task will only return top-level items, so we do not need to output assemblies, nor do we need to accept FileDefinitions.

Don't set IsTopLevelDependency metadata

aa3947c

This metadata always has 'true' as its value. We can infer this in the project system.

Don't set Type metadata

455892a

This metadata always has 'Package' as its value. We can infer this in the project system.

Require Name for DTB resolved package reference items

b286e4b

Extract var for common subexpression

24672f7

Update unit tests for PreprocessPackageDependenciesDesignTime

a515ab9

Remove ReportAssetsLogMessages task

f868606

Since VS 16.7, design-time builds no longer consume assets file diagnostics as items. This commit removes their generation and supporting code.

Make methods static

88f514f

Discard unused out parameters

0f1b57e

Inline methods

d1a5111

drewnoakes force-pushed the design-time-package-references branch from 17c60d0 to 28e7900 Compare May 12, 2020 02:01

dsplaisted approved these changes May 12, 2020

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets Outdated Show resolved Hide resolved

Document property value

e5e26b7

Co-authored-by: Daniel Plaisted <[email protected]>

drewnoakes merged commit b9874ce into dotnet:release/3.1.4xx May 13, 2020

drewnoakes deleted the design-time-package-references branch May 13, 2020 08:08

dsplaisted added a commit to dotnet/cli that referenced this pull request May 19, 2020

Fix deployment of hostfxr for SDK Resolver

ea951fd

After dotnet/sdk#11358, EmitLegacyAssetsFileItems needs to be set to true in order to generate FileDefinitions items.

drewnoakes mentioned this pull request Jun 17, 2020

Port dependencies node changes #12092

Merged

marcpopMSFT mentioned this pull request Aug 31, 2020

Design Time Builds will only return top-level packages references by default dotnet/docs#20372

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DTB only returns top-level package references #11358

DTB only returns top-level package references #11358

drewnoakes commented Apr 21, 2020 •

edited

Loading

wli3 left a comment

wli3 left a comment

wli3 commented May 7, 2020 •

edited

Loading

wli3 commented May 8, 2020

dsplaisted commented May 8, 2020

dsplaisted left a comment

drewnoakes commented May 11, 2020 •

edited

Loading

drewnoakes commented May 12, 2020

DTB only returns top-level package references #11358

DTB only returns top-level package references #11358

Conversation

drewnoakes commented Apr 21, 2020 • edited Loading

wli3 left a comment

Choose a reason for hiding this comment

wli3 left a comment

Choose a reason for hiding this comment

wli3 commented May 7, 2020 • edited Loading

wli3 commented May 8, 2020

dsplaisted commented May 8, 2020

dsplaisted left a comment

Choose a reason for hiding this comment

drewnoakes commented May 11, 2020 • edited Loading

drewnoakes commented May 12, 2020

drewnoakes commented Apr 21, 2020 •

edited

Loading

wli3 commented May 7, 2020 •

edited

Loading

drewnoakes commented May 11, 2020 •

edited

Loading