sdk icon indicating copy to clipboard operation
sdk copied to clipboard

dotnet publish should default to release mode

Open benaadams opened this issue 5 years ago • 23 comments

Currently it defaults to debug (bad for performance of deployed sites, libraries put on nuget, and apps)

While this is obvious when using dotnet publish without parameters as it puts it in a Debug folder; it is not obvious when using an output setting as it is put in the output directory without an obvious Debug moniker

benaadams avatar Jun 29 '19 10:06 benaadams

@KathleenDollard to consider this breaking change.

livarcocc avatar Jun 29 '19 16:06 livarcocc

It's not breaking. It's fixing.

Livar [email protected] schrieb am Sa., 29. Juni 2019, 18:26:

@KathleenDollard https://github.com/KathleenDollard to consider this breaking change.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dotnet/cli/issues/11668?email_source=notifications&email_token=AAAOANE7DEBURGDBUXHJ5ELP46EMXA5CNFSM4H4KD6AKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY334WI#issuecomment-506969689, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAOANDVRHGGH25CIE3JDHTP46EMXANCNFSM4H4KD6AA .

forki avatar Jun 29 '19 16:06 forki

Was pointed out pre-1.0 that this was incorrect behaviour https://github.com/dotnet/cli/issues/477 however the issue was closed but not fixed

There are number of examples of people putting Debug builds into production and Debug libs onto NuGet

benaadams avatar Jun 29 '19 18:06 benaadams

This is not as simple as it might seem.

dotnet publish doesn't explicitly set the configuration to Debug. It just does what msbuild /t:Publish does without a /p:Configuration=X argument. That is not necessarily debug: try it with a Directory.Build.props like this:

<Project>
  <PropertyGroup>
    <Configuration Condition=" '$(Configuration)' == '' ">Release</Configuration>
  </PropertyGroup>
</Project>

Further note that configurations can be arbitrarily customized. There might not even be one called Release in the project. Forcing /p:Configuration=Release by default is most definitely a breaking change. It might not even work. Or it might override to a less efficient configuration than a custom configuration that is not named Release and made to be the default in project code like above. And even when it works, it will certainly break cases where people are relying on getting Debug in the case where they have not passed an explicit configuration.

Microsoft.NET.Sdk chooses Debug by default here: https://github.com/dotnet/sdk/blob/ef7932d094d8cf6bfd567e3de0f852a3abda9fe0/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.props#L23

This applies to more than publish, and like so many other things in those props, it maps to what got spelled out in File -> New in classic projects. Breaking with that ancient tradition will likely have other unintended consequences.

nguerrera avatar Jun 29 '19 20:06 nguerrera

Perhaps a more general way of capturing the requirement is dotnet publish should default to optimized output if it is not overriden.

benaadams avatar Jun 29 '19 20:06 benaadams

There are a lot of different ways to interpret that requirement.

My above comment demonstrates why changing dotnet publish to be equivalent to today’s dotnet publish -c Release is very problematic, technically.

It’s certainly possible that a more involved change could work. We’d need a concrete proposal to evaluate that.

nguerrera avatar Jun 29 '19 21:06 nguerrera

Microsoft.NET.Sdk chooses Debug by default here:

Can publish and build/run be differentiated and the default swapped for publish?

benaadams avatar Jun 29 '19 21:06 benaadams

Possibly. We could set /p:RunningDotNetPublishWithDefaultConfig=true. And change above based on that. Or something. It would mean that dotnet publish and msbuild /t:Publish are different, though.

nguerrera avatar Jun 29 '19 21:06 nguerrera

It would mean that dotnet publish and msbuild /t:Publish are different, though.

Solving the compat worries in one fell swoop! 😉

Also I hear there's a major version (this and the next) which would be an ideal time for such a change 😄

benaadams avatar Jun 29 '19 21:06 benaadams

Just to set expectations, I think we’re way too late for 3, but happy to explore this for 5.

nguerrera avatar Jun 29 '19 21:06 nguerrera

Can this be done for 5.0?

benaadams avatar Jul 17 '20 20:07 benaadams

but happy to explore this for 5.

@nguerrera @KathleenDollard any news?

benaadams avatar Aug 14 '20 11:08 benaadams

Thanks for still pushing them to do the right thing Ben

forki avatar Aug 14 '20 11:08 forki

Recent example of someone having performance issues from not publishing in release mode https://github.com/dotnet/sdk/issues/12834

benaadams avatar Aug 14 '20 12:08 benaadams

@benaadams Sorry, I don’t work on this project or .NET anymore. It’s been five months since I left so I don’t know if there’s been any development on this.

nguerrera avatar Aug 14 '20 13:08 nguerrera

We're going to take another (and hopefully final) run at resolving this for .NET 6.0.

richlander avatar Aug 14 '20 17:08 richlander

Lol

forki avatar Aug 14 '20 17:08 forki

<Project>
  <PropertyGroup>
    <Configuration Condition=" '$(Configuration)' == '' ">Release</Configuration>
  </PropertyGroup>
</Project>

I guess there is no way to set this conditionally based on the target because there is no property that describes the targets that are being built/requested?

tmds avatar Nov 26 '20 08:11 tmds

@richlander here we are, shortly before 6.0 release (no pressure 😄)

DanielHabenicht avatar Oct 28 '21 15:10 DanielHabenicht

moving-goalpost

forki avatar Oct 28 '21 16:10 forki

A little late to the party, but one could say that the issue here is semantics causing confusion. With "publish" one sort of intuitively expect that it is actually about publishing. And not about producing code for debugging.

But taking a step backwards this also should match msbuild.

Perhaps the clean way would be to always have a default release path as well as the default debug path, and to define two new keywords for compiling to debug and to release. And have "publish" deprecated but mirroring the debug for backwards compatibility. Perhaps simply something like todebug and torelease? And if people have multiple configuration profiles obviously allow us to pick a specific one by name

EskeRahn avatar Mar 20 '22 10:03 EskeRahn

FYI: https://github.com/dotnet/sdk/issues/23551

richlander avatar Mar 26 '22 02:03 richlander

We think that we can do this for projects that target .NET 8 or higher, by setting the PublishRelease property to true (if not already set) in the .NET SDK targets if the project targets .NET 8 or higher.

dsplaisted avatar Aug 08 '22 20:08 dsplaisted

This is done and can be closed, right @nagilson?

dsplaisted avatar Mar 02 '23 00:03 dsplaisted

That's correct. We've changed the default to Release for 8.0+ TFMS: https://github.com/dotnet/sdk/pull/29155 ( as well as for pack.)

Was waiting for the release so there were no more 🥅 ;)

nagilson avatar Mar 02 '23 00:03 nagilson