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

Cease all use of Colorama on non-Windows systems. #345

Merged
merged 6 commits into from
Aug 11, 2021

Conversation

coredumperror
Copy link
Contributor

  • Added tests for changed code.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) are documented in the changelog.

This PR was inspired by the frustration and confusion I suffered over the last week, dealing with figuring out why colored logging wan't working on my apps, despite everything seemingly having been configured properly. I eventually tracked it down to the self.stream.isatty() check on colorama's ansitowin32.py file, line 92. Because my apps run in Docker containers, they don't log directly to a TTY, so it was returning False and forcing the ANSI-to-Win32 color code conversion, even though my apps run on Linux. This broke all the colors.

Yes, fixing this is what ConsoleRenderer's force_colors kwarg appears to be for, but I was using an old version of ConsoleRenderer from before that was added, and didn't realize it.

Ultimately, though, my experience showed me that colorama isn't intended to be used on non-Windows systems at all (See tartley/colorama#306). The only thing colorama.init() does is wrap sys.stdout and sys.stderr in the AnsiToWin32 wrapper, which either does nothing on non-Win32 systems (if logging to a TTY) or is actively harmful (if logging to a file).

So I believe that structlog should change to avoid the use of colorama at all unless it's running on Windows. That's what this PR does. I made the following changes:

  1. Replaced the blank color codes that structlog defines if colorama isn't installed with the actual raw ANSI color codes that colorama generates when one uses those constants.
  2. Removed the lazy init code for colorama, since it isn't done lazily on Windows, and shouldn't be done on other OSes at all.
  3. Fixed what appears to be a bug with the force_colors kwarg on Windows. The Windows init code was using self._force_colors, which was always False where that code was being called, even if the force_colors kwarg was True.
  4. Changed the default value of ConsoleRenderer's colors kwarg from "true if colorama is installed" to "True on Windows if colorama is installed, but always True on other OSes".
  5. Updated the docs, tests, and changelog to reflect this change.

I hope I did this right... I don't contribute to GitHub all that often, but your "How to Contribute" guide was freaking great for helping give me the confidence that I was at least mostly doing this right. I ran tox and pre-commit for the first time ever, and it showed me some bugs in my original code, which I fixed before committing.

The one thing I'm a bit iffy on is the tests. I really just updated the "Make sure this crashes if colors=True is sent when colorama isn't installed" test doesn't get run unless it's running on Windows. I'm not really certain that's all that needs to change, or if that's even the right change.

Copy link
Owner

@hynek hynek left a comment

Choose a reason for hiding this comment

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

I'm glad all the typing paid off in the end! :)

You've almost got it, it's just the semantic new lines that got lost in the litany.

I have left instructions that should fix the coverage problems too.

Getting rid of a dependency on most platforms sounds GREAT!

CHANGELOG.rst Outdated Show resolved Hide resolved
docs/getting-started.rst Outdated Show resolved Hide resolved
src/structlog/dev.py Outdated Show resolved Hide resolved
@coredumperror
Copy link
Contributor Author

OK, I've made the changes you suggested, and pushed them to this PR.

Copy link
Owner

@hynek hynek left a comment

Choose a reason for hiding this comment

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

ah sorry forgot about this one. once it's applied, we should be ready to merge!

src/structlog/dev.py Outdated Show resolved Hide resolved
Co-authored-by: Hynek Schlawack <hs@ox.cx>
@hynek hynek merged commit d20fd09 into hynek:main Aug 11, 2021
@hynek
Copy link
Owner

hynek commented Aug 11, 2021

Thanks!

@tartley
Copy link

tartley commented Oct 7, 2021

As the author of Colorama, I'm sorry we caused you trouble, and I approve of this change. I originally created colorama for my own very modest uses, and it since became wildly popular, without a commensurate amount of thought on the design, or effort on the maintenance. Thanks for your patience.

@hynek
Copy link
Owner

hynek commented Oct 8, 2021

Jonathan, please don't be sorry as I know how hard it is to write cross-platform code (Especially for messy topics like I/O.) and it has served us well over the years. As I wrote above, I'm glad to get rid of dependencies in general. If I'd have known that we need only the color codes on non-Windows, this would've been the approach from day 1.

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.

None yet

3 participants