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.

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

Conversation

denys-duchier
Copy link
Contributor

suppressing colorization makes the output easier to process by other scripts.

@@ -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.

inclement added a commit that referenced this pull request Nov 19, 2015
suppress colorization if stdout is not a tty
@inclement inclement merged commit 33a1616 into kivy:master Nov 19, 2015
@inclement
Copy link
Member

Thanks.

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.

2 participants