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
dsplaisted
left a comment
There was a problem hiding this comment.
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?
| <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.
@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.
@richlander suggested this string in the previous PR reviews.
There was a problem hiding this comment.
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.
@KathleenDollard It sounded a bit better to me, but it could certainly be reversed.
There was a problem hiding this comment.
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.
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. |
KathleenDollard
left a comment
There was a problem hiding this comment.
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.
| <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.
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-containedand--no-self-containedoptions to thedotnet buildcommand to achieve parity withdotnet publish. It also adds a warning for .NET 6+ apps when--runtimeis specified on the CLI without--self-containedor--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