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

Fixing File writing for EngineLogger #1415

Merged
merged 2 commits into from
Aug 18, 2023
Merged

Conversation

WardenGnaw
Copy link
Member

With the Natvis Diagnostics Logger work, the file logger would assume fully qualified paths and would try to create it in the root drive, this would fail since it would not have access. The original change also did not close the ChannelLoggers so this PR fixes that.

This PR also runs File.CreateText in MIDebugCommandDispatcher so errors can be caught earlier and not set an invalid file path.

With the Natvis Diagnostics Logger work, the file logger would fail
since it would not close the file.

This PR also runs 'File.CreateText' in MIDebugCommandDispatcher so
errors can be caught earlier and not set an invalid file path.
Copy link
Member

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

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

I am not sure if this is worth the effort since I don't know how often this is used, but ideally we would probably stop holding the log file path as a string, and instead hold it as a StreamWriter since otherwise we could still hit failures when we try to re-open the stream.

Otherwise, LGTM

@WardenGnaw
Copy link
Member Author

I am not sure if this is worth the effort since I don't know how often this is used, but ideally we would probably stop holding the log file path as a string, and instead hold it as a StreamWriter since otherwise we could still hit failures when we try to re-open the stream.

I'll take a stab at moving to StreamWriter. I also noticed there will be a bug if you try to run the command while debugging. Do we want to throw a warning if you try to change the output file mid-debug session?

@gregg-miskelly
Copy link
Member

I'll take a stab at moving to StreamWriter. I also noticed there will be a bug if you try to run the command while debugging. Do we want to throw a warning if you try to change the output file mid-debug session?

Makes sense, or fix it if that is just as easy.

@WardenGnaw
Copy link
Member Author

I'll take a stab at moving to StreamWriter. I also noticed there will be a bug if you try to run the command while debugging. Do we want to throw a warning if you try to change the output file mid-debug session?

Makes sense, or fix it if that is just as easy.

Making it a Stream was a bit difficult, I just added a check to see if a debug session is running and we will throw an error.

@WardenGnaw WardenGnaw merged commit ad8e28a into main Aug 18, 2023
4 of 6 checks passed
@WardenGnaw WardenGnaw deleted the dev/waan/fixFileEngineLogger branch August 18, 2023 22:17
WardenGnaw added a commit that referenced this pull request Oct 5, 2023
Fixing File writing for EngineLogger (#1415)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants