-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update self contained/ runtime command line options for build and publish #18837
Update self contained/ runtime command line options for build and publish #18837
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
4d4da1f
to
d549243
Compare
@dsplaisted can I get a quick review here? |
ce1fd38
to
7d4d129
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look good.
Personally I don't think we should have a --no-self-contained
option. I think we should instead just use --self-contained false
. I'd like @KathleenDollard to weigh in on this, and am happy to defer to whatever she thinks is best here.
I also believe that we will now repeat this warning for each (transitively) referenced project. Is that OK, or should we try to make the warning just show up once?
@@ -815,4 +815,8 @@ To install these workloads, run the following command: {1}</value> | |||
<value>NETSDK1177: Failed to sign apphost with error code {1}: {0}</value> | |||
<comment>{StrBegin="NETSDK1177: "}</comment> | |||
</data> | |||
<data name="SelfContainedOptionShouldBeUsedWithRuntime" xml:space="preserve"> | |||
<value>NETSDK1178: The '--self-contained' or '--no-self-contained' option should be used with '-r|--runtime'.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richlander @KathleenDollard To review the warning text here. Perhaps something like the following would be clearer:
When the target runtime is specified with '--runtime' or '-r', the deployment type should be specified with '--self-contained' or '--no-self-contained'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richlander suggested this string in the previous PR reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems a bit long, and I like to reduce wrapping where possible. Maybe:
<value>NETSDK1178: The '--self-contained' or '--no-self-contained' option should be used with '-r|--runtime'.</value> | |
<value>NETSDK1178: The '--self-contained' or '--no-self-contained' option should be used when '--runtime' is specified.</value> |
Daniel, is there a reason you want to lead with the conditional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KathleenDollard It sounded a bit better to me, but it could certainly be reversed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like shorter version better.
How about this one?
One of '--self-contained' or '--no-self-contained' options are required when '--runtime' is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate to keep this going, but...
Either '--self-contained' or '--no-self-contained' is required when '--runtime' is used.
Aligning "One of" and "are" grammatically seems hard ;-)
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets
Outdated
Show resolved
Hide resolved
For what its worth, `--no-self-contained is already an option for publish, so this PR just adds it to build for parity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I had an old pending comment as well on this
I'd like Rich's call on the string in question. I can live with any of the options as I think they are all clear.
@@ -815,4 +815,8 @@ To install these workloads, run the following command: {1}</value> | |||
<value>NETSDK1177: Failed to sign apphost with error code {1}: {0}</value> | |||
<comment>{StrBegin="NETSDK1177: "}</comment> | |||
</data> | |||
<data name="SelfContainedOptionShouldBeUsedWithRuntime" xml:space="preserve"> | |||
<value>NETSDK1178: The '--self-contained' or '--no-self-contained' option should be used with '-r|--runtime'.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems a bit long, and I like to reduce wrapping where possible. Maybe:
<value>NETSDK1178: The '--self-contained' or '--no-self-contained' option should be used with '-r|--runtime'.</value> | |
<value>NETSDK1178: The '--self-contained' or '--no-self-contained' option should be used when '--runtime' is specified.</value> |
Daniel, is there a reason you want to lead with the conditional?
2407ba0
to
3f7b9d0
Compare
@richlander @dsplaisted @KathleenDollard I've updated based on your feedback, thanks! We're hoping to get this in for preview 7 so can I please get another review? |
… SelfContainedCmdOptions
Fixes 4th and 5th bullets of #18832
Description
This change adds
--self-contained
and--no-self-contained
options to thedotnet build
command to achieve parity withdotnet publish
. It also adds a warning for .NET 6+ apps when--runtime
is specified on the CLI without--self-contained
or--no-self-contained
.These changes are all additive and do not impact the existing CLI. Note that this doesn't change functionality, as users can achieve the same result by specifying self contained as a MSBuild property on the command line. This change will make it more straight forward to achieve the same result.
Regression?
No
Risk
Low, this is a new feature and won't effect existing CLI.
Verification
[X] Manual (required)
[x] Automated