Default SentryOptions.Debug = true for Debug build configurations
#4330
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,4 +23,27 @@ public static void Initialize(Dictionary<string, string> properties) | |
| { | ||
| Values ??= properties.AsReadOnly(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Tries to retrieve a boolean value from the build properties. | ||
| /// </summary> | ||
| /// <param name="key">The item key</param> | ||
| /// <param name="value">The value (if any)</param> | ||
| /// <returns>True if the key was found, false otherwise</returns> | ||
| public static bool TryGetBoolean(string key, out bool value) | ||
|
There was a problem hiding this comment. suggestion: make I don't think this needs to be accessible from the user's assembly where the Generator is emitting source to. On the same note ... I think that the only member here, that needs to be accessible from the user's assembly, is the method So I'm wondering if we should also make the Technically a breaking change, but since our source generator wasn't emitting any source code up until now, this Perhaps |
||
| { | ||
| value = false; | ||
| if (!(Values?.TryGetValue(key, out var foundValue) ?? false)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (!bool.TryParse(foundValue, out var result)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| value = result; | ||
| return true; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,16 +13,15 @@ static AotHelper() | |
| IsTrimmed = CheckIsTrimmed(); | ||
| } | ||
|
|
||
| [UnconditionalSuppressMessage("Trimming", "IL2026: RequiresUnreferencedCode", Justification = AvoidAtRuntime)] | ||
| private static bool CheckIsTrimmed() | ||
| { | ||
| if (BuildProperties.TryGetBoolean("PublishTrimmed", out var trimmed)) | ||
| { | ||
| return trimmed; | ||
| } | ||
|
|
||
| if (BuildProperties.TryGetBoolean("PublishAot", out var aot)) | ||
|
There was a problem hiding this comment. I think this is a major bug that was introduced by the initial PR including the source generator... There was a problem hiding this comment. Great catch! There was a problem hiding this comment. Fixed in #4333: Changed the generated code to use case-insensitive lookup: -BuildProperties.Initialize(new Dictionary<string, string>()
+BuildProperties.Initialize(new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) |
||
| { | ||
| return aot; | ||
| } | ||
|
|
@@ -31,19 +30,4 @@ private static bool CheckIsTrimmed() | |
| var stackTrace = new StackTrace(false); | ||
| return stackTrace.GetFrame(0)?.GetMethod() is null; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| using Sentry.CompilerServices; | ||
| using Sentry.Extensibility; | ||
| using Sentry.Http; | ||
| using Sentry.Infrastructure; | ||
|
|
@@ -1324,6 +1325,18 @@ public SentryOptions() | |
| ); | ||
|
|
||
| NetworkStatusListener = new PollingNetworkStatusListener(this); | ||
|
|
||
| #if NET8_0_OR_GREATER | ||
|
There was a problem hiding this comment. suggestion: Remove the preprocessor directive. User code could build a .NET Framework app or a In this case, I don't see a reason for "Auto Debug Mode" not to work. The only thing, that we need to make very sure of, is that the source code that our Generator emits is compatible with There was a problem hiding this comment. Looks like from #4333 we need to leave the There was a problem hiding this comment. Indeed ... I was not able to remove it entirely from the generated source. |
||
| // If source generators are available, we can detect the user's build configuration and use this | ||
| // to infer whether to enable debug mode by default. | ||
| // See: https://develop.sentry.dev/sdk/expected-features/#auto-debug-mode | ||
| string? buildConfig = null; | ||
| BuildProperties.Values?.TryGetValue("Configuration", out buildConfig); | ||
| if (buildConfig == "Debug") | ||
|
There was a problem hiding this comment. question: Case-sensitivity? I wonder if this string equality check should be case insensitive 🤔 buildConfig.Equals("Debug", StringComparison.OrdinalIgnoreCase)There was a problem hiding this comment. I think it should, but we should configure the underlying dictionary to be case insensitive... that way we don't have to worry about it in method calls like this right? There was a problem hiding this comment. Via #4333, the generated lookup type is now case-insensitive: -BuildProperties.Initialize(new Dictionary<string, string>()
+BuildProperties.Initialize(new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)So we can lookup the But comparing the Value of the
Comment on lines
+1333
to
+1335
There was a problem hiding this comment. nitpick(style): Could we make this a one-liner? I find this code a bit verbose. if (BuildProperties.TryGetString("Configuration", out var buildConfig) && buildConfig == "Debug")by adding a new // public static class BuildProperties
internal static bool TryGetString(string key, [NotNullWhen(true)] out string? value)
{
if (Values?.TryGetValue(key, out var foundValue) == true)
{
value = foundValue;
return true;
}
value = null;
return false;
}and perhaps we could make the |
||
| { | ||
| Debug = true; | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
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.
note: We deleted
Sentry.SourceGenerators.targetsin #4333.It was redundant in the NuGet package, as only
Sentry.propsandSentry.targetsare automatically picked up in consuming projects (viaPackageReference), andSentry.SourceGenerators.targetswas neverImported in any of these MSBuild files either.