Skip to content

suppress colorization if stdout is not a tty #494

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

Merged
merged 2 commits into from
Nov 19, 2015
Merged

suppress colorization if stdout is not a tty #494

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c755662
suppress colorization if stdout is not a tty
denys-duchier Nov 19, 2015
fbeb675
use null logger when stdout is not a tty
denys-duchier Nov 19, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions pythonforandroid/toolchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,16 @@
import argparse
from appdirs import user_data_dir
import sh
from colorama import Style, Fore
if sys.stdout.isatty():
from colorama import Style, Fore
else:
from collections import defaultdict
class colorama_shim(object):
def __init__(self):
self._dict = defaultdict(str)
def __getattr__(self, key):
return self._dict[key]
Style = Fore = colorama_shim()

user_dir = dirname(realpath(os.path.curdir))
toolchain_dir = dirname(__file__)
Expand Down Expand Up @@ -69,7 +78,7 @@ def format(self, record):
# handler and reset the level
logger.setLevel(logging.INFO)
logger.touched = True
ch = logging.StreamHandler(stdout)
ch = logging.StreamHandler(stdout) if sys.stdout.isatty() else logging.NullHandler()
Copy link
Member

Choose a reason for hiding this comment

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

Why use a NullHandler if the stdout isn't a tty? Don't we still want to send the output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if stdout is not a tty, it is not connected to a terminal. which (presumably) means it is actually piped into another script. logging interspersed with actual data is ok for humans, it is very inconvenient for scripts. If the logger actually wrote to a file, then I would have kept it, but since it only writes to stdout that seems neither wise nor convenient. If logging to a file is subsequently added, then I'd be happy to revisit the logic.

Copy link
Member

Choose a reason for hiding this comment

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

So the real intention is to remove all stdout output when not going to a tty, and this just catches most of it (the rest should go through logging too, of course, but just checking).

Does this also mean that e.g. buildozer wouldn't get all the log output when running it via subprocess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the remaining cases of explicit calls to distribute.sh that I have seen, buildozer not only doesn't want the log, but it's actually detrimental to its functionning. Maybe the log should be preserved, but it has to go somewhere else than stdout. I am not certain what the best design should be (I am discovering the new code base).

Copy link
Member

Choose a reason for hiding this comment

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

I think that buildozer actually should be able to receive and display the log, even if it can't right now. I'm not worried about the log being preserved if nothing is using it, but we're always asking for buildozer logs to work out what problems people have, and seeing the full verbose log is important for this. That's really why I'm asking, I wasn't sure if buildozer already does this, but it did seem like it should be able to.

I'll merge this change (it's good anyway), but there probably does need to be some way to achieve this. Probably there should be an additional command line argument to toggle it anyway, so buildozer could toggle it on.

Copy link
Member

Choose a reason for hiding this comment

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

Also, just to be clear, a buildozer target for this toolchain would call the p4a executable (or maybe manually call the python), the distribute.sh is only there to give a clear error if people try the old-toolchain method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I am leaning towards calling the python so p4a doesn't have to be "installed".

Copy link
Member

Choose a reason for hiding this comment

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

You can also run toolchain.py file directly, it's the same.

formatter = LevelDifferentiatingFormatter('%(message)s')
ch.setFormatter(formatter)
logger.addHandler(ch)
Expand Down