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

'dotnet run' doesn't work on self-contained apps #811

Merged
merged 3 commits into from
Feb 4, 2017

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Feb 3, 2017

Fixing the build output of self-contained applications to be runnable. Also making 'dotnet run' and Visual Studio F5 work on self-contained applications.

This change also adds an option to append the $(RuntimeIdentifier) to the build output, since the output changes between RIDs. This would be a breaking change, so the option is defaulted to "off".

@davkean - can you check the usage of the <None> item. It is being used because <ReferenceCopyLocalPaths> won't let me rename the file (which is needed for the host executable). These items shouldn't show up in VS since they are added by a Target, right?

Fix #527

/cc @srivatsn

Scenario

When a user sets the <RuntimeIdentifier> property in their project, or does a build with dotnet build -r RID, the build output is not runnable. When calling dotnet run they get errors like:

A fatal error was encountered. The library 'hostpolicy.dll' required to execute the application was not found in 'F:\DotNetTest\DependencyTest\bin\Debug\netcoreapp2.0\'.

Bugs

#527 #791

Workarounds

Publish the self-contained app, and then you can run it.

Risk

The change to the OutputPath is risky this late. But @nguerrera said he needed this change for some desktop work as well. This change makes the output consistent with the output for project.json projects when a RuntimeIdentifier was set.

By default, there is no change to the OutputPath now.

This change only affects self-contained .NET Core apps during build/run and allows run to work.

Performance Impact

Minimal, a few more files are being copied to the output folder during build of a self-contained app.

Regression Analysis

This was a regression from project.json-based projects.

@nguerrera
Copy link
Contributor

For the output path, I don't quite need it, but there are scenarios that are busted without it. dotnet build -r will clobber and I don't think incremental build will do the right thing but I have to check. Here's what I'm thinking, we add 2 knobs: AppendTargetFrameworkToOutputPath and AppendRuntimeIdentifierToOutputPath with respective true, false defaults matching current behavior. Then you can at least opt in if you hit a scenario that is busted and folks have asked repeatedly to opt out of target framework in single-targeting case (in particular, it breaks MVC 5 app built using SDK).

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

LGTM.

I'd like to see comments explaining what the expected build output is for a self-contained app, and how it differs from the publish output. Inspecting the code, my understanding is that building a self-contained app should produce an executable in the output path that is directly runnable (without calling dotnet), but that still loads its NuGet dependencies from the NuGet package cache. When you publish it, you will also get all of the dependencies from NuGet packages copied locally.

As for the OutputPath, I agree we should default to including the RuntimeIdentifier in the OutputPath if there is a RuntimeIdentifier specified.

EDIT: Though for my opinion on the OutputPath, I'm not really taking into consideration the potential breaking impact of changing it this late.

Add options for appending TFM and RID to the output path.  Default the RID option to 'false', since it would be a breaking change at this point.

Add extra comments explaining the difference between build and publish outputs in .NET Core projects.
@eerhardt
Copy link
Member Author

eerhardt commented Feb 4, 2017

I've responded to the feedback. I agree adding the RID folder to the OutputPath is too risky at this point. I've hidden this behavior behind a switch that is defaulted to 'false', as suggested.

<PublishDir Condition="'$(PublishDir)' == '' and '$(RuntimeIdentifier)' != ''">$(OutputPath)$(RuntimeIdentifier)\$(PublishDirName)\</PublishDir>
<!-- ensure the PublishDir is RID specific-->
<PublishDir Condition="'$(PublishDir)' == '' and
'$(AppendRuntimeIdentifierToOutputPath)' != 'true' and
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be '$(AppendRuntimeIdentifierToOutputPath)' == 'true'?

Copy link
Member Author

Choose a reason for hiding this comment

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

If AppendRuntimeIdentifierToOutputPath is true, the RID will already be in the OutputPath. So we don't need to put the RID in again for the PublishDir.

@nguerrera
Copy link
Contributor

Thanks, @eerhardt. LGTM.

@srivatsn
Copy link
Contributor

srivatsn commented Feb 4, 2017

Tagging @MattGertz for approval.

@srivatsn
Copy link
Contributor

srivatsn commented Feb 4, 2017

JoC approved offline.

@srivatsn srivatsn merged commit d20405f into dotnet:master Feb 4, 2017
@eerhardt eerhardt deleted the SelfContainedRun branch February 6, 2017 16:01
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants