Skip to content
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

Make dotnet publish use Release by default in 8.0+ Projects #29155

Merged
merged 74 commits into from
Jan 27, 2023

Conversation

nagilson
Copy link
Member

@nagilson nagilson commented Nov 22, 2022

Changes:

  • dotnet publish (AND dotnet pack) use a configuration of Release by default now if the TFM is above 8. dotnet pack will use Release REGARDLESS*** of the TFM.

  • The environment variables to opt in or out for solutions have been removed. The error before you'd get using them was tighter than the new error, and while you won't be warned you need to turn on the ENV var anymore, you just don't need to.

  • There's a new Environment variable that disables the features all-together called DOTNET_CLI_DISABLE_PUBLISH_AND_PACK_RELEASE

  • <Configuration> in a project file never worked to begin with. This no longer considers if you set <Configuration> in your project file. Frankly, if that was supported, it would have dramatic consequences.

  • If you multi-target with 8.0, I initially wrote it so you're going to get the Release behavior for each TFM in dotnet pack. No way to go around that besides making this an 8.0 SDK thing and not a TFM thing. FWIW dotnet publish with TargetFrameworks won't work so it picks whatever you used with -f.

@nagilson nagilson marked this pull request as ready for review November 29, 2022 21:46
@nagilson nagilson requested review from AntonLapounov and a team as code owners November 29, 2022 21:46
@nagilson
Copy link
Member Author

@dotnet/dotnet-cli Gentle ping reminder that this is available for review. Merge conflicts are just NETSDK Errors being added to the .resx in the time I was gone.

Copy link
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.

Good progress.

I think we are missing tests for publishing and packing solutions where the PublishRelease or PackRelease properties aren't specified but there's no conflict between projects. I think currently that will fail.

To get it to work I think we may need to move the default values for the properties back into the .targets files.

@nagilson
Copy link
Member Author

nagilson commented Jan 25, 2023

Waiting for Daniel to approve of the name but @AntonLapounov in this instance it means it will only evaluate one project in the solution for Configuration correctness instead of all of them... saving efficiency/speed, but it cannot as precisely distinguish what is an error and what is not this way. By that, I mean if you add any 8.0 project to an existing solution containing not 8.0 projects, that does not define PublishRelease, give -c, or disable the new feature, it will fail. This will be off by default but can be turned on if the performance hit of evaluating every project is not desired.

I will be updating/letting gewarren know to include this as soon as the name is agreed upon.

@nagilson
Copy link
Member Author

@dsplaisted I have responded to the feedback here. Thanks!

Copy link
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.

I haven't made it through all the tests, but this is looking good so far.

nagilson and others added 9 commits January 26, 2023 10:35
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
…ror message.

Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
….) not actually sure why we are doing this one

Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
@nagilson nagilson merged commit 7f685ff into dotnet:main Jan 27, 2023
@dsplaisted
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants