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

[QUIC] Added tests to check IsSupported. #81481

Merged
merged 6 commits into from
Feb 10, 2023

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Feb 1, 2023

On Windows, straightforward.

On Linux, I'm probing system for libmsquic existence and if so then checking IsSupported. This should discover installed libmsquic without corresponding dependencies like openssl.
I'm also adding test that expects libmsquic on every tested Linux we have in the matrix and suppressing for known gaps.

Contributes to #81447

@ManickaP ManickaP added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 1, 2023
@ghost ghost assigned ManickaP Feb 1, 2023
@ghost
Copy link

ghost commented Feb 1, 2023

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

Issue Details

On Windows, straightforward.

On Linux, I'm probing system for libmsquic existence and if so then checking IsSupported. I couldn't come up with better way to do this unless we want to explicitly list the docker images which should have libmsquic installed.
This should at least discover installed libmsquic without corresponding openssl (i.e. missing dependency).

Contributes to #81447

Adding NO MERGE as this should fail now on Win11.

Author: ManickaP
Assignees: ManickaP
Labels:

NO-MERGE, area-System.Net.Quic

Milestone: -

@ManickaP ManickaP requested review from rzikm and wfurt February 1, 2023 15:39
@ManickaP
Copy link
Member Author

ManickaP commented Feb 1, 2023

Thinking about this some more and I think we should be more aggressive. On Linux, on every docker image, we should expect libmsquic. If it's not there, we should add it. If we cannot, we should file an issue and disable the test temporarily, e.g. dotnet/dotnet-docker#4322 (comment).

@ManickaP ManickaP force-pushed the mapichov/quic-test-supported branch from cf63fb4 to b1615d8 Compare February 3, 2023 12:27
@azure-pipelines

This comment was marked as off-topic.

@@ -20,20 +21,33 @@ public void UnsupportedPlatforms_ThrowsPlatformNotSupportedException()
Assert.ThrowsAsync<PlatformNotSupportedException>(async () => await CreateQuicConnection(new IPEndPoint(IPAddress.Loopback, 0)));
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/73290", typeof(PlatformDetection), nameof(PlatformDetection.IsSingleFile))]
Copy link
Member Author

Choose a reason for hiding this comment

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

@wfurt
Copy link
Member

wfurt commented Feb 8, 2023

ATM depends on #81742

why?

@ManickaP
Copy link
Member Author

ManickaP commented Feb 8, 2023

ATM depends on #81742

why?

Ohh we don't, you have too many PRs named "update msquic" 😄

We need to pull in microsoft/msquic#3394 for alpine, which was merged into transport in dotnet/msquic#120.

Do we know if it got propagated into the helix images already?

@wfurt
Copy link
Member

wfurt commented Feb 8, 2023

alpine-3.17-helix-amd64-20230208010446-6d7b22b and alpine-3.15-helix-amd64-20230208010517-6d7b22b should be ready. However, they did not roll into production yet so it is not possible to use the simplified stamp-less version like alpine-3.17-helix-amd64. Since there probably will be more changes we may not wait.
We can do it as part of this or I was planning to roll it in as contribution to #81801.

@wfurt
Copy link
Member

wfurt commented Feb 8, 2023

the image tags are waiting for dotnet/dotnet-buildtools-prereqs-docker#790. Should hopefully happen today.

@ManickaP ManickaP force-pushed the mapichov/quic-test-supported branch from bac381e to f9241c4 Compare February 8, 2023 19:56
@ManickaP ManickaP force-pushed the mapichov/quic-test-supported branch from 73de4ae to 2ea6ef0 Compare February 9, 2023 11:09
@ManickaP
Copy link
Member Author

ManickaP commented Feb 9, 2023

/azp run runtime-extra-platforms

@ManickaP
Copy link
Member Author

ManickaP commented Feb 9, 2023

/azp run runtime-libraries-coreclr outerloop

@dotnet dotnet deleted a comment from azure-pipelines bot Feb 9, 2023
@dotnet dotnet deleted a comment from azure-pipelines bot Feb 9, 2023
@dotnet dotnet deleted a comment from azure-pipelines bot Feb 9, 2023
@dotnet dotnet deleted a comment from azure-pipelines bot Feb 9, 2023
@dotnet dotnet deleted a comment from azure-pipelines bot Feb 9, 2023
@dotnet dotnet deleted a comment from azure-pipelines bot Feb 9, 2023
@ManickaP
Copy link
Member Author

ManickaP commented Feb 9, 2023

/azp run runtime-extra-platforms

@ManickaP
Copy link
Member Author

ManickaP commented Feb 9, 2023

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -52,6 +55,7 @@ public static partial class PlatformDetection
public static bool IsRedHatFamily => IsRedHatFamilyAndVersion();
public static bool IsNotRedHatFamily => !IsRedHatFamily;
public static bool IsRedHatFamily7 => IsRedHatFamilyAndVersion(7);
public static bool IsCentos7 => IsDistroAndVersion("centos", 7);
Copy link
Member

Choose a reason for hiding this comment

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

can we use the IsRedHatFamily7 above?

Copy link
Member Author

Choose a reason for hiding this comment

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

It includes RHEL (or rather excludes from testing) as well. So no, I don't think that's advisable as we want exact list of images that are failing.

Copy link
Member

Choose a reason for hiding this comment

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

they should be basically identical. but I guess they are not:
dotnet/arcade#10253 (comment)

Perhaps something we should investigate. We have at least one of the runs in container so it should be easy to check.

@ManickaP ManickaP requested a review from CarnaViire February 10, 2023 10:35
@ManickaP
Copy link
Member Author

This is ready for review: @CarnaViire @wfurt

@ManickaP ManickaP removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 10, 2023
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! :shipit:

@ManickaP ManickaP merged commit 5a62a79 into dotnet:main Feb 10, 2023
@ManickaP ManickaP deleted the mapichov/quic-test-supported branch February 10, 2023 17:08
@ghost ghost locked as resolved and limited conversation to collaborators Mar 13, 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.

5 participants