Implement framework-dependent application with apphost.

[peterhuene]

This commit implements building and publishing a framework-dependent
application with the application host.

Building with /p:UseAppHost=true (when a RuntimeIdentifier is specified)
and with /p:SelfContained=false will create an executable apphost that can be
used to run the application against globally installed runtimes.

Fixes dotnet/cli#6237.

[peterhuene]

Marked WIP until https://github.com/dotnet/core-setup/issues/4169 is addressed.

[nguerrera]

Just a heads up: You'll need to assign error codes to these when you merge with my changes. Tests will fail to alert you to it as well.

[peterhuene]

I'll review that PR and take the appropriate steps. Thanks for the heads up.

[nguerrera]

The steps should be:

1. Pick highest error code + 1 (tests will fail if you leave a gap)
2. Add NETSDK1234: to value like the others have now
3. Put StrBegin comment like the others too
4. Update XLF as usual.

There shouldn't be any code changes needed, only resx + xlf are impacted.

[nguerrera]

@dotnet-bot test this please

(I want to see if my newly added error code tests fail as expected)

[dasMulli]

heads-up: this will require a follow-up change to the websdk's web.config transform targets: https://github.com/aspnet/websdk/blob/bbb651e4ce9ca7eb6308babdd698a2c00f4cc208/src/Publish/Microsoft.NET.Sdk.Publish.Targets/netstandard1.0/TransformTargets/Microsoft.NET.Sdk.Publish.TransformFiles.targets#L35-L58

[dsplaisted]

@JunTaoLuo FYI on @dasMulli's comment about updates that will be needed in the web SDK to go along with this

[peterhuene]

There is another target at the end of the file that predicates _IsPortable on RuntimeIdentifier being non-empty, which may or may not need updating as well (I didn't dig too deeply).

If the tasks used by these targets are determining whether or not to use dotnet to run the application, they can be updated to check '$(UseAppHost)' == 'true', which will always be true for self-contained applications and may be true for framework-dependent applications. Additionally, UseAppHost being true also implies that RuntimeIdentifier is not empty.

[peterhuene]

@nguerrera I've updated the PR with the error codes.

[JunTaoLuo]

FYI, I think @vijayrkn would be more familiar with the affected areas in the web SDK.

[vijayrkn]

There is another target at the end of the file that predicates _IsPortable on RuntimeIdentifier being

This is a bug. This target conditions should be same as the other target.

If the tasks used by these targets are determining whether or not to use dotnet to run the application, they can be updated to check '$(UseAppHost)' == 'true',

Yes, the tasks just needs to know if the web.config needs to be updated with "dotnet appname.dll" or "appname.<extension>". We can change it to use 'UseAppHost' instead.

[peterhuene]

@livarcocc approved to merge? The core-setup issue I linked above only affects running with the CWD as the directory containing the apphost. The test I added to run the apphost runs with CWD set to a different directory and points DOTNET_ROOT at the toolset under test.

[livarcocc]

Did we run this through @KathleenDollard ?

[peterhuene]

Sent a detailed email to Kathleen last night.

[dsplaisted]

Building with /p:UseAppHost=true (when a RuntimeIdentifier is specified)
and with /p:SelfContained=false will create an executable apphost that can be
used to run the application against globally installed runtimes.

@peterhuene @eerhardt Is there a reason to require explicitly setting UseAppHost? Should we just default it to true if a RuntimeIdentifier is specified? In other words, shouldn't the default for RID-specific shared framework apps (added in #1053) be to use the apphost?

[nguerrera]

@dsplaisted That's certainly an option, but it's a behavioral change. I can buy an argument that once your RID specific you might as well get the benefits of the app host, but logically these can be separate too.

@eerhardt @steveharter Thoughts?

[peterhuene]

Currently users expect dotnet publish -r $RUNTIME --self-contained=false to mean a self-contained deployment. I'm not against making framework-dependent with apphost the default if you specify a RID, but it's a breaking change that we'd need to carefully consider.

Sorry, I misread Daniel's message. I can see the benefit of defaulting UseAppHost to true and users would need to turn it to false to publish or build without it. The intention of doing it this was was simply to prevent any existing behavioral change.

[dsplaisted]

@peterhuene That's not what I meant. I agree that dotnet publish -r $RUNTIME should still mean a self-contained deployment. What I'm suggesting is that dotnet publish -r $RUNTIME --self-contained false should default to using the apphost, without having to additionally specify /p:UseAppHost=true (or -useapphost, if we add it).

This is still a change in behavior, but I think it's probably more in line with user expectations.

[peterhuene]

This is still a change in behavior, but I think it's probably more in line with user expectations.

Given the amount of, let's say...impassioned, comments on dotnet/cli#6237, I think it would certainly align with user expectations.

I'd like a similar experience with dotnet build and dotnet run as well. I should be able to build and launch a framework-dependent app (assuming compatible with an installed runtime) using the apphost. The expectation of a lot of our users is that an executable is created for a console app for them to run during development (without having to use dotnet too), not just publish.

[KathleenDollard]

This looks good. Let me just confirm a few command line possibilities (you can infer the MSBuild)

framework dependent (dll)
dotnet publish
dotnet publish -r osx-x64 –self-contained=false
dotnet publish -r osx-x64 –with-apphost=false
dotnet publish -r osx-x64 –self-contained=false –with-apphost=false

self contained application (exe)
dotnet publish -r osx-x64
dotnet publish -r osx-x64 –self-contained
dotnet publish -r osx-x64 –self-contained –with-apphost=false

framework dependent with host (exe) (not sure what we should call this)
dotnet publish -r osx-x64 –with-apphost
dotnet publish -r osx-x64 –with-apphost –self-contained=false

Did I leave out any possibilities?

Is there anywhere else we have this kind of difference (nuanced) between MSBuild and dotnet?
with-apphost vs UseAppHost

[dsplaisted]

Is there a spec that specifies --with-apphost as the command-line option? I would go with --use-apphost for consistency.

As far as the command-line possibilities, I see several things I would change, but @peterhuene should probably chime in first as he's more familiar with the details.

[peterhuene]

I'm fine with --use-apphost to match the property if everyone else is (--with-apphost is what I suggested as a possibility to Kathleen).

Let me comment on the above:

dotnet publish

Without a runtime specified, this is considered to be a portable application and users can run it with any installed runtime via dotnet exec. No apphost will be used as no RID was specified. It is an error to publish with the apphost but without a RID specified.

dotnet publish -r osx-x64 –self-contained=false

This publishes as framework-dependent, but without a apphost (current behavior). See above for discussion for possibly making the default with apphost. It would then follow that we might need a --without-apphost or --no-apphost option instead of --with-apphost or --use-apphost.

dotnet publish -r osx-x64 –with-apphost=false

This causes an error from the SDK because you can't publish a self-contained application without the apphost. Users won't have a way of activating the application in a self-contained manner without the apphost.

dotnet publish -r osx-x64 –self-contained=false –with-apphost=false

This would be the same as dotnet publish -r osx-x64 –self-contained=false since --with-apphost currently defaults to false for framework-dependent deployments.

dotnet publish -r osx-x64

This publishes a self-contained application with the apphost (always with the apphost).

dotnet publish -r osx-x64 –self-contained

Same behavior as the above command.

dotnet publish -r osx-x64 –self-contained –with-apphost=false

This is the same error as described above. It doesn't make sense to create a self-contained deployment without an apphost.

dotnet publish -r osx-x64 –with-apphost

This is currently a self-contained deployment and the --with-apphost has no effect (defaults to true for self-contained deployments).

dotnet publish -r osx-x64 –with-apphost –self-contained=false

This publishes a framework-dependent application with the apphost.

[dasMulli]

For comparison: Go has a -buildmode flag that they use to switch between things. While this is mostly what .NET's <OutputType> would be, the idea of surfacing options on the CLI through a single option where they can just add features would be similar.

[dasMulli]

(sry for spamming). I think that --type will also be easier to document since you list the available values with an explanation and users will likely not try to combine switches.

[KathleenDollard]

I like the single option.

I'd like to complete the table with dotnet publish (nothing follows) is Framework-dependent without apphost for back compatibility.

I am not sure type is the right keyword. Anyone have other ideas?

[peterhuene]

@dasMulli suggested --deployment-mode, but I'd like to not make the option name specific to publishing/deploying. Would --mode work?

[dasMulli]

+1 for --mode, I think I accidentally typed that before (then edited it to --type).
alternative could be --method.

[peterhuene]

Ping @nguerrera. I've pushed up changes to make UseAppHost default to true when publishing framework-dependent. The two big changes from the previous commit: I had to move where _TargetFrameworkVersionWithoutV gets set because I need to use it when determining the UseAppHost default and I refactored the tests to check true, false, or unspecified for UseAppHost.

Would you mind re-reviewing? Thanks!

[nguerrera]

@peterhuene Do you have the SHA before the rebase so I can see the diff with what I reviewed last? Tried searching mail and failed, can you check reflog? FWIW, I prefer to hold off on rebasing until reviews are complete so that reviewers can see the iteration-to-iteration diffs easier.

[peterhuene]

One more thing I hope GitHub eventually copies from VSTS :)

The previous commit SHA was 1cd4e95.

[nguerrera]

One more thing I hope GitHub eventually copies from VSTS :)

I didn't know VSTS had something better for this. Until then, consider using git commit --fixup|--squash as the review goes along and git rebase --autosquash when it's done.

[nguerrera]

(I approved again of the latest changes in case the notification wasn't obvious.)

[peterhuene]

Thanks, @nguerrera. @livarcocc approved to merge?

[ericsampson]

I'm just a regular Joe moron trying to wrap my head around all this new-fangled core stuff, but my 2c is that "exe" is going to be confusing for people because they associate it deeply with Windows executables rather than being a cross-platform abbreviation. Instead, what about changing the parameters to be something a little more platform agnostic like "…-no-binary" or "…-no-bin"?

BTW thanks for all the good work, we like the direction that things are moving and how receptive y'all are to user feedback!

[ericsampson]

Also, will the RID argument be optional on any of the three modes? Again, please take pity on my noobness.

[ericsampson]

You know now that I think about it, "binary" might be a bad colloquialism in this case. Maybe just spelling out "-executable"? For better or worse, the abbreviation "exe" has Windows associations.

[peterhuene]

Hi @ericsampson. I would not oppose to spelling out fx-dependent-no-executable if enough people think that exe might be confusing to macOS and Linux users. As a long-time Linux user myself, I personally think that exe being short for executable should be okay, but I can see the point of view that exe being the file extension on Windows might be a source of confusion.

Regarding the RID: the RID is required for using this feature. The SDK will error if it isn't provided. This is because we need to know which NuGet RID-specific package to copy the platform-specific apphost from.

I think there's enough users that expect dotnet build without a RID to generate an executable they can use to run the application, so I think we can potentially make that happen by using the current RID just for generating the apphost in the build output directory. However, I don't think we should publish with an apphost unless a RID is specified because the apphost is platform-specific and publishing without a RID means a "portable" application.

Oh and thanks for the "thanks" 😄 We ❤️ our users and community and realize our software and platforms wouldn't exist without them!

[nguerrera]

Hmm, I'm very confused by this key name and message. You can publish a framework-dependent app without specifying a RID, you just can't get an exe for it without one. This is what happens out of the box with dotnet new mvc; dotnet publish and this is documented as a framework-dependent deployment.

[peterhuene]

The problem with calling that "framework dependent" is that, while it is dependent on a shared framework being installed, it doesn't care which one. I personally think there should be a distinction between a "portable" (RID agnostic) deployment and a "framework-dependent" (RID-specific) deployment, but I don't call the shots.

So if we call dotnet publish without RID a framework-dependent deployment, then what should dotnet publish --mode fx-dependent do? Right now it tries to use the apphost which requires a RID and errors.

Without this change dotnet publish --mode fx-dependent-no-exe does not error, which is fine if we call out it's the default when a RID is not specified.

I'm ok with backing out this commit and having the option text explain that fx-dependent-no-exe is the default without a RID, self-contained is the default with a RID, and fx-dependent requires a RID to use.

[nguerrera]

I think we should just make it so that you get an AppHost whenever you have a RID and don't when you don't. (We can allow /p:UseAppHost=false if you really want to be tied to a RID but not have the exe. This is current self-contained false, but the more I think about it, the more useless that case is so I don't think we need to have a nice CLI spelling for it.)

[peterhuene]

Ok, I'll amend this commit to only change the error text to:

A RuntimeIdentifier must be specified to publish a framework-dependent application with an application host.

and this will only be displayed for dotnet publish --mode fx-dependent. We'll allow dotnet publish --mode fx-dependent-no-exe with or without RID (default for without). dotnet publish --help will explain things as needed.

[nguerrera]

OK. Sounds good.

[nguerrera]

I'd like to consider iterating on this a bit more, but I don't want to block the checkin on it.

I personally think there should be a distinction between a "portable" (RID agnostic) deployment and a "framework-dependent" (RID-specific) deployment

We'd invalidate a lot of knowledge and docs if we say that "framework-dependent" as a general term means tied to particular OS and CPU. I don't call the shots either, it is something we can discuss with the broader team.

So if we call dotnet publish without RID a framework-dependent deployment, then what should dotnet publish --mode fx-dependent do?

I would be in favor of having it just produce a portable app without the apphost:

dotnet publish --mode fx-dependent --> portable framework-dependent app
dotnet publish --mode fx-dependent -r win10-x64 --> framework-dependent win10-x64 app

[peterhuene]

Pushed.

[peterhuene]

Sorry, I pushed up without seeing your comments. I like that proposal, so let me make that happen on the CLI side.

[nguerrera]

For my proposal, I think it only works if we drop the fx-dependent-no-exe mode. Might be worth talking it through some more with others. Feel free to iterate on this with a separate PR.

[peterhuene]

I'll merge when green and we can further that discussion on the CLI side.

I've made a change to the CLI now whereby fx-dependent and fx-dependent-no-exe do the right thing without RID specified (effectively mean the same thing). With RID, the former produces the apphost and the latter does not. The error message I changed just now only results if you explicitly try to set UseAppHost to true without a RID specified.

[peterhuene]

If there are any outstanding questions/concerns/comments regarding the dotnet publish UX to support this feature, let's continue the discussion at dotnet/cli#9460.