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

Remove Serilog, implement HostLogger integration, and deprecate net6.0 #2197

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

JustinGrote
Copy link
Collaborator

@JustinGrote JustinGrote commented Nov 14, 2024

Eliminate Serilog dependencies and wire up HostLogger to MEL directly after initialization.

I confirmed the HostLogger logs are showing up on the other side (this is using the new logoutputwindow on the vscode side but should look fine with the old LSP trace as well)

image

This shows where the file based logging stops and then resumes inside LSP communication
image

As part of the resolutions, this also drops net6.0 support due to some library changes (which was going to happen anyways as net6.0 is EOL as of November)

Fixes PowerShell/vscode-powershell#5084

@JustinGrote
Copy link
Collaborator Author

JustinGrote commented Nov 14, 2024

Test for the debug adapter is stuck on reading the stream for some reason...

int actualCount = _underlyingStream.Read(buffer, offset, count);
LogData("READ", buffer, offset, actualCount);

EDIT: StdioServerProcess used for testing has some loggerfactory references still. I'll investigate tomorrow. Should still be good for review, works otherwise for LSP and DAP as far as I can tell, just a faulty test.

@dkattan
Copy link
Contributor

dkattan commented Nov 14, 2024

This is awesome! Serilog version mismatches have caused lots of issues for us when launching from our parent project.

@JustinGrote
Copy link
Collaborator Author

I'm having issues with this test even on main, and I suspect it has to do with this deadlock weirdness.
image

@JustinGrote
Copy link
Collaborator Author

@andyleejordan this should be ready but it needs a package update to run tests.
Failed to retrieve information about 'Microsoft.Extensions.Logging.Debug' from remote source 'https://pkgs.dev.azure.com/powershell/2972bb5c-f20c-4a60-8bd9-00ffe9987edc/_packaging/e59e749b-417c-4664-9322-c190bb60de7f/nuget/v3/flat2/microsoft.extensions.logging.debug/index.json'.

I can remove MEL.Debug if you want but I hope to expand it more throughout the extension.

Copy link
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

Heck yeah.

@JustinGrote JustinGrote changed the title Remove Serilog and implement HostLogger integration Remove Serilog, implement HostLogger integration, and deprecate net6.0 Nov 14, 2024
Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks Justin!

@andyleejordan andyleejordan added the Issue-Bug A bug to squash. label Nov 14, 2024
@andyleejordan andyleejordan added this pull request to the merge queue Nov 14, 2024
Merged via the queue into main with commit feabfd5 Nov 14, 2024
8 checks passed
@andyleejordan andyleejordan deleted the fix/RemoveSerilog branch November 14, 2024 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug A bug to squash.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2024.4.0: Unable to start when using Windows PowerShell if older Serilog is already present
4 participants