Page MenuHomePhabricator

[Support] Enable color diagnostics for mintty
Needs ReviewPublic

Authored by SquallATF on Oct 23 2018, 6:23 AM.

Details

Reviewers
kristina
rnk

Diff Detail

Event Timeline

SquallATF created this revision.Oct 23 2018, 6:23 AM

This doesn't look like it is matching the normal coding style - use clang-format(-diff) to get the proper style.

I can't comment much on the changes themselves though.

SquallATF updated this revision to Diff 170797.Oct 23 2018, 6:09 PM
  • run clang-format
kristina added a reviewer: kristina.EditedOct 23 2018, 7:36 PM

Could you please explain the purpose of this patch, since mintty (I use it myself though as as the frontend for wslbridge) is not a native Windows console in any shape or form, unless I'm missing something? In either case this shouldn't really be necessary unless your setup involves running LLVM tools on Windows through mintty through Windows console.

As far as I understand, mintty is a xterm-256color compatible terminal emulator, so I'm not entirely sure why you're using Windows console APIs either?

If I may ask, could you write up a detailed explanation of what this patch is aiming to achieve and what specific problem does it solve?

kristina requested changes to this revision.Oct 23 2018, 8:27 PM
kristina added a reviewer: rnk.
kristina added a subscriber: rnk.

Another major problem I see is the difficulty in adding a test for this specific patch, due to its complicated nature this would require a fairly complex unit test that mocks that specific aspect of mintty, which has not been provided either. Given that this will be linked in by all Windows tools, and the complexity of the patch, I think at the very least this should provide said test.

Let me add that the maintenance burden added by this is fairly significant as it depends on compatibility with a project unrelated to LLVM, very specific naming conventions and the use of native APIs to use the named pipe and should it change, the code will stop working as well as potentially cause regressions for Cygwin/MSYS based toolchains (as they use mintty as well)? So at the very least, this very least this should require a test.

Personally I don't think this should make it into the support library at all, I can see a lot of things going wrong here requiring reverting this. I'm adding @rnk as a reviewer as well, as he is more qualified in this area than me, and I think his opinion would be of high value here (regarding the cost-benefit aspect of this patch, even if other comments are addressed).

This revision now requires changes to proceed.Oct 23 2018, 8:27 PM
  • rewrite detect mintty, port from vim.
SquallATF updated this revision to Diff 170825.Oct 24 2018, 1:02 AM
  • limit this change for mingw only

Another major problem I see is the difficulty in adding a test for this specific patch, due to its complicated nature this would require a fairly complex unit test that mocks that specific aspect of mintty, which has not been provided either. Given that this will be linked in by all Windows tools, and the complexity of the patch, I think at the very least this should provide said test.

Let me add that the maintenance burden added by this is fairly significant as it depends on compatibility with a project unrelated to LLVM, very specific naming conventions and the use of native APIs to use the named pipe and should it change, the code will stop working as well as potentially cause regressions for Cygwin/MSYS based toolchains (as they use mintty as well)? So at the very least, this very least this should require a test.

Personally I don't think this should make it into the support library at all, I can see a lot of things going wrong here requiring reverting this. I'm adding @rnk as a reviewer as well, as he is more qualified in this area than me, and I think his opinion would be of high value here (regarding the cost-benefit aspect of this patch, even if other comments are addressed).

Thanks!

So, funny story, I wanted to do this for ninja (https://github.com/ninja-build/ninja/pull/659) and @thakis didn't like the complexity it added.

If we do decide to do this, I don't think it's the kind of thing we can reasonably test in LLVM, for the reasons @kristina mentioned. If people agree it's a valuable feature that's worth the complexity, I think we can commit it, and rely on mintty users to tell us when the colors don't end up printing.

lib/Support/Windows/Process.inc
231

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.

In D53567#1274707, @rnk wrote:

Another major problem I see is the difficulty in adding a test for this specific patch, due to its complicated nature this would require a fairly complex unit test that mocks that specific aspect of mintty, which has not been provided either. Given that this will be linked in by all Windows tools, and the complexity of the patch, I think at the very least this should provide said test.

Let me add that the maintenance burden added by this is fairly significant as it depends on compatibility with a project unrelated to LLVM, very specific naming conventions and the use of native APIs to use the named pipe and should it change, the code will stop working as well as potentially cause regressions for Cygwin/MSYS based toolchains (as they use mintty as well)? So at the very least, this very least this should require a test.

Personally I don't think this should make it into the support library at all, I can see a lot of things going wrong here requiring reverting this. I'm adding @rnk as a reviewer as well, as he is more qualified in this area than me, and I think his opinion would be of high value here (regarding the cost-benefit aspect of this patch, even if other comments are addressed).

Thanks!

So, funny story, I wanted to do this for ninja (https://github.com/ninja-build/ninja/pull/659) and @thakis didn't like the complexity it added.

If we do decide to do this, I don't think it's the kind of thing we can reasonably test in LLVM, for the reasons @kristina mentioned. If people agree it's a valuable feature that's worth the complexity, I think we can commit it, and rely on mintty users to tell us when the colors don't end up printing.

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.

SquallATF added inline comments.Oct 24 2018, 11:50 PM
lib/Support/Windows/Process.inc
231
  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.
rnk added a comment.Oct 30 2018, 2:54 PM

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.

lib/Support/Windows/Process.inc
231

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.