[PATCH] D53567: [Support] Enable color diagnostics for mintty

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In https://reviews.llvm.org/D53567#1275043, @kristina wrote:

> I think **if** there's interest in this and mock-based tests are too difficult, it's possible to have one of the Windows builders run a very specific test with `mintty` just so we can watch out if something does go wrong. That said, I think for now it should need an opt-in (possibly through `-mllvm`) flag, and if we do get one or more Windows builders to run it (it should be relatively lightweight as it's literally just testing the support library) and it remains stable across `mintty` releases, and someone maintains it (I'm up for that, can even supply a builder just for this specific, hypothetical test), the opt-in requirement could eventually be dropped in favor of an "auto" style thing just as with other color diagnostics.
>
> I may be overthinking it, but I think that's a semi-reasonable plan for rolling this out and I think a tool-style test with FileCheck would be the most appropriate here?
>
> Anyway, would like to hear some opinions and seeing how many are interested in such a feature and what they think of the roll-out plan I proposed, despite it sounding slightly (over-)complicated.


I don't think we want to try to maintain some kind of mintty integration test harness infrastructure. It's hard to make these kinds of tests reliable, and when they fail, it's often a signal that something in the environment changed, not LLVM, like a new version of mintty that uses differently named pipes.

And, even if we had this test and it worked, it's not the kind of information an LLVM contributor wants to get from a buildbot somewhere, or a new contributor the first time they run the test suite on a new machine. We want the test suite to have a high likelihood of passing by default, with as few environmental requirements of dependencies as possible. Our coreutils dependence here gets in the way on Windows, of course, and causes all kinds of problems.

If we were to test this, I'd suggest using a unit test that creates appropriately named pipes and re-lauches itself as a subprocess and then emits some colored output to stdout. The parent process would read from the named pipe and look for some ANSI escape codes.



================
Comment at: lib/Support/Windows/Process.inc:231
 
+#if defined(__MINGW32__)
+// Hack to detect mintty, ported from vim
----------------
SquallATF wrote:
> rnk wrote:
> > This is plain old win32 API code. Why make it mingw-specific? Then clang-cl will use colors in mintty. I previously used it as my primary development environment, even for targeting MSVC. Today I use cmd, since it's improved greatly, but it's still pretty janky.
> 1. GetFileInformationByHandleEx will failed build on windows xp without FileExtd.lib, does llvm need support windows xp?
> 2. I did not want to affect MSVC users, if not necessary.
I think the XP ship has sailed. I think our STL requirements now require using an MSVC CRT version that doesn't support XP.

I would also like this feature in MSVC builds, and less ifdefs are good if possible. I'm willing to apply this patch locally and fix any compatibility issues in it.


Repository:
  rL LLVM

https://reviews.llvm.org/D53567