Skip to content

Move usages of COMPlus_ var to DOTNET_ fallback#77239

Merged
BruceForstall merged 1 commit intodotnet:mainfrom
am11:feature/coreclr-tools/DOTNET_<env-vars>
Oct 25, 2022
Merged

Move usages of COMPlus_ var to DOTNET_ fallback#77239
BruceForstall merged 1 commit intodotnet:mainfrom
am11:feature/coreclr-tools/DOTNET_<env-vars>

Conversation

@am11
Copy link
Member

@am11 am11 commented Oct 20, 2022

Following up on #77025 for the remaining COMPlus_ usages in src/coreclr/{tools,nativeaot}.

@ghost
Copy link

ghost commented Oct 20, 2022

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.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 20, 2022
@am11 am11 marked this pull request as ready for review October 20, 2022 02:02
BruceForstall
BruceForstall suggested changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except for one thing.

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Oct 20, 2022
AaronRobinsonMSFT
AaronRobinsonMSFT reviewed Oct 20, 2022
View reviewed changes
@am11 am11 force-pushed the feature/coreclr-tools/DOTNET_<env-vars> branch from c2ef3e7 to 0a47e9b Compare October 20, 2022 03:37
@build-analysis build-analysis bot mentioned this pull request Oct 20, 2022
Closed
@am11
Copy link
Member Author

am11 commented Oct 20, 2022

win-x64 test failure is unrelated gc assertion; #76801.

@AaronRobinsonMSFT, what do you think about issuing an obsoletion notice as "warn once" (using a static variable) in this block:

runtime/src/coreclr/utilcode/clrconfig.cpp

Lines 661 to 664 in 4cbe6f9

&& SString::_wcsnicmp(wszName, COMPLUS_PREFIX, LEN_OF_COMPLUS_PREFIX) == 0)
{
wszName += LEN_OF_COMPLUS_PREFIX;
s_EnvNames.Add(wszName, (DWORD) (wszCurr - wszName));

Warning: COMPlus_ prefixed environment variables will be removed in the future, use DOTNET_ prefix instead.

By net10.0, we can remove the COMPlus_ fallbacks and issue an error if COMPlus_ variables are set in the environment (or change the code to nop and issue a deprecation warning: `COMPlus_` variables has no affect. Use `DOTNET_` instead.).

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Oct 20, 2022 •
edited
Loading

@AaronRobinsonMSFT, what do you think about issuing an obsoletion notice as "warn once" (using a static variable) in this block:

All for it but understanding how that works in practice is hard. It was in my original proposal for this work actually. Start reading at #47283 (comment). Unfortunately, I deleted the gist I wrote my proposal in, but the idea is captured in those comments.

@am11 am11 requested a review from BruceForstall October 20, 2022 18:54
@BruceForstall
Copy link
Contributor

BruceForstall commented Oct 24, 2022

@BruceForstall, could you please take another look?

I'll try to look soon.

BruceForstall
BruceForstall approved these changes Oct 25, 2022
View reviewed changes
Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work.

@BruceForstall BruceForstall merged commit ecf677d into dotnet:main Oct 25, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 27, 2022
@am11 am11 deleted the feature/coreclr-tools/DOTNET_<env-vars> branch June 2, 2024 10:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

+1 more reviewer

@BruceForstall BruceForstall BruceForstall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@am11 @AaronRobinsonMSFT @BruceForstall