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

dotnet.exe prints error messages to console when launched with empty DOTNET_MULTILEVEL_LOOKUP #84322

Merged
merged 10 commits into from
Apr 11, 2023

Conversation

pedrobsaila
Copy link
Contributor

@pedrobsaila pedrobsaila commented Apr 4, 2023

Fixes #54073
Fixes #82260

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 4, 2023
@ghost
Copy link

ghost commented Apr 4, 2023

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #54073

Author: pedrobsaila
Assignees: -
Labels:

area-Host

Milestone: -

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

We should add a test to validate that nothing goes to stderr.

@pedrobsaila
Copy link
Contributor Author

Don't print to stderr on error getting env. variable, just trace a warning (which by default doesn't go anywhere)

since warnings do not go anywhere, checking stderr would not be useful.

pedrobsaila and others added 2 commits April 6, 2023 22:55
Co-authored-by: Vitek Karas <10670590+vitek-karas@users.noreply.github.com>
@vitek-karas
Copy link
Member

since warnings do not go anywhere, checking stderr would not be useful.

We try to add regression tests for pretty much every fix. So in this case the test should fail before the change and pass after. The tests also often enable tracing which will make the warnings visible and verifiable.
And also just checking that stderr is indeed empty in this case is a good test.

@elinor-fung
Copy link
Member

Since we don't always read DOTNET_MULTILEVEL_LOOKUP (since it is disabled in latest versions) and the root issue concerns any environment variable we read, it probably makes sense to target a another specific variable.

Incorrectly writing to stderr in the STARTUP_HOOK case was reported in #82260. We have an existing test that sets an empty value for the STARTUP_HOOK environment variable - that should be updated to check that nothing is written to stderr.

public void Muxer_activation_of_Empty_StartupHook_Variable_Succeeds()

We also have tests setting an empty DOTNET_ROOT:

public void AppHost_FrameworkDependent_GlobalLocation_Succeeds(bool useRegisteredLocation)

Comment on lines 176 to 179
.EnvironmentVariable(Constants.HostTracing.TraceLevelEnvironmentVariable, "1")
.EnvironmentVariable(Constants.HostTracing.VerbosityEnvironmentVariable, "2")
.CaptureStdOut()
.CaptureStdErr()
Copy link
Member

Choose a reason for hiding this comment

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

This can be replaced with .EnableTracingAndCaptureOutputs, it does not set verbosity (so it defaults to everything), but that should work for this test as well.

Similarly in the other places.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Thank you!

@elinor-fung elinor-fung merged commit 7ebb888 into dotnet:main Apr 11, 2023
@pedrobsaila pedrobsaila deleted the 54073 branch April 11, 2023 07:36
@ghost ghost locked as resolved and limited conversation to collaborators May 11, 2023
@agocke
Copy link
Member

agocke commented Jun 17, 2023

/backport to release/7.0-staging

@github-actions github-actions bot unlocked this conversation Jun 17, 2023
@github-actions
Copy link
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/5299837208

@elinor-fung
Copy link
Member

/backport to release/6.0-staging

@github-actions github-actions bot unlocked this conversation Jul 11, 2023
@github-actions
Copy link
Contributor

Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/5522137951

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Host community-contribution Indicates that the PR has been added by a community member
Projects
None yet
4 participants