Allow Opt-In to change the default to Release for Pack/Pub

[nagilson]

Fix for #23551 (comment). Drafting until unit tests are added to assure this works. Fixes the issue, but unlike the fix in this PR #25926 it adds a property through MSBuild.

A concern:

• Do we want a separate property to provide more granular control over Release for pack and publish? I used one for now, I think that use case is quite rare. (YES)

[dotnet-issue-labeler]

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.

[nagilson]

These unrelated tests, and some centered around it, fail here, but they are also failing on main. Should probably make an issue for that? Discussed that it's OK to review this nonetheless.

NativeAotStaticLib_only_runs_when_switch_is_enabled
It_publishes_the_project_with_a_refs_folder_and_correct_deps_file
ItReturnsACommandSpecWhenToolIsInAProjectRef
ItRestoresWithTheSpecifiedVerbosity

[nagilson]

@joeloff If there is a concern with this breaking VS Publish, please reach out to me to discuss.

[joeloff]

@joeloff If there is a concern with this breaking VS Publish, please reach out to me to discuss.

I'll let @vijayrkn weigh in on this, but I think VS should be fine. In VS, the publishing wizard usually lets you choose the configurations and I believe it is also stored in the profile. The wizards usually trigger a solution build programmatically and should be passing a value for the Configuration property.

[nagilson]

Ok, the feature is opt-in now instead of by default. If the build succeeds and no more tests need to be fixed, would appreciate a final review before merging. Shall I also raise a PR reflecting this change here: docs/msbuild-props.md at main · dotnet/docs (github.com) ?

[richlander]

LGTM ... Yes, a PR on docs would be great.

Thanks!

[joeloff]

nit: can you remove the extra space?

[nagilson]

Yes, thanks for the call here on these artifacts.

I installed this extension now https://marketplace.visualstudio.com/items?itemName=mynkow.FormatdocumentonSave&ssr=false#overview so these artifacts of removed changes shouldn't reoccur. Sadly there were some problems elsewhere in the file that also got changed. Not sure if that's going to be annoying in the future if my auto-formatter is fixing the format of code that's in the same file but not relevant to my changes? (IMO it's better to fix the formatting everywhere if the file is being touched anyways, but will make the PRs uglier)

[baronfel]

There are a few files where the only changes are whitespace, like the Publish.targets and WebSdk Publish props. Can you revert those whitespace-only changes to minimize diffs/reduce visual clutter?

[dsplaisted]

I haven't been involved in the recent conversations around this, but I don't get what's going on here. With this PR, it looks like if you set PackRelease or PublishRelease to true, then all builds (whether doing a pack, publish, or a normal build) will be set to release by default.

Is this the intention? I would expect that if you set PublishRelease to true, that would change the default configuration for publish, but non-publish builds would still use Debug.

[nagilson]

@dsplaisted

Is this the intention? I would expect that if you set PublishRelease to true, that would change the default configuration for publish, but non-publish builds would still use Debug.

Yikes, with these new changes that is a problem and I should've considered the implications more. Thank you, will work on that! But I don't think we can tell which command is going to be done early enough for this change to be made here. (Maybe in the MsBuild Invocation target?) But then there are concerns of setting the Configuration later down in the chain. Will talk to @joeloff about this.

Resolved

[richlander]

I would expect that if you set PublishRelease to true, that would change the default configuration for publish, but non-publish builds would still use Debug.

@dsplaisted -- That is indeed the intention.

[dsplaisted]

@richlander Note that as it currently stands, PublishRelease will affect dotnet publish, but not something like dotnet build /t:Publish. That's the best we think we can do, because if you're just calling msbuild targets we don't know during evaluation which ones are going to be called.

Same thing goes for PackRelease and dotnet pack.

[nagilson]

Note that as it currently stands, PublishRelease will affect dotnet publish, but not something like dotnet build /t:Publish. That's the best we think we can do, because if you're just calling msbuild targets we don't know during evaluation which ones are going to be called.

Yep, called that out here: #23551 (comment)

[EskeRahn]

@richlander Note that as it currently stands, PublishRelease will affect dotnet publish, but not something like dotnet build /t:Publish. That's the best we think we can do, because if you're just calling msbuild targets we don't know during evaluation which ones are going to be called.

Same thing goes for PackRelease and dotnet pack.

Any way to set this would certainly be an improvement, but will it not be a cause of confusion having them acting differently?
But why not extend the idea and add a new option: dotnet build /t:PublishRelease and perhaps even dotnet publishrelease?