Skip to content
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

Roll forward self-contained apps to latest patch version #2085

Merged

Conversation

dsplaisted
Copy link
Member

  • Bring back behavior where self-contained apps will roll-forward to the latest patch the SDK knows about
  • Add an error message when the version of .NET Core in the assets file is different than what was expected based on current settings: The project was restored using Microsoft.NETCore.App version 2.0.0, but with current settings, version 2.0.6 would be used instead. To resolve this issue, make sure the same settings are used for restore and for subsequent operations such as build or publish. Typically this issue can occur if the RuntimeIdentifier property is set during build or publish but not during restore.
  • Fix various test issues

Related: #1570

@nguerrera @livarcocc @dotnet/dotnet-cli for review

@dsplaisted dsplaisted force-pushed the self-contained-roll-forward branch from 6eb1c43 to bf9a4ef Compare March 23, 2018 23:56
// (1) different from the fixed framework-dependent defaults (1.0.5, 1.1.2, 2.0.0)
// (2) currently available on nuget.org
//
// This allows bumping the versions before builds without causing tests to fail.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've lost this property again. Note that we will need to bump this for security updates before they are available on nuget.org or even to this repo from anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic we had before felt too complex to me, and was never going to test the actual roll-forward behavior (since it always used a fake version to roll forward to when testing). My thinking was that we could set LatestNetCorePatchVersion as a way to override this in tests. Thinking about it more now, I'm not sure how well that would work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nguerrera I've added properties that allow the latest patch for each minor version of .NET Core to be overridden. We could use these to override this from tests like we were doing before, but I think a better solution would be to set the latest version in the CLI via BundledVersions.props.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Love the BundledVersions approach :)

@dsplaisted
Copy link
Member Author

@eerhardt Is it supported to do dotnet foo.dll where foo.dll has a runtimeconfig.json that's empty?

{
  "runtimeOptions": {}
}

Investigating the test failures in this PR revealed that there are tests (eg GivenThatWeWantToReferenceAnAssembly) that build a project with a RID and then do dotnet run on the built DLL. Since the RID is specified the app is considered self-contained and the runtimeconfig.json that's generated is empty. We're it fail to run the DLL on Mac OS in our tests, but pass everywhere else.

This PR rolls the runtime for self-contained apps forward to 2.0.6, where previously 2.0.0 would have been used. So it seems like something may have changed in the runtime here.

@eerhardt
Copy link
Member

@eerhardt Is it supported to do dotnet foo.dll where foo.dll has a runtimeconfig.json that's empty?

I believe that will try to run the app as "self-contained" - meaning all the framework assemblies (especially the hostfxr.dll) need to be in the app folder. Although, it's kind of a weird scenario, using the dotnet muxer to invoke a self-contained app.

It feels like a scenario we shouldn't try to optimize for.

@steveharter would know more.

@dsplaisted dsplaisted force-pushed the self-contained-roll-forward branch from 78582b0 to 84f50b5 Compare March 28, 2018 02:26
@dsplaisted
Copy link
Member Author

@steveharter @eerhardt We tried to isolate a repro for the issue on Mac OS I was asking about, and it looks like it was a regression in the muxer or host that has since been fixed in newer builds. I can provide more details if you're interested.

@steveharter
Copy link
Member

As @eerhardt said the runtimeconfig.json with empty runtimeOptions is expected for a self-contained application because there is no framework name or version to specify.

@dsplaisted I'm not aware of any Mac issues that were fixed around the muxer\host except for a case where you could have a date-mismatched hostfxr and hostpolicy libraries, but that shouldn't be the case for self-contained provided both are rolled-forward together (I assume that we roll both of these libraries together here)

restoredPlatformLibraryVersion,
expectedPlatformPackageVersion));
WriteMetadata(MetadataKeys.Severity, nameof(LogLevel.Error));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to see this refactored a bit so that I can quickly read WriteAdditionalLogMessages and see which issues these are. Right now it's a little buried that all of this is for one specific additional message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nguerrera I refactored this:

private void WriteAdditionalLogMessages()
{
    WriteUnsupportedRuntimeIdentifierMessageIfNecessary();
    WriteMismatchedPlatformPackageVersionMessageIfNecessary();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. 👍

@dsplaisted
Copy link
Member Author

@steveharter This was a self-contained app, but in this scenario it wasn't published, just built and run from the output folder. The output folder ends up with an app executable, app.dll, an "empty" app.runtimeconfig.json, and some other pieces of the host (hostfxr I think), but not the full .NET Core runtime.

The issue occurred when running dotnet app.dll, which is what some of our tests do. The normal way to run the app now is just to run the app executable directly, but I think our tests were authored before this was possible. If the app was built using Microsoft.NETCore.App version 2.0.0, then dotnet app.dll would succeed. If it used version 2.0.3 or higher (there didn't appear to be a version 2.0.1 or 2.0.2), then dotnet app.dll would fail when using dotnet from version 2.1.300-preview2-008290 of the CLI. The failures varied but looked like native crashes (sometimes it would say malloc failed or similar).

The issue stopped repro-ing when we updated to version 2.1.300-preview3-008414 of the .NET CLI.

@eerhardt
Copy link
Member

When you dotnet build/dotnet run a self-contained app, you almost get into this situation.

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.0</TargetFramework>
    <RuntimeIdentifier>win-x64</RuntimeIdentifier>
  </PropertyGroup>
</Project>

However, the difference here is is we don't use the muxer dotnet.exe during dotnet run. Instead, we use the apphost that is placed in the bin directory.

<PropertyGroup Condition="'$(SelfContained)' == 'true'">
<RunCommand Condition="'$(RunCommand)' == ''">$(TargetDir)$(AssemblyName)$(_NativeExecutableExtension)</RunCommand>
<RunArguments Condition="'$(RunArguments)' == ''">$(StartArguments)</RunArguments>
</PropertyGroup>

@dsplaisted
Copy link
Member Author

However, the difference here is is we don't use the muxer dotnet.exe during dotnet run. Instead, we use the apphost that is placed in the bin directory.

Yes, I banged my head against the wall for a bit trying to figure out why dotnet run worked but dotnet app.dll didn't. 🤪

@dsplaisted
Copy link
Member Author

@dotnet-bot test Ubuntu16.04 Debug

@steveharter
Copy link
Member

@dsplaisted the Mac malloc issue you mentioned is very likely the issue https://github.com/dotnet/corefx/issues/27571 with date-mismatched hostfxr and hostpolicy. More than likely the hostfxr was being loaded locally and hostpolicy loaded globally. In either case, that scenario is supported and now fixed.

@dsplaisted
Copy link
Member Author

@steveharter Thanks, that sounds like it explains it.

@dsplaisted dsplaisted merged commit 46d307c into dotnet:release/2.1.3xx Mar 28, 2018
@dsplaisted
Copy link
Member Author

@MattGertz FYI for tell mode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants