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.

Sign up for GitHub

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

Jump to bottom

#50575 colorbehavior default should disable colors in android/applemobile #74496

Merged
merged 4 commits into from
Aug 25, 2022
Merged

#50575 colorbehavior default should disable colors in android/applemobile #74496

merged 4 commits into from
Aug 25, 2022

Conversation

mkhamoyan
Copy link
Contributor

Fix #50575 and #51398

@ghost ghost assigned mkhamoyan Aug 24, 2022
@ghost
Copy link

ghost commented Aug 24, 2022

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

Issue Details

Fix #50575 and #51398

Author: mkhamoyan
Assignees: -
Labels:

area-Extensions-Logging
Milestone: -

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan mkhamoyan marked this pull request as ready for review August 24, 2022 15:06
steveisok
steveisok reviewed Aug 24, 2022
View reviewed changes
src/libraries/Microsoft.Extensions.Logging.Console/src/SimpleConsoleFormatter.cs
@@ -157,8 +158,12 @@ private static string GetLogLevelString(LogLevel logLevel)

private ConsoleColors GetLogLevelConsoleColors(LogLevel logLevel)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to have a mobile specific .cs file that implemented a function that returned true for disableColors?

Copy link
Member

Choose a reason for hiding this comment

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

We don't build RID specific versions of these libraries, and don't really plan on adding them. It impacts the build times of these libraries.

See also #53900.

cc @ViktorHofer

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we don't want OOB libraries to have runtime specific build configurations. Instead they are better of using runtime configurations when possible.

@tarekgh tarekgh added os-android os-ios Apple iOS os-tvos Apple tvOS labels Aug 24, 2022
@ghost
Copy link

ghost commented Aug 24, 2022

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #50575 and #51398

Author: mkhamoyan
Assignees: mkhamoyan
Labels:

os-android, os-ios, area-Extensions-Logging, os-tvos
Milestone: -

@mkhamoyan mkhamoyan merged commit ebdb045 into dotnet:main Aug 25, 2022
@mkhamoyan mkhamoyan mentioned this pull request Aug 25, 2022
Closed
@akoeplinger akoeplinger mentioned this pull request Aug 30, 2022
Merged
akoeplinger added a commit that referenced this pull request Aug 31, 2022
@akoeplinger @jkotas
Extensions.Logging.Console: Only check for Android/Apple mobile on ne…
7e22e41 
…tcoreapp configs (#74798)

We can keep the previous behavior before #74496 on non-netcoreapp configs since they only apply to legacy Xamarin.iOS/Android.
That allows us to use the more efficient `OperatingSystem.Is*()` APIs.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@steveisok steveisok steveisok left review comments

@ViktorHofer ViktorHofer ViktorHofer left review comments

@eerhardt eerhardt eerhardt left review comments

@tarekgh tarekgh tarekgh approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

Assignees

@mkhamoyan mkhamoyan

Labels
area-Extensions-Logging os-android os-ios Apple iOS os-tvos Apple tvOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Microsoft.Extensions.Logging.Console.Tests fail on Android
5 participants
@mkhamoyan @steveisok @ViktorHofer @eerhardt @tarekgh