Made compilation error colors come through et. #57174
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
Ahhhh, nertz! This breaks the gcc path fix since it breaks the regular expression. I'm putting this back into Draft and removing reviewers. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. |
| environment: rbeConfig.environment, | ||
| environment: { | ||
| ...rbeConfig.environment, | ||
| 'CLICOLOR_FORCE': '1', |
There was a problem hiding this comment.
I wonder if this is correct, that is, if this should be conditionally set if the current environment is an ANSI terminal with color support.
If it was conditionally set, would we need the parsing and massaging you did above?
There was a problem hiding this comment.
The behavior most people follow is if you aren't connected to a tty you don't emit ansi escape sequences. That's the behavior ninja was doing. I can emulate it. It's only a problem if someone wants to run et as a subprocess.
There was a problem hiding this comment.
Done, now we only show the colors if stdout supports it or if it has been overwritten with CLICOLOR_FORCE.
There was a problem hiding this comment.
If it was conditionally set, would we need the parsing and massaging you did above?
Yes, we still need it because the gcc path fix is a whole different thing that assumed no ansi escape sequences.
zanderso
left a comment
zanderso
left a comment
There was a problem hiding this comment.
It's good practice with colors to have some way to disable them on the command line. Is there a ninja command line option (that could be plumbed into extraNinjaArgs in another PR) that will override the environment variable?
I'll duplicate ninja's behavior and use the environment variable. One sec, it's also related to matans comment. |
Done, see #57174 (comment) |
There was a problem hiding this comment.
Seems reasonable to me. Please get an explicit approval from @zanderso before merging.
| processResult = _dryRunResult; | ||
| } else { | ||
| final bool shouldEmitAnsi = | ||
| io.stdout.supportsAnsiEscapes || io.Platform.environment['CLICOLOR_FORCE'] == '1'; |
There was a problem hiding this comment.
There should be a way to disable color even when attached to an ansi terminal. It doesn't have to be plumbed all the way to the et command line in this PR, but it should at least be exposed, for example as an argument to the constructor of the containing class.
There was a problem hiding this comment.
Done, I made the negation turn off colors too despite them being supported. I'm not sure of the use-case where they are supported but someone wouldn't' want them. Usually they are disabled to support parsing.
gaaclarke
added
the
autosubmit
…160290) flutter/engine@f0ff4f2...c4e3cc2 2024-12-13 [email protected] [skwasm] Fix implementation of SkwasmPath.relativeLineTo (flutter/engine#57201) 2024-12-13 [email protected] Made compilation error colors come through et. (flutter/engine#57174) 2024-12-13 [email protected] Roll Fuchsia Linux SDK from iWMEbVYaNdH8RJmXZ... to iYz_WkWk1uPr8BLSZ... (flutter/engine#57198) 2024-12-13 [email protected] [web] allow imports to line-break (flutter/engine#57170) 2024-12-13 [email protected] [ios]limit web view not tappable workaround to a limited depth (flutter/engine#57193) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from iWMEbVYaNdH8 to iYz_WkWk1uPr If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
fixes flutter#147931 environment variable documented in github issue: ninja-build/ninja#2196 ## screenshot of results <img width="712" alt="Screenshot 2024-12-13 at 10 18 15 AM" src="https://github.com/user-attachments/assets/571da2d8-065d-4b94-8ca2-a5bef5150dc7" /> ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/engine/blob/main/docs/testing/Testing-the-engine.md [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md
3 participants
fixes flutter/flutter#147931
environment variable documented in github issue: ninja-build/ninja#2196
screenshot of results
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.