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

Use FORCE_COLOR to enable colored output in CI #36

Merged
merged 2 commits into from
May 9, 2023

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented May 6, 2023

Changes

  • Reverted 36cbe2f in favor of setting FORCE_COLOR=1 to activate color output in CI

Notes

  • While there isn't a standard for enabling/forcing colored output, there is some agreement about using FORCE_COLOR
    • For context, this all revolves around the shells in GitHub runner not using a tty
    • At any rate, Python, at least, seems to have aggregated around FORCE_COLOR for the most part:
      • pytest had been using PY_COLOR env var for a while...but it never really caught on
      • Initially, Will was against this in Rich; he was eventually convinced that respecting FORCE_COLOR made sense
      • tox has supported it since v4
      • pytest has supported it since v6
      • While pip is supposed to respect FORCE_COLOR, I don't see it working in CI...
  • So, I think this is a better approach than trying to activate it for each tool
    • Perhaps unfortunately, it does require setting the env var in each workflow
    • I couldn't find a way to configure GitHub to inject an env var in to every workflow run...

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16
Copy link
Member Author

rmartin16 commented May 6, 2023

hmm....this is activating the Rich progress bar....creating some definitely undesired CI logs (see Build iOS App).

Looking over the Rich docs, it says:

If the environment variable FORCE_COLOR is set, then color/styles will be enabled regardless of the value of TERM. This is useful on CI systems which aren’t terminals but can none-the-less display ANSI escape sequences.

However, immediately preceding that in the Interactive Mode section:

Some CI systems support ANSI color and style but not anything that moves the cursor or selectively refreshes parts of the terminal. For these you might want to set force_terminal to True and force_interactive to False.

So, given that Rich is basically explicitly referencing this situation....and says "change the way youre initializing Console", I think this deserves changes in Briefcase.

Because this doesn't only affect BeeWare's own CI; this would affect anyone trying to use Briefcase in their own CI with FORCE_COLOR.

@rmartin16
Copy link
Member Author

rmartin16 commented May 6, 2023

In Briefcase, we'd have to really move some things around to control how Console is instantiated since its construction happens at import.

However, it is simple to disable the use of Rich animations based on an arbitrary flag. I pushed a commit to disable progress bars and wait bars if the --no-input switch is used. This produces much better logs when FORCE_COLORS FORCE_COLOR is used.

@freakboy3742, can you please provide your thoughts on these changes?

  • Using FORCE_COLORS FORCE_COLOR instead of turning colors on each tool at a time and for each invocation?
  • Expanding the meaning of --no-input to take on more of a meaning of "non-interactive"?

@freakboy3742
Copy link
Member

  • Using FORCE_COLORS instead of turning colors on each tool at a time and for each invocation?

If there's an emerging environment based standard, it makes sense to me to adopt it. It's a lot easier to set this flag once, and then have every tool pick it up, rather than have to remember to turn on colors for each individual tool invocation.

However - can I get a confirmation on whether the the flag is FORCE_COLOR or FORCE_COLORS? The implementations all seem to say FORCE_COLOR, but a lot of the descriptions here say FORCE_COLORS... I just want to make sure this is a typo, and not some deep seated schism in the community that we're diving into :-)

  • Expanding the meaning of --no-input to take on more of a meaning of "non-interactive"?

Definitely agree that there's a need to be able to disable the animations; I guess my question is whether we should overload --no-input, or use a more traditional TTY detection approach. I'm guessing FORCE_COLOR is overriding default TTY detection, which is why it's made a mess of the iOS logs; not sure if we can pull back the "color, but not TTY" aspect easily.

If we can't - the only practical use of --no-input is in a CI configuration, so I don't fundamentally object to --no-input being overloaded.

@rmartin16 rmartin16 changed the title Use FORCE_COLORS to enable colored output in CI Use FORCE_COLOR to enable colored output in CI May 8, 2023
@rmartin16
Copy link
Member Author

However - can I get a confirmation on whether the the flag is FORCE_COLOR or FORCE_COLORS?

It's FORCE_COLOR; definitely my typo. However, there is definitely discontent with the fact that FORCE_COLOR=0 disables color in some apps....and enables it in others....

  • Expanding the meaning of --no-input to take on more of a meaning of "non-interactive"?

Definitely agree that there's a need to be able to disable the animations; I guess my question is whether we should overload --no-input, or use a more traditional TTY detection approach. I'm guessing FORCE_COLOR is overriding default TTY detection, which is why it's made a mess of the iOS logs; not sure if we can pull back the "color, but not TTY" aspect easily.

So, Rich is simply short-circuiting its "is tty" logic when FORCE_COLOR is set.

I'm somewhat hesitant to introduce our own detection since it's only really relevant to Rich. However, a call to sys.stdout.isatty() is pretty straightforward. The main risk would be inappropriately disabling dynamic Rich elements; although, I guess we could just default --no-input to the inverse of isatty()....not sure if that would break the piping to stdin we're currently using in CI...

Personally, I'd rather let Rich do what it does but allow users to turn it off in non-interactive sessions.

If we can't - the only practical use of --no-input is in a CI configuration, so I don't fundamentally object to --no-input being overloaded.

It's also very useful for quickly creating the helloworld app :)

@freakboy3742
Copy link
Member

I'm somewhat hesitant to introduce our own detection since it's only really relevant to Rich. However, a call to sys.stdout.isatty() is pretty straightforward. The main risk would be inappropriately disabling dynamic Rich elements; although, I guess we could just default --no-input to the inverse of isatty()....not sure if that would break the piping to stdin we're currently using in CI...

Personally, I'd rather let Rich do what it does but allow users to turn it off in non-interactive sessions.

Agreed. This isn't an area where need reinvent wheels. Piggybacking on --no-input, or, in the extreme case, a separate --no-animations option seems more than enough.

If we can't - the only practical use of --no-input is in a CI configuration, so I don't fundamentally object to --no-input being overloaded.

It's also very useful for quickly creating the helloworld app :)

True - but in that case, you probably also want the progress bars so you can see the downloads happening.

@rmartin16
Copy link
Member Author

I went round and round for a bit trying different things...but landed on beeware/briefcase#1267.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

👍

@freakboy3742 freakboy3742 merged commit 3c11d0e into beeware:main May 9, 2023
@rmartin16 rmartin16 deleted the color branch May 9, 2023 04:29
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