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

Issue a deprecation warning for COMPlus_ prefix #107759

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

am11
Copy link
Member

@am11 am11 commented Sep 12, 2024

Before removing it completely, we can deprecate it for .NET 10:

$ COMPlus_DefaultStackSize=5 artifacts/bin/coreclr/osx.arm64.Debug/corerun foo.dll
Warning: COMPlus_ prefixes are deprecated and will be removed in future releases. Use DOTNET_ prefix in COMPlus_DefaultStackSize instead.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 12, 2024
Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@AaronRobinsonMSFT
Copy link
Member

@am11 I wanted this when I first implemented it. There was a bit of push back on that front. Particularly from @stephentoub.

@AaronRobinsonMSFT
Copy link
Member

The biggest issue here is the vast number of scripts and other tooling that sets using the COMPlus_ prefix. I'm not sure it is all that costly to retain and in that sense there is only downside to removing it.

@am11
Copy link
Member Author

am11 commented Sep 12, 2024

We replaced all the usages in runtime and diagnostics repos. Are there still scripts using COMPlus_ in internal/non-public areas of .NET?

@AaronRobinsonMSFT
Copy link
Member

We replaced all the usages in runtime and diagnostics repos. Are there still scripts using COMPlus_ in internal/non-public areas of .NET?

Not just in internal and non-public places but sprinkled throughout the .NET ecosystem. There are 20+ years worth of scripts that .NET shops have used and we will (a) be flooding their logs with warnings and (b) potentially forcing them to change scripts in a manner that has no practical benefit. As I've said, I'm not sure the clean-up is worth the potential cost.

@am11
Copy link
Member Author

am11 commented Sep 12, 2024

has no practical benefit

Except it saves one lookup probing environment variable. :)
We are trying to read each unset clr config environment variable twice (DOTNET_ then COMPlus_), when majority users don't have any clr config set in their setups.

I'm not sure the clean-up is worth the potential cost.

If we are not planning to ever remove this fallback / in foreseeable future, then it makes sense to not deprecate it. Otherwise, generally it's the consumer's responsibility to read the docs when upgrading to newer dotnet version; first deprecating, then removing the deprecated feature a few releases later is reasonable.

@AaronRobinsonMSFT
Copy link
Member

Except it saves one lookup probing environment variable. :)

If you can see it on a normal trace, sure. I am dubious this is going to show up in a practical application.

If we are not planning to ever remove this fallback / in foreseeable future, then it makes sense to not deprecate it.

I don't see us ever removing it. That was something I tried when I first wrote the dual look-up. If we are going to deprecate this, I would also avoid the noise and not issue a warning, but instead file a breaking change and just remove it.

@am11
Copy link
Member Author

am11 commented Sep 14, 2024

If you can see it on a normal trace, sure. I am dubious this is going to show up in a practical application.

We avoided additional probe during the P/Invoke of PAL libs: #33236 (comment), so this is similar in spirit. All configs are looked up twice (string concat+utf8-16 conversion+getenv() syscall) unless they are set.. IMHO, at the very least, we shouldn't allow "new" clrconfigs to be used with COMPlus_ prefix.

@AaronRobinsonMSFT
Copy link
Member

This is going to have to go through @jkotas and @stephentoub. They originally gave the most push back. I'm don't see this as a real cost and the potential impact to the ecosystem seems to far outweight the very little win we are getting in practice.

@jkotas
Copy link
Member

jkotas commented Sep 22, 2024

Our official docs say in multiple places that COMPlus_ prefix will continue to work, e.g. https://learn.microsoft.com/dotnet/core/runtime-config/garbage-collector . The first step would be to fix the docs and then wait a few releases.

Also, it would be useful to have some data about COMPlus vs. DOTNET prefix usage in apps out there to see where people really are.

@AaronRobinsonMSFT
Copy link
Member

Also, it would be useful to have some data about COMPlus vs. DOTNET prefix usage in apps out there to see where people really are.

Agree.

@am11 I think we should close this PR or convert it to draft and then file an issue about making this change. It isn't something we are going to be able to due in .NET 10 and my mind immediately goes to .NET 12 being the next opportunity we should make this sort of change, if at all.

@am11
Copy link
Member Author

am11 commented Sep 23, 2024

Our official docs say in multiple places that COMPlus_ prefix will continue to work, e.g. https://learn.microsoft.com/dotnet/core/runtime-config/garbage-collector . The first step would be to fix the docs and then wait a few releases.

This PR is not contradicting what is written in the docs and only issuing a deprecation warning for the future. For old versions support (pre net6.0 and net4x), user can set variable with both prefixes to avoid the warning; we have this pattern in use under org:dotnet where old frameworks support is relevant.

Also, it would be useful to have some data about COMPlus vs. DOTNET prefix usage in apps out there to see where people really are.

I think this would never be easy to assess. For projects using old runtime or desktop clr or the part of code targeting those, this change is not relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants