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

SupportedLinuxPlatforms_IsSupportedIsTrue fails on distros without quic #82077

Closed
tmds opened this issue Feb 14, 2023 · 13 comments · Fixed by #82108
Closed

SupportedLinuxPlatforms_IsSupportedIsTrue fails on distros without quic #82077

tmds opened this issue Feb 14, 2023 · 13 comments · Fixed by #82108

Comments

@tmds
Copy link
Member

tmds commented Feb 14, 2023

#81481 added a test which hard-codes the availability of quic for the runtime CI configuration:

[ActiveIssue("https://github.com/dotnet/runtime/issues/81901", typeof(PlatformDetection), nameof(PlatformDetection.IsAlpine313), nameof(PlatformDetection.IsInContainer))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/81901", typeof(PlatformDetection), nameof(PlatformDetection.IsAlpine314), nameof(PlatformDetection.IsInContainer))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/81901", typeof(PlatformDetection), nameof(PlatformDetection.IsMariner1), nameof(PlatformDetection.IsInContainer))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/81901", typeof(PlatformDetection), nameof(PlatformDetection.IsCentos7), nameof(PlatformDetection.IsInContainer))]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsLinux))]
public void SupportedLinuxPlatforms_IsSupportedIsTrue()
{
_output.WriteLine($"Running on {PlatformDetection.GetDistroVersionString()}");
Assert.True(QuicListener.IsSupported);
Assert.True(QuicConnection.IsSupported);
}

This test fails on our internal CI builds both on Fedora and RHEL as there is no quic there.

The other test that was added in this PR (SupportedLinuxPlatformsWithMsquic_IsSupportedIsTrue) looks like it is doing the right thing: check the IsSupported based on the availability of the native libmsquic library.

Can we remove SupportedLinuxPlatforms_IsSupportedIsTrue?

Additionally, SupportedLinuxPlatformsWithMsquic_IsSupportedIsTrue can probably be improved by using NativeLibrary to try and load libmsquic, and also verify that when the native library is not present, IsSupported returns false.

@ManickaP @CarnaViire @wfurt @karelz wdyt?

cc @omajid

Known Issue Error Message

Fill the error message using known issues guidance.

{
  "ErrorMessage": "",
  "BuildRetry": false
}
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 14, 2023
@ghost
Copy link

ghost commented Feb 14, 2023

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

Issue Details

#81481 added a test which hard-codes the availability of quic for the runtime CI configuration:

[ActiveIssue("https://github.com/dotnet/runtime/issues/81901", typeof(PlatformDetection), nameof(PlatformDetection.IsAlpine313), nameof(PlatformDetection.IsInContainer))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/81901", typeof(PlatformDetection), nameof(PlatformDetection.IsAlpine314), nameof(PlatformDetection.IsInContainer))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/81901", typeof(PlatformDetection), nameof(PlatformDetection.IsMariner1), nameof(PlatformDetection.IsInContainer))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/81901", typeof(PlatformDetection), nameof(PlatformDetection.IsCentos7), nameof(PlatformDetection.IsInContainer))]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsLinux))]
public void SupportedLinuxPlatforms_IsSupportedIsTrue()
{
_output.WriteLine($"Running on {PlatformDetection.GetDistroVersionString()}");
Assert.True(QuicListener.IsSupported);
Assert.True(QuicConnection.IsSupported);
}

This test fails on our internal CI builds both on Fedora and RHEL as there is no quic there.

The other test that was added in this PR (SupportedLinuxPlatformsWithMsquic_IsSupportedIsTrue) looks like it is doing the right thing: check the IsSupported based on the availability of the native libmsquic library.

Can we remove SupportedLinuxPlatforms_IsSupportedIsTrue?

Additionally, SupportedLinuxPlatformsWithMsquic_IsSupportedIsTrue can probably be improved by using NativeLibrary to try and load libmsquic, and also verify that when the native library is not present, IsSupported returns false.

@ManickaP @CarnaViire @wfurt @karelz wdyt?

cc @omajid

Author: tmds
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@tmds tmds removed the untriaged New issue has not been triaged by the area owner label Feb 14, 2023
@marek-safar marek-safar added this to the 8.0.0 milestone Feb 14, 2023
@ManickaP
Copy link
Member

This is on purpose, we had a big test hole exactly due to optionality of Quic support.
And for the find vs NativeLibrary.Load, once again this is on purpose trying to get the library by other means than loading. As we also want to catch cases where libmsquic is present, but some of its dependencies aren't (we had this exact issue), which would not be caught with regular Load.

But we can figure something out that would suppress that particular test in your environment but not in ours. Do you have any suggestion for this?

@tmds
Copy link
Member Author

tmds commented Feb 14, 2023

This is on purpose, we had a big test hole exactly due to optionality of Quic support.

If this test is meant to verify the the runtime CI configurations include quic, it can be skipped when not running on the runtime CI configurations.

Probably there is already a way to check this. It may be:

public static bool IsInHelix => s_IsInHelix.Value;

@ViktorHofer is this the right property to detect Microsoft CI?

And for the find vs NativeLibrary.Load, once again this is on purpose trying to get the library by other means than loading. As we also want to catch cases where libmsquic is present, but some of its dependencies aren't (we had this exact issue), which would not be caught with regular Load.

SupportedLinuxPlatforms_IsSupportedIsTrue does this check already. You could make it explicit by asserting a NativeLibrary load works.

SupportedLinuxPlatforms_IsSupportedIsTrue verifies msquic is available and loadable on Microsoft CI.

SupportedLinuxPlatformsWithMsquic_IsSupportedIsTrue could just use NativeLibrary.

@wfurt
Copy link
Member

wfurt commented Feb 14, 2023

There is also [Trait(XunitConstants.Category, XunitConstants.IgnoreForCI)] - and we can perhaps flip it.

Preferably, your internal CI probably should include Quic & Https as well @tmds as it is part of the product.
But I understand if that takes some time.

@tmds
Copy link
Member Author

tmds commented Feb 14, 2023

We don't consider it part of the product.
If it were part of the product, source-build .NET would build the native msquic library.

RHEL/Fedora don't provide it either.

That means there is no msquic library that is built (and supported) by Red Hat.

I'm not familiar with what is available or what may become available for quic (msquic or other) in RHEL, and other Linux distros.

Are there distros that build and ship msquic on their own?

@wfurt
Copy link
Member

wfurt commented Feb 14, 2023

Arch Linux seems to https://aur.archlinux.org/packages/msquic
I'm talking with MSQuic team about Alpine as they do not have any story for that.
For RHEL/Fedora/Centos we consume packages they publish and sign on packages.microsoft.com

@wfurt
Copy link
Member

wfurt commented Feb 14, 2023

BTW it would be great IMHO if msquic could become one or OS packages....

@tmds
Copy link
Member Author

tmds commented Feb 14, 2023

As .NET maintainers, we're not looking to maintain msquic.
I have no idea where things are heading for quic support in Linux.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 14, 2023
@tmds
Copy link
Member Author

tmds commented Feb 14, 2023

IgnoreForCI

I think this is meant for skipping tests that are inappropriate for CI, like tests that require some interaction.

We're looking for a condition that explicitly detects the Microsoft CI that is triggered on this repo.

@ManickaP
Copy link
Member

I used the "InHelix" as you suggested @tmds. It's already in PR.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 15, 2023
@jakobbotsch jakobbotsch added blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab labels Feb 17, 2023
@jakobbotsch
Copy link
Member

jakobbotsch commented Feb 17, 2023

This is still failing CI. Edited the OP with known error details.

@jakobbotsch jakobbotsch reopened this Feb 17, 2023
@tmds
Copy link
Member Author

tmds commented Feb 17, 2023

This issue is about this test failing when it is not running in Microsoft CI.
@ManickaP is tracking the failures that happen in Microsoft CI here: #81901.

@tmds tmds closed this as completed Feb 17, 2023
@jakobbotsch
Copy link
Member

Ok. I edited that issue with the known error details instead.

@jakobbotsch jakobbotsch removed blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab labels Feb 17, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants