-
Notifications
You must be signed in to change notification settings - Fork 70
Refactor LZMA generation to support Docker and Antares usage #1245
Conversation
8bcb7b6
to
209d75e
Compare
FYI this is necessary to react to dotnet/aspnetcore#3292. Since the baseline is going to rev each package, we'll need new antares archives and updated packages in the lzma for reach runtime update. |
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.NETCore.App" Version="$(MicrosoftNETCoreAppPackageVersion)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to combine this with the webapp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but i wanted this refactoring to be explicitly clear about the types of projects and the scenarios that the offline cache supports. I think the code has been unclear about this in the past because we were trying too hard to optimize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the reason why I'd want to combine this with WebApp is that the web app will also contain an implicit reference to NETCore.App as well. Class library is completely different so I can see why that's a separate scenario.
string line; | ||
for (var i = 0; i < 2; i++) | ||
{ | ||
line = reader.ReadLine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of essentially writing your custom XML parser, is there an xml parse we can use to ensure we have a top level element instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that. Unfortunately, some xml docsfiles contain corrupted xml, so XDocument.Load fails to parse. :-/. It seems VS and roslyn are more forgiving on XML correctness.
By the way, this detected that all .xml files in the restore folder are xmldocs. I didn't find any .xml files in packages which were not xmldocs. Is that your expectation too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I found as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI if there are corrupted xmls, we should probably at least notify the relevant teams that it's not proper XML, even though it may not matter for VS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a really good point. Did a quick check...looks like it was only one file out of 3505.
/Users/namc/dev/aspnet/Universe21/obj/pkgs/netstandard.library/2.0.3/build/netstandard2.0/ref/netstandard.xml The 'p' start tag on line 654 position 2 does not match the end tag of 'th'. Line 654, position 86.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List explicitly as .csproj files the scenarios for which the offline package cache is important Produces new artifacts designed for various scenarios, such as: * Docker (where xml doc files are not needed) * Azure web apps (where 1.x SDKs must still be supported, but xml docs are not needed)
Changes:
Output:
