-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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] Fix test for debug/release version of a library #93449
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThe originally used preexisting platform detection is for runtime not libraries. Fixes #93404
|
@@ -103,6 +103,12 @@ public static bool IsPrivilegedProcess | |||
public static bool IsReleaseRuntime => s_isReleaseRuntime.Value; | |||
public static bool IsDebugRuntime => s_isDebugRuntime.Value; | |||
|
|||
private static readonly Lazy<bool> s_isReleaseLibraries = new Lazy<bool>(() => AssemblyConfigurationEquals("Release")); | |||
private static readonly Lazy<bool> s_isDebugLibraries = new Lazy<bool>(() => AssemblyConfigurationEquals("Debug")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this the same as s_isReleaseRuntime
and s_isDebugRuntime
?
AssemblyConfigurationEquals
checks typeof(string).Assembly
so I think an easier solution would be to add an overload that takes the assembly there instead of IsDebuggable(Assembly assembly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, those are remnants, they shouldn't be here. The two bellow are important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok but why do you need to check DebuggableAttribute instead of adding an overload to the existing AssemblyConfigurationEquals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see in my tests, it returns the configuration of runtime, not the library and the attribute is not present for System.Net.Quic.dll:
Console.WriteLine(typeof(QuicConnection).Assembly.GetCustomAttribute<AssemblyConfigurationAttribute>() is null); // True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see, looks like we disable generating AssemblyConfiguration
attributes for all libraries:
runtime/eng/versioning.targets
Lines 3 to 4 in b9acf73
<!-- Libraries have never generated these attributes so don't let the SDK generate them. --> | |
<GenerateAssemblyConfigurationAttribute>false</GenerateAssemblyConfigurationAttribute> |
5cf81c7
to
06a85f0
Compare
Failures are #92423 |
if (PlatformDetection.IsReleaseLibrary(typeof(QuicConnection).Assembly)) | ||
{ | ||
throw new SkipTestException("Retrieving SSL secrets is not supported in Release mode."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nitpicking, but since there's also 'checked' build possibility, I feel it might be better to check !IsDebugLibrary
instead of IsReleaseLibrary
(I see that implementation-wise this is the same, but it would make a bit more sense to me)
if (PlatformDetection.IsReleaseLibrary(typeof(QuicConnection).Assembly)) | |
{ | |
throw new SkipTestException("Retrieving SSL secrets is not supported in Release mode."); | |
if (!PlatformDetection.IsDebugLibrary(typeof(QuicConnection).Assembly)) | |
{ | |
throw new SkipTestException("Retrieving SSL secrets is supported only in Debug mode."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Baah, I spent too much time writing that 😂 you can disregard since this is merged already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no "checked" version of library, that applies only to CLR.
The originally used preexisting platform detection is for runtime not libraries.
Fixes #93404