-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix publishing self-contained ASP.NET apps #3002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dsplaisted
commented
Mar 2, 2019
- Ignore .json files in runtime packs. Fixes self-contained ASP.NET apps failing to launch with new runtime packs. @pakrym
- Remove UseRefTargetingPacks property. Fixes Remove UseRefTargetingPacks once SDK round-trips through core-sdk #2962
@@ -71,6 +71,7 @@ void AddAsset(string assetPath, string assetType) | |||
assetPath.EndsWith(".map", StringComparison.OrdinalIgnoreCase) || | |||
assetPath.EndsWith(".txt", StringComparison.OrdinalIgnoreCase) || | |||
assetPath.EndsWith(".xml", StringComparison.OrdinalIgnoreCase) || | |||
assetPath.EndsWith(".json", StringComparison.OrdinalIgnoreCase) || |
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.
Should this be an include list instead of exclude list?
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.
The reason it's not an include list is because native assets can have a variety of extensions and I'm not sure if we know what that list is.
Eventually we would like the runtime packs to include a manifest which lists the managed and native files in the pack explicitly, so we don't have to glob for them at all.
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.
https://github.com/dotnet/cli/issues/10537 tracks the manifests.
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.
native assets can have a variety of extensions and I'm not sure if we know what that list is.
Can't we look at the fixed set of runtime packs we have now? Is it not .dll,.exe,.so,.dylib?
Switching to include list until we have the manifests seems reasonable to me to err on the safer side.
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.
But I'm ok with this more targeted fix to unblock things with the least change given that we have an even safer medium term plan.
This version should have a version of the x86 ASP.NET Core runtime pack which is published to the feeds.
FYI @nguerrera @dotnet/dotnet-cli I had to make a few small fixes to get everything to pass: b81ebc6 and 663bab6. I'm going to go ahead and merge. |
….4 (#3002) - Microsoft.DotNet.Cli.Runtime - 5.0.100-alpha1.19480.4