port changes in SDK after Microsoft.NET.HostModel.AppHost is created #7373
Conversation
Port dotnet/sdk#3310 by @peterhuene The resource updater was not specifying a CharSet for the Windows API P/Invoke signatures and therefore the ANSI versions of the APIs were being used. This caused an unhandled exception when customizing the apphost on Windows when the intermediate apphost path contained Unicode characters because the ANSI resource updating API couldn't find the file. Fixes AB#900429.
|
@swaroop-sridhar @peterhuene @nguerrera please review it. It is blocking dotnet/sdk#3447 |
|
Looks good to me, just a small suggestion. EDIT: Sorry, more thoughts on second reading. |
Port dotnet/sdk#3427 by @peterhuene with my edit This commit deletes the output apphost when the CreateAppHost task fails from an exception. Partially fixes dotnet#2989 ------------- * deletes the output apphost when the `CreateAppHost` task fails from an exception. * aggreates the Exception for future logging from deleting the failed apphost
| { | ||
| throw new AppHostCustomizationUnsupportedOSException(); | ||
| failedToDeleteApphostException = new FailedToDeleteApphostException(failedToDeleteEx.Message); |
There was a problem hiding this comment.
I would put failedToDeleteEx in innner exception so that the AggregateException has all stack traces.
There was a problem hiding this comment.
Actually, do we need FailedToDeleteException at all? Can we just throw an AggregateException of the deletion exception and the original exception?
The goal was not to lose the info if Delete is also failing, but an arbitrary exception here is going to become a big scary stack trace in MSBuild, right? If we aggregate both, we'll have all the info in that bug report. The SDK wouldn't need to handle AggregateException specially and log it.
There was a problem hiding this comment.
I would also remove the when from the Delete catch.
try
{
// create apphost
}
catch (Exception ex)
{
try
{
// delete apphost
}
catch (Exception deleteEx)
{
throw new AggregateException(ex, deleteEx);
}
throw;
}
| /// </summary> | ||
| public class FailedToDeleteApphostException : AppHostUpdateException | ||
| { | ||
| public string ExceptionMessage { get; } |
There was a problem hiding this comment.
Why isn't this passed to base so that we get it as Exception.Message?
There was a problem hiding this comment.
lgtm (after accommodating Nick's feedback)
@elinor-fung mentioned to me that we should also ensure that the subscription for this component is updated to reflect the latest (that work was done for master but not release).
|
It looks like dotnet/sdk#3447 will be removing a bunch of unit tests, but their equivalents are not here in core-setup. Could we port them over as well? Or at least have an item pointing to any validation gaps introduced with moving the product code over here without the tests? And as @jeffschwMSFT said, if the intent is to port this to release/3.0 as well, the appropriate subscription will need to be added between core-setup and sdk. |
|
We do need a subscription from release/3.0 of core-setup to sdk master and cli master if we don't have it already. |
|
Were the unit tests not ported with the original copy of the code from sdk? |
|
@elinor-fung @jeffschwMSFT I haven't push Nick's suggestion I'll send a small follow up PR |
|
I don’t see any unit tests in core-setup for the AppHost parts of HostModel. It doesn’t look like they were ported with the original copy in #6931 |
|
I filed https://github.com/dotnet/core-setup/issues/7377 to port the unit tests over. |
port changes in SDK after Microsoft.NET.HostModel.AppHost is created Commit migrated from dotnet/core-setup@f5776f8
dotnet/sdk#3458
Need to port
#3427
#3310