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

[libmonodroid] correct excess log level that should be info rather than warning. #1959

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

atsushieno
Copy link
Contributor

This should fix #1909

@atsushieno atsushieno requested a review from jonpryor as a code owner July 13, 2018 07:13
@jonpryor
Copy link
Member

This is a wonderfully straightforward solution, but I have one problem with it: It’ll complicate certain Debug scenarios.

Consider: https://github.com/xamarin/xamarin-android/blob/master/Documentation/workflow/DevelopmentTips.md#update-directory

When a Xamarin.Android app launches on an Android device, and the app was built in the Debug configuration, it will create an "update" directory during process startup, printing the created directory to adb logcat:

W/monodroid( 2796): Creating public update directory: `/data/data/Mono.Android_Tests/files/.__override__`

After this change, the above message will only be generated if the developer also does:

adb shell setprop debug.mono.log default

Which is (1) not documented (but could be corrected) and (2) not at all obvious.

I realize it would be far uglier, but I wonder if this would be better, for at least some of the messages:

#ifdef RELEASE
    log_info (LOG_DEFAULT, "Creating public update directory: `%s`", override_dir);
#else   /* ndef RELEASE */
    log_warn (LOG_DEFAULT, "Creating public update directory: `%s`", override_dir);
#endif  /* ndef RELEASE */

-or-, using more macro-fu:

#if defined(RELEASE)
#define log_debug_warn log_info
#else   /* ndef RELEASE */
#define log_debug_warn log_warn
#endif  /* ndef RELEASE */

/* ... */
    log_debug_warn (LOG_DEFAULT, "Creating public update directory: `%s`", override_dir);

@atsushieno
Copy link
Contributor Author

The actual question is: should this be really printed as WARNING at all?

It is our design choice. I believe this specific usage should be notified only when people have problem and try to diagnose the cause of it. IMO it should raise error when monodroid failed to create the directory, with any additional information that lets people correctly understand why.

Base automatically changed from master to main March 5, 2021 23:08
@jonpryor jonpryor requested a review from grendello as a code owner March 5, 2021 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libmonodroid.so is printing out extraneous messages in Release builds.
2 participants