Skip to content

Conversation

@ManickaP
Copy link
Member

Fixes #75596

@ghost ghost added the area-System.Net.Quic label Jan 13, 2023
@ghost ghost assigned ManickaP Jan 13, 2023
@ManickaP ManickaP requested review from rzikm and wfurt and removed request for rzikm January 13, 2023 13:38
@ghost
Copy link

ghost commented Jan 13, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #75596

Author: ManickaP
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@ManickaP ManickaP assigned CarnaViire and unassigned CarnaViire Jan 13, 2023
@ManickaP ManickaP requested review from CarnaViire and rzikm January 13, 2023 13:38
Copy link
Member

@CarnaViire CarnaViire 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!

if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(null, $"Incompatible MsQuic library version '{version}', expecting '{MsQuicVersion}'");
NetEventSource.Error(null, $"Cannot retrieve {nameof(QUIC_PARAM_GLOBAL_LIBRARY_GIT_HASH)} from MsQuic library: '{status}'.");
Copy link
Member

Choose a reason for hiding this comment

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

Did you check it with both official and manually build package?

Copy link
Member Author

Choose a reason for hiding this comment

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

CI is running with the official packages and I run locally with manually built msquic from the its main branch.

Copy link
Member

Choose a reason for hiding this comment

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

CI would not log events though, would it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this won't get hit unless the library is built without this property, which doesn't exists ATM. It's just checking for the status as we do with all msquic calls.

uint paramSize;
int status;

arraySize = 4;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@ManickaP ManickaP Jan 14, 2023

Choose a reason for hiding this comment

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

How much does this matter if this is called once per the whole application? Or is this more about setting up good examples/usage patterns?
EDIT: I did change it in the code, I'm just curious about the reasoning.

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@ManickaP ManickaP merged commit 258025c into dotnet:main Jan 16, 2023
@ManickaP ManickaP deleted the mapichov/log-msquic-version branch January 16, 2023 09:41
@ghost ghost locked as resolved and limited conversation to collaborators Feb 15, 2023
@karelz karelz added this to the 8.0.0 milestone Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

QUIC and HTTP/3 tests should log which exact version of msquic they use

5 participants