Include assembly and file versions in deps file for non self-contained publish #2118

dsplaisted · 2018-04-06T06:11:59Z

Also fixes #1942 (in most cases)

dsplaisted · 2018-04-06T06:19:01Z

@steveharter @eerhardt Here's my PR for adding the assembly and file versions to the deps.json as requested in #1847.

My end-to-end test (Inbox_version_of_assembly_is_loaded_over_applocal_version) is failing, and I'm not sure if it's an issue with the test, the SDK code I'm adding, or in the host's conflict resolution. The test sets up an app that targets .NET Core 2.0 but uses a version of System.Collections.Immutable from the .NET Core 2.1 band. When the app is run on the .NET Core 2.1 runtime, I would expect the assembly bundled with the runtime to be loaded, as it has the same assembly version but a higher file version than the one that gets deployed locally with the app. However, this is not what is happening when I run the test:

Expected string to be equivalent to "C:\git\dotnet-sdk\artifacts.dotnet\2.1.300-preview3-008414\shared\Microsoft.NETCore.App\2.1.0-preview2-26314-02" with a length of 112 because immutable collections library from Framework should win, but "C:\git\dotnet-sdk\artifacts\Debug\tmp\Inbox_version---4DDF6F7D\DepsJsonVersions\bin\Debug\netcoreapp2.0\publish" has a length of 111.

Can you help me figure out why the test is failing?

eerhardt · 2018-04-06T14:09:16Z

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

+            //  deps.json to support rolling forward to a newer shared framework.  See https://github.com/dotnet/core-setup/pull/3704
+            //  Reading these versions could impact build perf, so we avoid doing it if we aren't publishing or if we are
+            //  publishing a self-contained app.
+            bool includeRuntimeFileVersions = IsPublish && !IsSelfContained;


It may be better to use projectContext.IsFrameworkDependent here instead of !IsSelfContained. The comment even says "publishing a framework-dependent app".

What do you think about moving this logic out of the C# task all together and making a property for "IncludeRuntimeFileVersions" that is defaulted in the MSBuild XML? That way someone could turn this on even if they weren't publishing, or in a self-contained app in case they wanted this info for some reason.

I think we want to be able to have the info on non-publish optionally.

What Steve and I had considered was:

Default experience:

Build (F5) -> runtimeconfig.dev.json disables roll forward

Publish -> writes the info

But there could be an opt-in to have build write the info (taking a perf hit) and not disable roll forward.

eerhardt · 2018-04-06T14:17:51Z

Can you help me figure out why the test is failing?

Can you try repro'ing it outside of the test, and then set COREHOST_TRACE=1 and inspect the output? That should tell you why the host choose what it did.

Also, ensure you are using a recent host build. How old of a version are you using?

dsplaisted · 2018-04-06T18:26:41Z

@eerhardt Here's the COREHOST_TRACE output:

corehost_trace.txt

I think the version of the host I'm using is 2.1.0-preview2-26314-02 (that's the version of the runtime that's being used).

steveharter · 2018-04-06T21:01:44Z

Here's the log for the two deps entries:

Adding runtime asset runtimes/win-x64/lib/netcoreapp2.1/System.Collections.Immutable.dll assemblyVersion= fileVersion= from runtime.win-x64.Microsoft.NETCore.App/2.1.0-preview2-26314-02

Adding runtime asset lib/netstandard2.0/System.Collections.Immutable.dll assemblyVersion=1.2.3.0 fileVersion=4.6.26216.2 from System.Collections.Immutable/1.5.0-preview1-26216-02

So it looks like the framework's deps file doesn't have the metadata added yet... So it will never "win" in a conflict.

dsplaisted · 2018-04-09T20:01:05Z

@eerhardt @steveharter @weshaggard I think this is ready for review. Per discussion with @eerhardt, the runtime's deps.json is generated using the SDK, so in order to include the file and assembly versions in it, we will need to get this change into the SDK and then update the runtime deps generation process to use the updated SDK. I've filed #2124 to enable the failing end-to-end test from this PR once that happens.

steveharter · 2018-04-09T23:05:14Z

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

+      <!--
+      If publishing a framework-dependent app, then include the assembly and file versions of files in the
+      deps.json to support rolling forward to a newer shared framework.  See https://github.com/dotnet/core-setup/pull/3704
+      Reading these versions could impact build perf, so we avoid doing it by default if we aren't publishing or if we are


Have you measured the overhead?

steveharter · 2018-04-09T23:08:30Z

...ts/Microsoft.NET.Publish.Tests/GivenThatAPublishedDepsJsonShouldContainVersionInformation.cs

+            }
+        }
+
+        [Fact(Skip = "Host deps.json doesn't have runtime file version info yet: https://github.com/dotnet/sdk/issues/2124")]


Did you try mocking the metadata (to see if it will pass when the metadata is available)?

steveharter

LGTM pending test success

Do you have an issue or reminder to enable the test once the framework metadata is available?

eerhardt · 2018-04-10T14:01:40Z

the runtime's deps.json is generated using the SDK, so in order to include the file and assembly versions in it, we will need to get this change into the SDK and then update the runtime deps generation process to use the updated SDK.

I think this is going to cause issues with source-build. Source-build is currently using 2.1.101 of the SDK to build all of the .NET Core 2.1 release. Since these new DependencyModel APIs are only in .NET Core 2.1, and the new 2.1.300 SDK will be needed to generate the shared framework's .deps.json file, it will mean that source-build will have to use the latest SDK in order to get this new info generated in the shared framework's deps.json file.

/cc @dseefeld @weshaggard

eerhardt · 2018-04-10T14:06:33Z

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

@@ -538,7 +569,7 @@ private IEnumerable<RuntimeLibrary> GetDirectReferenceRuntimeLibraries()
                    name: GetReferenceLibraryName(r),
                    version: r.Version,
                    hash: string.Empty,
-                    runtimeAssemblyGroups: new[] { new RuntimeAssetGroup(string.Empty, r.FileName) },
+                    runtimeAssemblyGroups: new[] { new RuntimeAssetGroup(string.Empty, new[] { CreateRuntimeFile(r.FileName, r.FullPath) }) },


Note - when @tannergooding's change #2090 goes in, we will need to merge this with that, so the new references get CreateRuntimeFile called on them as well.

#2090 was merged, was this handled?

@tannergooding @eerhardt It looks like this is handled correctly, as the new code from #2090 ends up calling through GetReferenceRuntimeLibraries for the indirect references, the same as how the direct references are handled.

eerhardt · 2018-04-10T14:09:14Z

using System;

(nit) copyright header

Refers to: src/Tests/Microsoft.NET.Publish.Tests/GivenThatAPublishedDepsJsonShouldContainVersionInformation.cs:1 in e18c743. [](commit_id = e18c743, deletion_comment = False)

eerhardt · 2018-04-10T14:12:56Z

...ts/Microsoft.NET.Restore.Tests/GivenThatWeWantToRestoreProjectsUsingNuGetConfigProperties.cs

@@ -156,5 +159,52 @@ private void ExtractNupkg(string nugetCache, string nupkg)
                }
            }
        }
+
+        private class MockPackageDownloader : IPackageDownloader


This seems unrelated.

This is unrelated to the feature. I retargeted this to master, and tests targeting .NET Core 2.1 were failing. So I'm trying to update the stage 0 CLI, which brought in an update to NuGet, which had API changes which required this addition... 🤷‍♂️

dsplaisted · 2018-04-10T23:44:09Z

...ts/Microsoft.NET.Restore.Tests/GivenThatWeWantToRestoreProjectsUsingNuGetConfigProperties.cs

@@ -156,5 +159,52 @@ private void ExtractNupkg(string nugetCache, string nupkg)
                }
            }
        }
+
+        private class MockPackageDownloader : IPackageDownloader


@nkolev92 @rohit21agrawal Can you provide guidance on how to implement an IPackageDownloader for a test like this? I tried naively just implementing one method, and it looks like it's failing when it calls the SignedPackageReader getter. What actually needs to be implemented here?

we have a test implementation here: https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Core.Tests/NuGet.Packaging.Test/NugetPackageUtilTests.cs#L805

Hope that helps!

dsplaisted · 2018-04-11T01:08:36Z

@dotnet-bot test this

dsplaisted · 2018-04-11T02:41:39Z

@dotnet-bot test this

dsplaisted · 2018-04-11T04:53:56Z

@dotnet-bot test this

mmitche · 2018-04-11T14:34:21Z

@dotnet-bot test this please

dsplaisted · 2018-04-11T18:36:21Z

@dotnet-bot test this

dsplaisted · 2018-04-11T18:39:39Z

@dotnet-bot test this please

dsplaisted · 2018-04-13T22:16:59Z

...ts/Microsoft.NET.Publish.Tests/GivenThatAPublishedDepsJsonShouldContainVersionInformation.cs

-            var testAsset = _testAssetsManager.CreateTestProject(testProject)
-                .Restore(Log, testProject.Name);
+            var testAsset = _testAssetsManager.CreateTestProject(testProject);
+            NuGetConfigWriter.Write(testAsset.TestRoot, NuGetConfigWriter.DotnetCoreMyGetFeed);


@johnbeisner Without adding this feed I get an error on CI: Unable to find package Microsoft.NETCore.App with version (>= 2.1.0-preview3-26411-06

Should this package not be in the dotnet-core blob storage feed?

ahsonkhan · 2018-04-19T17:10:49Z

@steveharter, @dsplaisted, when will this change get merged? It is blocking release due to the following issue: https://github.com/dotnet/corefx/issues/29032

ahsonkhan · 2018-04-19T17:13:35Z

cc @joshfree, @weshaggard, @ericstj

dleeapho · 2018-04-19T18:56:39Z

I think this is going to cause issues with source-build. Source-build is currently using 2.1.101 of the SDK to build all of the .NET Core 2.1 release. Since these new DependencyModel APIs are only in .NET Core 2.1, and the new 2.1.300 SDK will be needed to generate the shared framework's .deps.json file, it will mean that source-build will have to use the latest SDK in order to get this new info generated in the shared framework's deps.json file.

source-build is using 2.1.101 SDK to build 2.1 Preview 2 (producing a source-built 2.1.300 SDK). We will then move to using 2.1.300 SDK to build RC. We need to do this because msbuild also needs 2.1.300 SDK to build. I think source-build is good here.

dsplaisted · 2018-04-19T20:20:54Z

source-build is using 2.1.101 SDK to build 2.1 Preview 2 (producing a source-built 2.1.300 SDK). We will then move to using 2.1.300 SDK to build RC. We need to do this because msbuild also needs 2.1.300 SDK to build. I think source-build is good here.

@dleeapho So source-build 2.1 RC will be released after the RTM of the 2.1.300 SDK?

dleeapho · 2018-04-20T18:28:59Z

@dsplaisted no, RC is before RTM for source-build. The point is that we are bootstrapping the use of 2.1.300 SDK as a source-build toolset for RC, RTW via Preview 2.

Fixes dotnet#1942 (in most cases)

…d publish

… deps.json

…e or .NET Standard

dsplaisted · 2018-04-20T23:49:34Z

@dotnet-bot test OSX10.12 Release

nguerrera · 2018-04-20T23:50:36Z

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

+        }
+
+        public static string NormalizeRelativePath(string relativePath)
+                => relativePath.Replace('/', Path.DirectorySeparatorChar);


nit: indentation looks non-standard here.

dsplaisted · 2018-04-21T00:13:30Z

@ahsonkhan It just got merged

ahsonkhan · 2018-04-21T01:36:56Z

@dsplaisted, what version of the cli/sdk do we need to test that this change fixed https://github.com/dotnet/corefx/issues/29032?

dsplaisted · 2018-04-21T01:44:49Z

@mmitche can you tell us when there's a prodcon build with this change?

natemcmaster · 2018-04-24T16:17:39Z

It appears this was in last night's build, according to https://github.com/dotnet/versions/tree/657165b2995d4383d61f02cff0e89f27721daa34/build-info/dotnet/product/cli/release/2.1#built-repositories

nguerrera · 2018-04-24T17:42:09Z

Make note again that a follow-up change is needed to get versions in to runtime's deps.json as well.

weshaggard · 2018-04-24T18:19:29Z

@dsplaisted no, RC is before RTM for source-build. The point is that we are bootstrapping the use of 2.1.300 SDK as a source-build toolset for RC, RTW via Preview 2.

This means that core-setup will only be able to use the Preview 2 SDK for building RC which means that core-setup will not have this update deps file for RC (at least not in source-build).

ahsonkhan · 2018-05-01T03:22:01Z

Make note again that a follow-up change is needed to get versions in to runtime's deps.json as well.

I am assuming we have to wait for https://github.com/dotnet/core-setup/issues/4082.

It appears this was in last night's build

I tried that build, and still see a failure when building the repro provided in dotnet/corefx#29032 (with a different error though)

ericstj · 2018-05-01T04:13:32Z

Different error is normal, its failing earlier now: on load of the wrong assembly, rather than later on when it tries to find API that isn't there.

dsplaisted requested a review from nguerrera April 6, 2018 06:19

eerhardt reviewed Apr 6, 2018

View reviewed changes

dsplaisted mentioned this pull request Apr 9, 2018

Enable test Inbox_version_of_assembly_is_loaded_over_applocal_version once framework deps.json has runtime file versions #2124

Closed

dsplaisted force-pushed the 1847-assembly-versions-in-deps branch from 9c87cf6 to b4c7edf Compare April 9, 2018 19:41

dsplaisted changed the base branch from release/2.1.3xx to master April 9, 2018 19:42

dsplaisted changed the title ~~WIP - Include assembly and file versions in deps file for non self-contained publish~~ Include assembly and file versions in deps file for non self-contained publish Apr 9, 2018

steveharter reviewed Apr 9, 2018

View reviewed changes

steveharter approved these changes Apr 9, 2018

View reviewed changes

dsplaisted force-pushed the 1847-assembly-versions-in-deps branch from 50ff015 to e18c743 Compare April 10, 2018 00:31

eerhardt reviewed Apr 10, 2018

View reviewed changes

dsplaisted force-pushed the 1847-assembly-versions-in-deps branch from e18c743 to e346737 Compare April 10, 2018 22:51

dsplaisted commented Apr 10, 2018

View reviewed changes

dsplaisted force-pushed the 1847-assembly-versions-in-deps branch from e346737 to dcc217c Compare April 11, 2018 00:58

dsplaisted force-pushed the 1847-assembly-versions-in-deps branch from 820d4a0 to afa2714 Compare April 13, 2018 20:55

dsplaisted commented Apr 13, 2018

View reviewed changes

dsplaisted added 9 commits April 20, 2018 14:14

Add MSBuildAllProjects to GenerateBuildDependencyFile target inputs

986625a

Fixes dotnet#1942 (in most cases)

Include assembly and file versions in deps file for non self-containe…

b9d8bcf

…d publish

Add tests for file and assembly versions in deps file

7621a25

Add test for roll-forward assembly-loading behavior

c813fb4

Allow projects to opt in or out of including runtime file versions in…

484df50

… deps.json

Add more end-to-end tests

30e87c6

Style updates based on PR feedback

adfc2bb

Include file versions in deps file by default when targeting .NET Cor…

5f34610

…e or .NET Standard

Update tests to always expect file versions in deps.json

d29e8a5

dsplaisted force-pushed the 1847-assembly-versions-in-deps branch from 3bc399f to d29e8a5 Compare April 20, 2018 21:48

livarcocc added the Pending Shiproom Approval label Apr 20, 2018

nguerrera approved these changes Apr 20, 2018

View reviewed changes

livarcocc added Approved to merge and removed Pending Shiproom Approval labels Apr 21, 2018

dsplaisted merged commit 8a1b772 into dotnet:release/2.1.3xx Apr 21, 2018

dsplaisted deleted the 1847-assembly-versions-in-deps branch April 21, 2018 00:09

Include assembly and file versions in deps file for non self-contained publish #2118

Include assembly and file versions in deps file for non self-contained publish #2118

Conversation

dsplaisted commented Apr 6, 2018

dsplaisted commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Apr 6, 2018

dsplaisted commented Apr 6, 2018

steveharter commented Apr 6, 2018

dsplaisted commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveharter left a comment

Choose a reason for hiding this comment

eerhardt commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Apr 10, 2018

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

dsplaisted commented Apr 11, 2018

dsplaisted commented Apr 11, 2018

dsplaisted commented Apr 11, 2018

mmitche commented Apr 11, 2018

dsplaisted commented Apr 11, 2018

dsplaisted commented Apr 11, 2018

Choose a reason for hiding this comment

ahsonkhan commented Apr 19, 2018

ahsonkhan commented Apr 19, 2018

dleeapho commented Apr 19, 2018

dsplaisted commented Apr 19, 2018 • edited Loading

dleeapho commented Apr 20, 2018

dsplaisted commented Apr 20, 2018

Choose a reason for hiding this comment

dsplaisted commented Apr 21, 2018

ahsonkhan commented Apr 21, 2018

dsplaisted commented Apr 21, 2018

natemcmaster commented Apr 24, 2018

nguerrera commented Apr 24, 2018

weshaggard commented Apr 24, 2018

ahsonkhan commented May 1, 2018

ericstj commented May 1, 2018

dsplaisted commented Apr 19, 2018 •

edited

Loading