Skip to content
This repository was archived by the owner on Apr 20, 2023. It is now read-only.

Implement mode option for dotnet publish.#9460

Merged
wli3 merged 3 commits intodotnet:release/2.1.4xxfrom
peterhuene:fx-dep-apphost
Jun 15, 2018
Merged

Implement mode option for dotnet publish.#9460
wli3 merged 3 commits intodotnet:release/2.1.4xxfrom
peterhuene:fx-dep-apphost

Conversation

@peterhuene
Copy link
Copy Markdown

@peterhuene peterhuene commented Jun 11, 2018 •
edited
Loading

This PR implements a mode option that can control how an application is
published with the dotnet publish command.

There are three supported modes:

  • self-contained: publishes a self-contained application (same as
    --self-contained).
  • fx-dependent: publishes a framework-dependent application (with an
    application host when a runtime is specified).
  • fx-dependent-no-exe: publishes a framework-dependent application without an
    application host.

The default when publishing without a runtime specified is
fx-dependent-no-exe.

The default when publishing with a runtime specified is self-contained.

fx-dependent requires netcoreapp2.1 or later when a runtime is specified.

The --self-contained option is still supported, but is now hidden so that
users will be encouraged to move to the --mode option.

Fixes #6237.

Implement mode option for dotnet publish.
bc96902 
This commit implements a `mode` option that can control how an application is
published with the `dotnet publish` command.

There are three supported modes:

* self-contained: publishes a self-contained application (same as
--self-contained).
* fx-dependent: publishes a framework-dependent application (with an
application host when a runtime is specified).
* fx-dependent-no-exe: publishes a framework-dependent application without an
application host.

The default when publishing without a runtime specified is
`fx-dependent-no-exe`.

The default when publishing with a runtime specified is `self-contained`.

`fx-dependent` requires netcoreapp2.1 or later when a runtime is specified.

The `--self-contained` option is still supported, but is now hidden so that
users will be encouraged to move to the `--mode` option.

Fixes #6237.
@peterhuene peterhuene requested a review from a team June 11, 2018 22:46
@peterhuene peterhuene mentioned this pull request Jun 11, 2018
Merged
dsplaisted
dsplaisted approved these changes Jun 12, 2018
View reviewed changes
Copy link
Copy Markdown
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.

Should the --mode switch only be a parameter that the CLI understands, or should we have a PublishMode MSBuild property and have the inference of the other properties be based on that in the SDK?

Comment thread src/dotnet/commands/dotnet-publish/LocalizableStrings.resx Outdated
<data name="ModeOptionDescription" xml:space="preserve">
<value>The mode to use when publishing the application.
The 'self-contained' mode publishes the application with the .NET Core runtime.
The 'fx-dependent' mode publishes the application as framework-dependent with an executable if a target runtime is specified.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread src/dotnet/commands/dotnet-publish/LocalizableStrings.resx Outdated
<value>The configuration to publish for. The default for most projects is 'Debug'.</value>
</data>
<data name="PublishModeAndSelfContainedOptionsConflict" xml:space="preserve">
<value>The '--mode' and '--self-contained' options conflict. Specify only one of the options.</value>

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread test/dotnet-publish.Tests/GivenDotnetPublishPublishesProjects.cs Outdated
{
var testInstance = TestAssets.Get(testAppName)
.CreateInstance($"PublishesSelfContained{selfContained}")
.CreateInstance($"PublishApp_{rid ?? "none"}_{mode ?? "none"}")

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@peterhuene
Copy link
Copy Markdown
Author

peterhuene commented Jun 12, 2018 •
edited
Loading

Tests are failing because the framework-dependent apphost needs to find the runtime-under-test, probably with spawning with DOTNET_ROOT like we do in the SDK tests. I think it works locally because I have a globally installed runtime.

I'll make the fix.

Peter Huene added 2 commits June 11, 2018 21:31
Fix PR review feedback.
da2b1b9 
Amending string resources based on suggestions.
Fixing tests that need DOTNET_ROOT set.
Adding args to the asset instance name for `PublishApp`.
Fix publish test for 32-bit environments.
8205dd4 
Ensure the correct DOTNET_ROOT variable is set for the apphost being run.
@peterhuene
Copy link
Copy Markdown
Author

peterhuene commented Jun 13, 2018

@KathleenDollard Here's the PR for what we settled on from the SDK PR for this feature. Would you mind reviewing the UX here and see if there's anything that can be improved? Thanks!

@dasMulli
Copy link
Copy Markdown

dasMulli commented Jun 13, 2018

About the CLI logic vs MSBuild logic:
Everything could be done in both places. It gets more interesting when thinking about scenarios that could include adding additional targets, e.g. /t:LinkNative.
The mode is more or less "syntactic sugar" to easily use the available options. The only benefit of doing it in MSBuild is that it is easier to use for direct msbuild invocation. Currently, one needs to look up the parser code to find out which CLI options correspond to which MSBuild property - or find it in the docs or StackOverflow (PackageOutputPath for example).

@KathleenDollard KathleenDollard mentioned this pull request Jun 13, 2018
Closed
@wli3
Copy link
Copy Markdown

wli3 commented Jun 15, 2018

@dsplaisted thanks! looks this is all green, should we merge it? @livarcocc

@livarcocc
Copy link
Copy Markdown

livarcocc commented Jun 15, 2018

Talked to @peterhuene and we will wait for @nguerrera to be back and take a look before merging.

@wli3
Copy link
Copy Markdown

wli3 commented Jun 15, 2018

Talked to @livarcocc later. This is blocking info bar change. So, let's merge it first and follow up later.

@wli3
Copy link
Copy Markdown

wli3 commented Jun 15, 2018

Added https://github.com/dotnet/cli/issues/9503 to keep track

@wli3 wli3 merged commit f66dc4e into dotnet:release/2.1.4xx Jun 15, 2018
@peterhuene peterhuene mentioned this pull request Jun 18, 2018
Closed
@sbomer sbomer mentioned this pull request Sep 26, 2018
Closed
@wli3 wli3 mentioned this pull request Jan 31, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@dsplaisted dsplaisted dsplaisted approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@peterhuene @dasMulli @wli3 @livarcocc @dsplaisted