Rename {CLR,DOTNET}_ICU/OPENSSL_VERSION_OVERRIDE

[am11]

It's the only environment variable using CLR_ prefix.

[tarekgh]

This is a breaking change. any strong reason we need to change this now other than consistency?

CC @janvorli

[am11]

This is a breaking change.

It is an undocumented variable (https://github.com/search?q=repo:dotnet/docs%20CLR_ICU_VERSION_OVERRIDE). I can add a fallback if needed and document the DOTNET_ version with the note about CLR_ for < .NET 10 versions (same thing which was done in COMPlus_ vars in .NET 6).

[tarekgh]

We have been recommending it to the users in some situations like #8102 (comment), https://github.com/daxxog, and dotnet/sdk#25069 (comment). Also, users already learned about it and using it, like https://forum.manjaro.org/t/dotnet-3-1-builds-fail-after-icu-system-package-updated-to-71-1-1/114232 and #60439 (comment). Even stack overflow refers to it https://stackoverflow.com/questions/71530497/globalization-errors-when-running-entity-framework-migrations-on-linux. I mean it is implicitly documented.

We want to be carefull not supporting the old one in .NET, the reason is users create docker images and set such environment variable there and anyone can install and use .NET 10 on such images.

If this is not documented in our doc portal, why we care much renaming it?

If you think it is still worth doing it, I would either continue to support the old name in addition to the new one (like aliased name). Or do your suggestion enable the new one in .NET 10 and later with documenting it and create a breaking change doc for the change. Thanks!

[jkotas]

It's the only environment variable using CLR_ prefix.

It is not the only environment variable using CLR_ prefix. There is also CLR_OPENSSL_VERSION_OVERRIDE:

runtime/src/native/libs/System.Security.Cryptography.Native/opensslshim.c

Line 73 in e7d837d

char* versionOverride = getenv("CLR_OPENSSL_VERSION_OVERRIDE");

[jkotas]

I think this is niche-enough that it would be ok to take a breaking change for consistency.

[am11]

Thanks @jkotas, I have updated openssl one as well.

If this is not documented in our doc portal, why we care much renaming it?

Namespacing the environment variables is a good practice in general to avoid collision etc. In user environment (e.g. containers), when listing the environment variables this makes it easier to filter them by the prefix.

[dotnet-policy-service]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

[tarekgh]

_ICU_VERSION_OVERRIDE LGTM. I'll let @bartonjs review the cryptography one.

[AaronRobinsonMSFT]

I'd like to see some documentation for DOTNET_ICU_VERSION_OVERRIDE at https://learn.microsoft.com/dotnet/core/extensions/globalization-icu. Perhaps we can document DOTNET_OPENSSL_VERSION_OVERRIDE somewhere also? @bartonjs, is it worth documenting?

[tarekgh]

Created the breaking change issue dotnet/docs#43828.

[tarekgh]

I have opened the PR dotnet/docs#43840 to update the docs. I appreciate if you can review. Thanks!