Platform manifest in TargetingPack should refer to contents of runtime pack

[wtgodbe]

#14538 changed PlatformManifest.txt in the Targeting pack to list the assemblies in the targeting pack, rather than to list the assemblies in the runtime pack. At the time, we thought the former was the correct behavior, but in reality the latter is correct. The platform manifest exists to inform an app, while building against the ref pack assemblies, what will be available in the shared framework at runtime. We changed this behavior between 3.0.0 and 3.0.1, which broke customers who had transitive 2.x dependencies in their 3.0 apps (specifically aspnetcorev2_inprocess.dll).

We have a somewhat ugly workaround for this and are working on finding a better one.

[wtgodbe]

CC @nguerrera @mmitche @jkotalik @Pilchie @JunTaoLuo @dougbu

[Pilchie]

👀

[wtgodbe]

@nguerrera another question here - do we need a platform manifest in the runtime pack? looks like we try to put it there, but don't actually get it included in the package content:

https://github.com/aspnet/AspNetCore/blob/bc0c7121eff6e084b930e96be44f704b53924633/src/Framework/src/Microsoft.AspNetCore.App.Runtime.csproj#L283-L289

[nguerrera]

PlatformManifest.txt is not read from the runtime pack. I think there had been some discussion about needing to do that, but I think it was mistaken, and I don't see any code that would read it. @dsplaisted may remember more details, but is OOF.

[wtgodbe]

Verified locally that this produces a ref pack with a correct-looking platform manifest

[mmitche]

Is this something that gets added back in on the next patch? If so, should we just comment this out or conditionalize it properly (e.g. by VersionPrefix)

[wtgodbe]

I think I don't fully understand the ramifications of removing this, so I'll put it back

[mmitche]

Why does this get removed?

[jkotalik]

It looks to be called from Microsoft.AspNetCore.App.Runtime.csproj: https://github.com/aspnet/AspNetCore/blob/20fc1adf2ada01a4a1fc7934706d2b641acbb9f4/src/Framework/src/Microsoft.AspNetCore.App.Runtime.csproj#L230. As I don't think the result of GenerateSharedFrameworkDepsFile is being used for the Runtime project, should we move it all to the Ref project?

[wtgodbe]

It looks to be called from Microsoft.AspNetCore.App.Runtime.csproj

That's correct, we use the platform manifest generated by the Runtime pack since that's what the file should refer to. The runtime pack uses ReferenceCopyLocalPaths to calculate what should be included, so I guess it should stay in that file. I'll add a comment explaining why it's there and what it's for.

[nguerrera]

Since edited out from comment, but I assume that the manifest still has entries like this

Microsoft.Extensions.Configuration.Xml.dll|Microsoft.AspNetCore.App.Ref|3.0.2.0|3.0.219.60403

I think I remember now how we might have gotten lured into this bug. The assembly version in the manifest should be 3.0.0.0. The targeting pack is meant to be representative of what any 3.0 app can rely on having. If the 3.0 app is running on < 3.0.2, it cannot rely on having assembly version 3.0.2.0. So if a package brings 3.0.1, it should take it. The assembly version needs to be 3.0.0.0 here. I think this led to harvesting the ref pack instead of the runtime pack for generation. The correct logic is really harvesting the 3.0.0 runtime pack, which is obviously complicated. :( You could almost get away with checking in the 3.0.0 manifest and repackaging it into 3.0.* when it is rebuilt, but that's not something that is easy to make continuous. :(

[wtgodbe]

Bump this to 3.0.3 if we ship this with that release

[wtgodbe]

Bump this to 3.0.3 if we ship this with that release

[wtgodbe]

This broke the template tests:

error CS0234: The type or namespace name 'Mvc' does not exist in the namespace 'Microsoft.AspNetCore' (are you missing an assembly reference?) [F:\workspace_work\1\s\src\ProjectTemplates\test\bin\Release\netcoreapp3.0\TestTemplates\AspNet.angularindividualuld.msq0wkru3kv\AspNet.angularindividualuld.msq0wkru3kv.csproj]\r\nobj\Release\netcoreapp3.0\AspNet.angularindividualuld.msq0wkru3kv.RazorAssemblyInfo.cs(10,33): error CS0234: The type or namespace name 'Mvc' does not exist in the namespace 'Microsoft.AspNetCore' (are you missing an assembly reference?)

Looking

[nguerrera]

If there are in fact 3.0.2.0 assembly versions in the new manifest, that is a problem.

(Edit: looks like I don't have permission to request changes and can only suggest them, but I am in fact requesting them, lol.)

[wtgodbe]

@nguerrera yep, the AssemblyVersions are all 3.0.2.0 (or 42.42.42.42 on my local build from the implementation assemblies) 😢. We could try getting ref assembly versions, but then we hit issues w/ the assemblies that are in the runtime pack, but not the ref pack (e.g. System.Security.Cryptography.Pkcs, System.Drawing.Common, the native .dll...). I suppose we could also update the tool that generates this manifest (which is checked into AspNetCore: https://github.com/aspnet/AspNetCore/blob/master/eng/tools/RepoTasks/GenerateSharedFrameworkDepsFile.cs)

[nguerrera]

I suppose we could also update the tool that generates this manifest

That seems reasonable at first glance. You could override the build and revision of assembly version portions to 0. This whole system of dealing with things that are sometimes packages and sometimes in frameworks is quite complex. I would love to get other thoughts on this. @dsplaisted @ericstj

[wtgodbe]

That seems reasonable at first glance. You could override the build and revision of assembly version portions to 0. This whole system of dealing with things that are sometimes packages and sometimes in frameworks is quite complex.

Do you know if the FileVersions matter in the manifest? The entries look like:

Microsoft.Extensions.Options.dll|Microsoft.AspNetCore.App.Ref|3.0.2.0|3.0.219.60403

We could have the tool zero-out the last 2 portions like you suggested, but we'd still be "lying" about the FileVersion.

[nguerrera]

I'm thinking that the file version could be left alone as it is a tie breaker and any assembly in a package that is newer than 3.0.0 would have a higher assembly version anyway.My confidence on this is not 100% yet though, thinking about the sorts of validation we could do.

[nguerrera]

Safest would probably be to find a reasonable way to revert to 3.0.0 manifest, I suppose.

[dougbu]

@wtgodbe what's the blocker here -- flaky tests or something deeper?

[jamshedd]

Approved for Feb for 3.1.

[analogrelay]

@wtgodbe can you retarget this PR to 3.1?

[ericstj]

However, this is obviously not the case for native dlls from this issue. That turns the platform manifest filtering from a trimming optimization to a correctness issue for native dlls. It also suggests to me that taking a native assembly in-box is a roll-forward breaking change, which is not obvious...

How do you figure? Conflict resolution does handle native DLLs, between 1.x and 2.x we took all files (assembly and native dll) and transitioned from package to "inbox".

The deps file should contain the fileversion for native files and the host should honor this when comparing the app's version of the native file vs the one from the shared framework.

With respect to questions around file version: we need this thing to represent the assets in the minimum shared framework the app targets. So if an app is getting a runtimeconfig that says it runs on 3.1.0 the platform manifest that the SDK uses ought to have the file versions from 3.1.0 (and not 3.1.1 or 3.1.n). If the app is getting a runtimeconfig that says it runs on 3.1.n, then it should be given a platform manifest with the versions from 3.1.n.

[wtgodbe]

With respect to questions around file version: we need this thing to represent the assets in the minimum shared framework the app targets. So if an app is getting a runtimeconfig that says it runs on 3.1.0 the platform manifest that the SDK uses ought to have the file versions from 3.1.0 (and not 3.1.1 or 3.1.n). If the app is getting a runtimeconfig that says it runs on 3.1.n, then it should be given a platform manifest with the versions from 3.1.n.

@ericstj in that case, given that we've patched the ref pack, and therefore can't really generate a correct platform manifest based on what we're building in 3.0.n, would it be better to just harvest the ref pack from 3.0.0 & use its platform manifest?

The trickier question is 3.1.n - we never produced a correct manifest there (3.1.0 has the bug), so we can't harvest it. I suppose we could just check in a file that represents what the correct version would have looked like...

[nguerrera]

How do you figure? Conflict resolution does handle native DLLs... and the host should honor this when comparing the app's version of the native file vs the one from the shared framework.

This is not the behavior that I observe from the host. I see a 2.x native dll in deps.json and app local winning over 3.x native dll in framework. I will try to produce a repro and log a bug. It seems to me that this increased the severity of a missing native dll in the manifest. If the host had conflict resolved this at runtime, the impact would be publishing with the old native dll, but not using it and crashing at runtime.

[nguerrera]

I've filed dotnet/runtime#1459

[wtgodbe]

@nguerrera @ericstj do either you have a suggestion regarding:

The trickier question is 3.1.n - we never produced a correct manifest there (3.1.0 has the bug), so we can't harvest it. I suppose we could just check in a file that represents what the correct version would have looked like...

It looks like we'll only take this fix in 3.1, for 3.1.2. We'll have to generate a ref pack for that release, with a platform manifest that refers to the contents of the 3.1.0 runtime pack. However, we can't just harvest the manifest from 3.1.0, because it was also incorrect. Is there anything better than just manualy generating a correct manifest based off of the SHA we used to build 3.1.0, and checking that in to the repo?

[Pilchie]

We'll have to generate a ref pack for that release, with a platform manifest that refers to the contents of the 3.1.0 ref pack

with a platform manifest that refers to the contents of the 3.1.0 runtime pack, right?

[wtgodbe]

with a platform manifest that refers to the contents of the 3.1.0 runtime pack, right?

That's correct, edited

[nguerrera]

I think checking in a known good manifest is reasonable at this stage.

[wtgodbe]

Closing in favor of #18250