Rewrite deps file generation #3086

dsplaisted · 2019-04-05T16:22:55Z

Background

The ResolvePackageAssets task outputs the resolved files from the assets file in various MSBuild items. It uses an optimized cache file in order to avoid the performance overhead of reading the assets file if the assets file hasn't changed.

The GenerateDepsFile task currently still reads the assets file directly. This is a bit of a perf hit, but the task is not part of our "inner loop" perf scenario, since if you just make a code change and rebuild, the deps file will be up-to-date and won't need to be regenerated.

The GenerateDepsFile task has a FilesToSkip parameter, which is a list of files that should not be written to the deps file. Generally, these are the "conflict" files that come from ResolvePackageFileConflicts. However, the linker also wants to be able to specify files that shouldn't be written to the deps file because they have been linked out

The FilesToSkip items are full resolved paths. Currently, the GenerateDepsFile task essentially translates assets from the assets file to the deps file, and in both of these cases the assets are identified by a package ID and version, and a relative path inside the package. So for the FilesToSkip, the resolved path needs to be translated into a NuGet package ID and a relative path inside the package. This is done via a "heuristic" that walks up the directory tree until it finds a .nuspec file, since NuGet puts the .nuspec file in the root of the package's folder in the package cache.

However, now that we use targeting packs and apphost packs which are installed to dotnet\packs instead of coming from a NuGet package, there are assets for which this heuristic doesn't work. This is the issue described in #3010, and it means that any FilesToSkip that come from these packs will be ignored, and the file will still be written to the deps file.

Description of changes

This PR implements new logic for generating the deps file. Most of the logic for doing this is actually implemented in the DependencyContextBuilder class. So this PR adds a new DependencyContextBuilder2 class which implements the new logic, and the GenerateDepsFile task can use the old logic, the new logic, or both of them and ensure that the results are the same.

DependencyContextBuilder2 works similarly to DependencyContextBuilder, except that instead of relying on the assets file for the assets to write to the deps file, it takes lists of files which come from the outputs of the ResolvePackageAssets task. These items have metadata which indicates which package they belong to and the relative path to the asset within the package. The new code still reads the assets file in order to get the "libraries" from the assets file and their dependencies. In the future we can update ResolvePackageAssets to output and cache this data as well, and then we would be able to avoid the perf hit of reading the assets file in GenerateDepsFile as well. The data that comes from the assets file (via the ProjectContext class) is all read in the DependencyContextBuilder2 constructor, which should help make it clearer what data we would need for this.

The new logic does not use the FilesToSkip parameter, rather the files to skip should simply be removed from the items that are passed to the GenerateDepsFile task, for example:

    <ItemGroup>
      <ResolvedCompileFileDefinitions Remove="@(_ConflictPackageFiles)" />
      <NativeCopyLocalItems Remove="@(_ConflictPackageFiles)" />
      <ResourceCopyLocalItems Remove="@(_ConflictPackageFiles)" />
      <RuntimeCopyLocalItems Remove="@(_ConflictPackageFiles)" />
      <RuntimeTargetsCopyLocalItems Remove="@(_ConflictPackageFiles)" />
      <RuntimePackAsset Remove="@(_ConflictPackageFiles)" />
    </ItemGroup>

Comparing old and new deps file generation logic

With this PR, the GenerateDepsFile task is able to use either the old logic, the new logic, or run both and compare the results, erroring out if there is a difference. The comparison and erroring out was necessary in order to ensure that the new code produced the same results as the old code (except in cases where we wanted it to differ). Right now the default is still to run both modes and compare the result. If we want to get more coverage ensuring that the results are the same in the wild, we could leave this as the default for a few previews, before removing the old logic. Otherwise, I'd suggest we default to the new logic and leave the old logic in as an option for a few previews as an escape hatch if this breaks anything.

Other changes

Stop generating runtime signature for deps file. This used the SHA1 algorithm but as far as we can tell was never consumed by the runtime or anywhere else
Read PackageOverrides.txt from targeting pack
Avoid using heuristic to find NuGet package ID. If there wasn't metadata on the item to indicate the package ID, but the heuristic would have found a package ID, throw an exception so that we can find these cases and add the appropriate metadata

peterhuene · 2019-04-05T18:18:44Z

src/Tasks/Common/ConflictResolution/ConflictItem.cs

+                            {
+                                path = SourcePath;
+                            }
+                            throw new InvalidOperationException("NuGetPackageId metadata not set on " + path);


Judging from the description, this is an internal diagnostic meant to weed out instances where our assets aren't emitting with the correct metadata and therefore this doesn't need to be loc'd, correct?

Yes, the idea is eventually we just remove this whole section of logic.

What happens for projects using packages.config? Won't they always depend on this path?

.NET Core projects never use packages.config. Under what circumstances could this be an issue with .NET Framework projects?

This code is in Common/ so I imagine it is used by the extensions for classic projects?

That was what I was thinking.

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/all.asset.types.osx.deps.json

wli3 · 2019-04-05T21:29:30Z

src/Tasks/Common/ConflictResolution/FrameworkListReader.cs

@@ -98,7 +98,7 @@ private static IEnumerable<ConflictItem> LoadConflictItems(string frameworkListP
                }

                ret.Add(new ConflictItem(assemblyName + ".dll",
-                                                packageId: null,
+                                                packageId: "TargetingPack",


just question, when is this get used? Do we match "TargetingPack" and do something somewhere

As part of eliminating the heuristic to find the package ID based on a path in NuGetUtils, I was going through and trying to make sure all items where we would want to find the package ID had package ID metadata set on them.

rainersigwald

Treat this as a weak signoff, please. I like your tests and run-in-parallel strategy, and I don't see anything that scares me too much here, but I don't know the details of this process well. But LGTM . . .

rainersigwald · 2019-04-05T21:22:15Z

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

+                var oldJson = JObject.Parse(File.ReadAllText(DepsFilePath));
+                var newJson = JObject.Parse(File.ReadAllText(newDepsFilePath));
+
+                newJson.Should().BeEquivalentTo(oldJson);


Not sure how I feel about pulling fluent-assertions into product code.

Agreed, I don't think we should do that either. I should change it to a simple string comparison, or if that doesn't work something that doesn't bring in a new dependency.

Taking into account that you called out the perf of this is not necessarily a main line concern (at least until we have confidence that we can get rid of the "old"), but I wonder if WriteDepsFileOld and WriteDepsFileNew should take a stream instead of the path. In the case of new and old, we write to a file stream. In the case of both, we write to memory streams and compare them, then only write one of them to disk?

Oh and I second @rainersigwald's comment regarding the use of fluent. I don't know what the proper exception message should be, however. Even a diff of two deps files might be too verbose for a message.

I guess if the comparison fails, we might just want both written to disk anyway so someone could diagnose the differences, so take my previous comment with a grain of salt.

Didn't review everything, but let's not write turd files to disk if we keep the comparison. Even without perf, it's sloppy. Also, I believe it is writing straight to the final output dir? This will cause junk left behind if we crash, etc.

If we don't write it to disk, I think we should emit a diagnostic message containing the old and new content that we can get in a log (probably not in the exception message; who wants to see that in their build output when they run into a bug?).

My concern is that we're going to fail the build when the deps file isn't what we expect, and users won't be able to provide us with something we can use to diagnose the problem.

And, yes, no Fluent assertions in the product.

While you're in here, I'd also recommend getting rid of the new Builder().WithX(x).WithY(x).WithZ(z).Build(). It has always irritated me. It is Fluent for the sake of fashion. It is a glorified function call with a bunch of parameters. But you don't need to fix this now. Just getting it off my chest while we're here. LOL.

I'll let you guys decide, but I think new only with an escape hatch is sufficient.

I'd also be comfortable "enough" with "new by default with escape hatch" as well, but the "both" model does have the benefit of ensuring compatibility correctness, such that we don't erroneously publish a user's application with a bad deps file, breaking them at runtime. That scares me.

We have a lot of previews to go. If the app breaks, we'll hear about it and tell them to use the escape hatch while we fix it. The real interesting thing I think the comparison buys us is finding differences in .deps.json that don't lead to obvious consequences. That said, I trust you guys to make the call, having invested the effort in writing and reviewing it.

wli3

given the old and new comparison. I think we could ship it.

But at the same time. I don't think it is ideal for "put it in the wild for 2 month" as a way to gain confidence. I do understand msbuild property's global nature, it is hard to define input and output while every Target can dump more stuff into the itemgroup. But we should still try to add some boundary to make change like this less risky and easier to understand in the future.

wli3 · 2019-04-05T21:39:38Z

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

-                IEnumerable<string> assemblies = GetCompileTimeAssemblies(export, referenceProjectInfo);
+                IEnumerable<string> assemblies = Enumerable.Empty<string>();
+
+                //  In some situations, the assets file will include compilation assets under the RID-specific


Tests for this kind of packages?

(complaint for existing code, what does a boolean "runtime" mean...)

This is covered somewhere in our tests, which is how I ran into the need for this. I don't remember exactly where though.

wli3 · 2019-04-05T22:11:51Z

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

+
+namespace Microsoft.NET.Build.Tasks
+{
+    internal class DependencyContextBuilder2


nit: The "style" is close to existing DependencyContextBuilder. The methods are still long. It could use some extract method at least. Considering there are so many fields, it should be able to extract more classes. I don't think we should block on that, but I hope this class can be more clear. And ideally add some unit tests

I also agree with Will that the complexity of some of these methods inherited from the old implementation might need some refactoring for clarity.

If now isn't the time to do that, then I think we should file an issue to address it when we remove the old code.

peterhuene · 2019-04-06T00:05:21Z

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

+                hashPath);
+        }
+
+        private void GetCommonLibraryProperties(DependencyLibrary library,


Nit: maybe a tuple here instead of the out params?

peterhuene · 2019-04-06T00:06:59Z

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

+            hashPath = null;
+            if (library.Type == "package")
+            {
+                // TEMPORARY: All packages are serviceable in RC2


Nit: is this temporary comment from 2016 still relevant today?

I don't think so, but I wasn't sure so I figured it was best to preserve it in the re-implementation.

Do we have any test coverage for this servicing mechanism?

We have to hard-code this because NuGet/Home#2923 hasn't been implemented yet.

peterhuene · 2019-04-06T00:22:47Z

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

@@ -139,7 +139,7 @@ void ProcessInputFileList(ITaskItem[] inputFiles, List<ITaskItem> imageCompilati
                if (!InputFileEligibleForCompilation(file))
                    continue;

-                var outputR2RImageRelativePath = file.GetMetadata(MetadataKeys.RelativePath);
+                var outputR2RImageRelativePath = file.GetMetadata("RelativePath");


Should we not have kept MetadataKeys.RelativePath? When I saw the change to MetadataKeys earlier, I thought PathInPackage was going to replace all uses of this metadata key.

Originally I had replaced all occurrences, but #2997 was merged while I was working on this PR, adding a use of RelativePath. I'm not entirely sure that couldn't be changed to PathInPackage, but I left it as RelativePath and the crossgen tests passed, so I think it was probably right.

It is reasonable to add it back to MetadataKeys now though.

peterhuene

A few nit comments and the comment on the use of fluent from the build task assembly.

Given how important correct deps file generation is to the functioning of user applications, I think the approach of catching discrepancies by default (at least for a few previews) is a good one, plus an escape hatch of saying "use the old generation" for anyone that runs into problems.

nguerrera · 2019-04-06T01:57:30Z

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

+                WriteDepsFileNew(newDepsFilePath);
+
+                var oldJson = JObject.Parse(File.ReadAllText(DepsFilePath));
+                var newJson = JObject.Parse(File.ReadAllText(newDepsFilePath));


This is quite expensive. deps files can be big. We're reading into Strings unnecessarily. I know this isn't an inner loop task, but that's a lot of waste.

dsplaisted · 2019-04-06T01:58:03Z

@peterhuene @rainersigwald @nguerrera @KathleenDollard What are your thoughts on the error message I've added now (instead of using Fluent Assertion's assertion failure)?

C:\git\dotnet-sdk\artifacts\bin\Debug\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(145,5): Internal error: new deps file generation logic did not produce the same result as the old logic.
Please file an issue for this at https://github.com/dotnet/sdk
You can work around this by setting the DepsFileGenerationMode MSBuild property to 'old'
Deps file from old logic: C:\git\dotnet-sdk\artifacts\tmp\Debug\It_only_publi---24BA390C\FilterSatelliteAssemblies\bin\Debug\netcoreapp2.0\FilterSatelliteAssemblies.deps.json
Deps file from new logic: C:\git\dotnet-sdk\artifacts\tmp\Debug\It_only_publi---24BA390C\FilterSatelliteAssemblies\bin\Debug\netcoreapp2.0\FilterSatelliteAssemblies.deps.new.json
[C:\git\dotnet-sdk\artifacts\tmp\Debug\It_only_publi---24BA390C\FilterSatelliteAssemblies\FilterSatelliteAssemblies.csproj]

nguerrera · 2019-04-06T03:32:43Z

The message is good, but I'm not liking writing temporary .new file to true output directory (even if only temporarily).

wli3 · 2019-04-06T17:54:26Z

So we will target preview 5 or ask mode?

dsplaisted · 2019-04-08T15:37:55Z

I tried to optimize the file writing, but wasn't able to do much. The DependencyContextWriter.Write() method takes a stream, but it apparently closes that stream when it's done. So if you pass it a MemoryStream when you try to read the stream afterwards it will be closed. So you really do need to write to a file.

I switched from a JToken comparison to a simple string comparison. This avoids the JSON parsing, but means that the files get read into memory as one stream. We could load the JSON via a file stream, which would use more memory but probably wouldn't have any giant chunks of memory for large deps files. Or we could try to compare the file contents via streams instead of reading them all in at once. I'm not sure it's worth it for code that will only be active during a few previews. Let me know what you think.

I do think having the comparison enabled by default for a while is valuable. I ran into another issue last night that only repro'd on the ASP.NET perf tests, which could have been difficult to diagnose without a full repro (which might be harder for people to provide than the deps files).

rainersigwald · 2019-04-08T16:01:44Z

That message looks pretty good but I recommend explicitly saying "please include both of these files in the bug report you're about to file".

eerhardt · 2019-04-08T17:17:25Z

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

+        }
+        protected override void ExecuteCore()
+        {
+            if (DepsFileGenerationMode.Equals("old", StringComparison.InvariantCultureIgnoreCase))


What do you think about:

V1

V2

All

instead of

old

new

both

?

If we ever wanted to add more "modes", the current strings seem limiting.

That's not a bad suggestion, but we don't expect for these to live for long. The only reason we are including multiple modes is in order to help validate that the new logic behaves the same as the old, and to offer an escape hatch if there is a bug in the new logic. After a few previews, we plan to remove the old logic altogether.

I've filed #3098 to track removing the old logic.

dsplaisted · 2019-04-08T18:10:31Z

@rainersigwald I've updated the message to the following:

C:\git\dotnet-sdk\artifacts\bin\Debug\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(145,5): Internal error: new deps file generation logic did not produce the same result as the old logic.
Please file an issue for this at https://github.com/dotnet/sdk and include the following two files:
Deps file from old logic: C:\git\dotnet-sdk\artifacts\tmp\Debug\It_only_publi---24BA390C\FilterSatelliteAssemblies\bin\Debug\netcoreapp2.0\FilterSatelliteAssemblies.deps.json
Deps file from new logic: C:\git\dotnet-sdk\artifacts\tmp\Debug\It_only_publi---24BA390C\FilterSatelliteAssemblies\bin\Debug\netcoreapp2.0\FilterSatelliteAssemblies.deps.new.json
You can work around this by setting the DepsFileGenerationMode MSBuild property to 'old' [C:\git\dotnet-sdk\artifacts\tmp\Debug\It_only_publi---24BA390C\FilterSatelliteAssemblies\FilterSatelliteAssemblies.csproj]

… different library versions

This was logic in ProjectContext.GetRuntimeLibraries that I missed when re-implementing it

…e deps file

…es match

… deps file generation

….json

…ntained output

This aligns with current 3.0 templates, and no longer generates warnings

nguerrera · 2019-04-11T03:18:52Z

👏 Thanks for pushing on this, @dsplaisted !

….25 (dotnet#3086) - Microsoft.DotNet.Cli.Runtime - 3.1.100-preview1.19504.25

dsplaisted requested review from peterhuene, rainersigwald and a team April 5, 2019 18:07

peterhuene reviewed Apr 5, 2019

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/all.asset.types.osx.deps.json Show resolved Hide resolved

wli3 reviewed Apr 5, 2019

View reviewed changes

rainersigwald approved these changes Apr 5, 2019

View reviewed changes

dsplaisted force-pushed the rewrite-deps-generation branch from 73cd21e to fc3a34e Compare April 5, 2019 22:13

wli3 reviewed Apr 5, 2019

View reviewed changes

peterhuene reviewed Apr 6, 2019

View reviewed changes

peterhuene approved these changes Apr 6, 2019

View reviewed changes

dsplaisted force-pushed the rewrite-deps-generation branch from e7cbd76 to eb5a845 Compare April 6, 2019 01:47

nguerrera reviewed Apr 6, 2019

View reviewed changes

dsplaisted marked this pull request as ready for review April 8, 2019 14:43

eerhardt reviewed Apr 8, 2019

View reviewed changes

dsplaisted mentioned this pull request Apr 8, 2019

Remove old deps file generation logic #3098

Closed

dsplaisted mentioned this pull request Apr 8, 2019

Assemblies referenced by EF are not coming from the Shared Framework dotnet/aspnetcore#8836

Closed

dsplaisted changed the base branch from master to release/3.0.1xx April 9, 2019 17:52

dsplaisted added 4 commits April 9, 2019 14:32

Add tests for passing FilesToSkip to GenerateDepsFile

3b824b6

Stop generating runtime signature for deps.json

64acbcc

Set package ID metadata on conflict items

45bcfd4

Implement new deps file generation logic

acc8b26

dsplaisted added 16 commits April 9, 2019 14:32

Filter publish conflict packages from new deps file generation

abd3f36

Fix deps library lookup to handle multitargeted projects depending on…

91d36a9

… different library versions

Update tests that need new deps file generation mode

bd10b02

Read PackageOverrides from targeting packs

eb9afbb

Don't rely on heuristic to find NuGet package ID for a file

030137f

Fix platform library calculation for ASP.NET Core apps

fc31c03

This was logic in ProjectContext.GetRuntimeLibraries that I missed when re-implementing it

Fix resource skipping tests on non-Windows

f353115

Match previous logic for what library dependencies are included in th…

c7c062e

…e deps file

Don't use fluent assertions in product code to validate that deps fil…

e0ec4d0

…es match

Code review feedback

b0ee865

Fix generating error about mismatching deps files

b20aada

Fix GenerateDepsFile task not returning failure when error is logged

68e6297

Don't remove compile conflicts from runtime assets and vice versa for…

7f3c8b7

… deps file generation

Use string comparison instead of parsing and comparing json

ca946c4

Don't put runtime pack dependencies in the "compile" projects in deps…

adb4014

….json

Update deps file mismatch error text

4754e44

dsplaisted closed this Apr 10, 2019

dsplaisted reopened this Apr 10, 2019

dsplaisted force-pushed the rewrite-deps-generation branch from 98f314d to 3572190 Compare April 10, 2019 19:38

Update stage 0

a56fb4f

dsplaisted force-pushed the rewrite-deps-generation branch from 3572190 to a56fb4f Compare April 10, 2019 20:31

dsplaisted added 3 commits April 10, 2019 15:03

Add System.Data.DataSetExtensions.dll to expected .NET Core 3 self-co…

c8f8b9d

…ntained output

Install 2.2.4 shared framework

3ce6af5

Update perf test sources

9ead3df

This aligns with current 3.0 templates, and no longer generates warnings

dsplaisted merged commit 814b789 into dotnet:release/3.0.1xx Apr 11, 2019

This was referenced Apr 11, 2019

Include a fixed GenerateDepsFile task dotnet/linker#479

Closed

Add ILLink targets and tests #3125

Merged

ViktorHofer mentioned this pull request May 5, 2019

Update the dotnet SDK to 3.0.100-preview6-011681 (latest) dotnet/corefx#37340

Merged

dsplaisted pushed a commit to dsplaisted/sdk that referenced this pull request Feb 19, 2020

Update dependencies from https://github.com/dotnet/cli build 20191004…

34fc451

….25 (dotnet#3086) - Microsoft.DotNet.Cli.Runtime - 3.1.100-preview1.19504.25

Rewrite deps file generation #3086

Rewrite deps file generation #3086

Conversation

dsplaisted commented Apr 5, 2019 • edited Loading

Background

Description of changes

Comparing old and new deps file generation logic

Other changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhuene Apr 6, 2019 • edited Loading

Choose a reason for hiding this comment

peterhuene Apr 6, 2019 • edited Loading

Choose a reason for hiding this comment

nguerrera Apr 6, 2019 • edited Loading

Choose a reason for hiding this comment

peterhuene Apr 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhuene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted commented Apr 6, 2019

nguerrera commented Apr 6, 2019 • edited Loading

wli3 commented Apr 6, 2019

dsplaisted commented Apr 8, 2019

rainersigwald commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted commented Apr 8, 2019

nguerrera commented Apr 11, 2019

dsplaisted commented Apr 5, 2019 •

edited

Loading

peterhuene Apr 6, 2019 •

edited

Loading

peterhuene Apr 6, 2019 •

edited

Loading

nguerrera Apr 6, 2019 •

edited

Loading

peterhuene Apr 6, 2019 •

edited

Loading

wli3 left a comment •

edited

Loading

eerhardt Apr 8, 2019 •

edited

Loading

nguerrera commented Apr 6, 2019 •

edited

Loading