Re: [cmake-developers] [CMake] Forcing colorization of output from cmake

2014-10-14 Thread Brad King
On 10/09/2014 01:38 PM, Paul Smith wrote:
 What I really want is that if MAKE_TERMOUT is set to a non-empty value,
 cmake should pretend that it's in a terminal regardless of what isatty()
 says.  We can do this easily enough by adding a simple test to
 Terminal.c:kwsysTerminalStreamIsVT100() _without_ adding any code
 anywhere else:

One problem with this is that non-make processes launched by make may
run their own children to capture the output through pipes and not
unset MAKE_TERMOUT.  We need it to be 'make' that evaluates that env
variable.  Your original command-line approach did this.

 In a way this is gross, certainly, but this function already checks for
 environment variable such as TERM, EMACS, etc. which are set by calling
 utilities and handles them specially.  So why not MAKE_TERMOUT as well?

The TERM and EMACS variables are checked to determine whether the
terminal supports color when isatty() returns true.  That is not
the same as forcing tty behavior.

 I would expect FORCE to be stronger than that, and return
 true regardless of TERM settings.

Okay, but the internal API in KWSys Terminal needs to be more granular
than that.  Please revise your patch to use my KWSys part but also
add a ForceVT100 setting that causes TERM and EMACS to be ignored.
Then the --switch=FORCE can set both the ForceTTY and ForceVT100
setting internally.

 I'm not at all familiar with Windows so I have no strong opinions on
 whether FORCE should also force color output on a Windows console...
 although I know a number of people who do use GNU make on Windows and
 the Windows port of GNU make does set MAKE_TERMOUT properly.

Native Windows terminals do not support the color escape sequences
at all.  We need to get a handle to the underlying console buffer and
set parameters on it directly.  If the output pipe is not connected
to a real console then we cannot get the console handle and print
color at all.

 We already have a CMAKE_COLOR_MAKEFILE option.  Perhaps instead
 of just ON or OFF values it could have a GNU value that enables
 
 Well, it's easy enough to detect if CMAKE_MAKE_PROGRAM is GNU make, if
 we can run it to test the output of ${CMAKE_MAKE_PROGRAM} --version.
 
 The question is, what do you do in the generated makefile if you see
 that it's GNU make?

The CMake generator would recognize CMAKE_COLOR_MAKEFILE==GNU and
enable the GNU-specific content.  CMAKE_COLOR_MAKEFILE is a cache
setting the user could set if they don't like the default.  We can
set the default based on whether the CMAKE_MAKE_PROGRAM is GNU.

 It would need to be the equivalent of:
 
--switch=$(or $(COLOR),$(if $(MAKE_TERMOUT),FORCE))
 
 to preserve the current behavior, where the COLOR variable setting can
 be inherited from the environment.

Yes.

 What if someone runs cmake and it detects GNU make, but then they run
 /my/other/make which is not GNU make?  That would work fine now but
 fail if we generated GNU-specific content in 'Unix Makefiles'
 generators.

The user could always set CMAKE_COLOR_MAKEFILE to ON instead of GNU
to work with other make tools.

 If we had a different generator, like 'GNU Makefiles' for example, then
 people who chose that would clearly expect the results would only work
 with GNU make, but we don't have that.

One day we may create a 'GNU Makefiles' generator to enable use of
GNU-specific features, but that would need to be a more comprehensive
effort than just changing the color option.

Thanks,
-Brad

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers


Re: [cmake-developers] [CMake] Forcing colorization of output from cmake

2014-10-09 Thread Brad King
On 10/08/2014 02:36 PM, Paul Smith wrote:
 Hi all.  Attached please find a proposed patch for the above.  I still
 think there's some slightly awkward redundancy between AssumeTTY and the
 new ForceTTY, but I'm not sure these can actually be combined into one
 flag... certainly not without reworking more of how AssumeTTY is
 interpreted and used than I felt confident with.

In this hunk:

 @@ -68,14 +68,16 @@ void kwsysTerminal_cfprintf(int color, FILE* stream, 
 const char* format, ...)
  #if defined(KWSYS_TERMINAL_SUPPORT_CONSOLE)
CONSOLE_SCREEN_BUFFER_INFO hOutInfo;
HANDLE hOut = kwsysTerminalGetStreamHandle(stream);
 -  if(GetConsoleScreenBufferInfo(hOut, hOutInfo))
 +  if(color  kwsysTerminal_Color_ForceTTY ||
 + GetConsoleScreenBufferInfo(hOut, hOutInfo))
  {
  pipeIsConsole = 1;
  kwsysTerminalSetConsoleColor(hOut, hOutInfo, stream, color);
  }

we cannot enter the if() block unless GetConsoleScreenBufferInfo
returns true because otherwise the hOutInfo will not be initialized
correctly.  In the Windows console code path we really need a handle
to the console from the stream or it cannot work.  Also, once the
pipeIsConsole value is true, the kwsysTerminalSetVT100Color code
path should not be used, so we do not want to force the second code
path either.  Instead ForceTTY should just influence the return
value of kwsysTerminalStreamIsVT100.

The Source/kwsys part of the change actually belongs in a separate
KWSys upstream first.  I've added a modified version of your change
for upstream review and testing here:

 http://review.source.kitware.com/17578

Please try out that version with the rest of your changes.

 it's darn handy to have this just work without having to export
 COLOR='$(if $(MAKE_TERMOUT),FORCE)' in your ~/.bashrc or whatever.
 There's no GNU Makefiles generator, and I couldn't come up with a good
 way to implement this in the generic Unix Makefiles generator.

We already have a CMAKE_COLOR_MAKEFILE option.  Perhaps instead
of just ON or OFF values it could have a GNU value that enables
this behavior.  When initializing it in CMakeGenericSystem.cmake
perhaps it is possible to detect if CMAKE_MAKE_PROGRAM is a GNU
make tool and provide a good default.

Thanks,
-Brad

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers


Re: [cmake-developers] [CMake] Forcing colorization of output from cmake

2014-10-09 Thread Paul Smith
Thanks Brad.  I wrote a bunch more below and left it for posterity.
However thinking more about this I wonder if we couldn't make this
simpler.

What I really want is that if MAKE_TERMOUT is set to a non-empty value,
cmake should pretend that it's in a terminal regardless of what isatty()
says.  We can do this easily enough by adding a simple test to
Terminal.c:kwsysTerminalStreamIsVT100() _without_ adding any code
anywhere else:

   /* Check for a valid terminal.  */
   if(!default_vt100)
 {
 ...
 }
+
+  /* If we're being run from GNU make 4.1+ see if IT thinks we have a TTY.  */
+  const char* termout = getenv(MAKE_TERMOUT);
+  if(termout  termout[0] != '\0')
+{
+return 1;
+}

In a way this is gross, certainly, but this function already checks for
environment variable such as TERM, EMACS, etc. which are set by calling
utilities and handles them specially.  So why not MAKE_TERMOUT as well?

That one change is enough for my particular use-case, without any other
changes (don't need a FORCE setting for color).  I think it would be
useful to allow FORCE (I think this is a good capability to provide; as
I mentioned other tools that support colorization such as GCC etc. to
provide an always-type flag) but it would be decoupled from this GNU
make capability.

Thoughts?


On Thu, 2014-10-09 at 09:55 -0400, Brad King wrote:
 The Source/kwsys part of the change actually belongs in a separate
 KWSys upstream first.  I've added a modified version of your change
 for upstream review and testing here:
 
  http://review.source.kitware.com/17578
 
 Please try out that version with the rest of your changes.

OK.  I see that in your version FORCE only comes into effect if we have
a known good terminal (the new code comes after the check for valid
terminal).

Personally I would expect FORCE to be stronger than that, and return
true regardless of TERM settings.  Whether or not it should override
EMACS env.var. I don't know... I'm on the fence.  In my solution I had
it really FORCE; that is, kwsysTerminalStreamIsVT100() effectively
always was true if --switch=FORCE, regardless of EMACS or TERM.

I'm not at all familiar with Windows so I have no strong opinions on
whether FORCE should also force color output on a Windows console...
although I know a number of people who do use GNU make on Windows and
the Windows port of GNU make does set MAKE_TERMOUT properly.

  it's darn handy to have this just work without having to export
  COLOR='$(if $(MAKE_TERMOUT),FORCE)' in your ~/.bashrc or whatever.
  There's no GNU Makefiles generator, and I couldn't come up with a good
  way to implement this in the generic Unix Makefiles generator.
 
 We already have a CMAKE_COLOR_MAKEFILE option.  Perhaps instead
 of just ON or OFF values it could have a GNU value that enables
 this behavior.  When initializing it in CMakeGenericSystem.cmake
 perhaps it is possible to detect if CMAKE_MAKE_PROGRAM is a GNU
 make tool and provide a good default.

Well, it's easy enough to detect if CMAKE_MAKE_PROGRAM is GNU make, if
we can run it to test the output of ${CMAKE_MAKE_PROGRAM} --version.

The question is, what do you do in the generated makefile if you see
that it's GNU make?  Anything you'd generate into the makefile that
could choose to set --switch=FORCE would have to be GNU-specific (I
can't think of a way to write it using POSIX standard makefile syntax).
It would need to be the equivalent of:

   --switch=$(or $(COLOR),$(if $(MAKE_TERMOUT),FORCE))

to preserve the current behavior, where the COLOR variable setting can
be inherited from the environment.

What if someone runs cmake and it detects GNU make, but then they run
/my/other/make which is not GNU make?  That would work fine now but
fail if we generated GNU-specific content in 'Unix Makefiles'
generators.

If we had a different generator, like 'GNU Makefiles' for example, then
people who chose that would clearly expect the results would only work
with GNU make, but we don't have that.

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers


Re: [cmake-developers] [CMake] Forcing colorization of output from cmake

2014-10-08 Thread Paul Smith
On Wed, 2014-10-08 at 12:55 -0400, Brad King wrote:
 On 10/08/2014 12:15 PM, Paul Smith wrote:
  Maybe we could add another valid value to --switch, like --switch=FORCE
  which would always colorize.
 [snip]
  The best option seems to be to add another flag to the color bitflag
  variable that forces colorization always.
 
 Yes, I think both of the above make sense.  If you want to work on
 it please read CONTRIBUTING.rst from the top of our source tree
 in Git ( http://cmake.org/cmake.git ) and come to the developers
 list with a proposal:

Hi all.  Attached please find a proposed patch for the above.  I still
think there's some slightly awkward redundancy between AssumeTTY and the
new ForceTTY, but I'm not sure these can actually be combined into one
flag... certainly not without reworking more of how AssumeTTY is
interpreted and used than I felt confident with.

I didn't try to allow FORCE to be case-insensitive.  It wouldn't be
hard, but I wasn't sure if it was worth it.  You must capitalize it as
the code is written today.  I found precedent for both options.

Also, the check for MAKE_TERMOUT might not be something you want to
keep... there's not a lot of precedent in the code, that I found, for
looking at other utilities' environment variables.  On the other hand
it's darn handy to have this just work without having to export
COLOR='$(if $(MAKE_TERMOUT),FORCE)' in your ~/.bashrc or whatever.
There's no GNU Makefiles generator, and I couldn't come up with a good
way to implement this in the generic Unix Makefiles generator.
From 82d2c63750c706e5f4f749a123934c725432 Mon Sep 17 00:00:00 2001
From: Paul Smith p...@mad-scientist.net
Date: Wed, 8 Oct 2014 14:18:14 -0400
Subject: [PATCH] cmake: Allow forced color output

Provide a new Terminal.h flag, Color_ForceTTY, which causes color output
always regardless of TTY/Console settings.  If --switch=FORCE is given
on the cmake command line, enable this flag.  Also check for the
MAKE_TERMOUT variable, set by GNU make 4.1+ when make is capturing
stdout which will eventually go to a terminal, and set Color_ForceTTY.
---
 Source/cmSystemTools.cxx   | 36 +---
 Source/cmcmd.cxx   |  8 +++-
 Source/kwsys/Terminal.c|  8 +---
 Source/kwsys/Terminal.h.in | 17 +++--
 4 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/Source/cmSystemTools.cxx b/Source/cmSystemTools.cxx
index fbb4416..7c983c6 100644
--- a/Source/cmSystemTools.cxx
+++ b/Source/cmSystemTools.cxx
@@ -2290,31 +2290,45 @@ std::string const cmSystemTools::GetCMakeRoot()
 void cmSystemTools::MakefileColorEcho(int color, const char* message,
   bool newline, bool enabled)
 {
+  if(!enabled)
+{
+// Color is disabled.  Print without color.
+fprintf(stdout, %s%s, message, newline? \n : );
+return;
+}
+
   // On some platforms (an MSYS prompt) cmsysTerminal may not be able
   // to determine whether the stream is displayed on a tty.  In this
   // case it assumes no unless we tell it otherwise.  Since we want
   // color messages to be displayed for users we will assume yes.
   // However, we can test for some situations when the answer is most
-  // likely no.
-  int assumeTTY = cmsysTerminal_Color_AssumeTTY;
+  // likely no, and other cases where it should be forced to yes.
+  int flags = cmsysTerminal_Color_AssumeTTY;
+
   if(cmSystemTools::GetEnv(DART_TEST_FROM_DART) ||
  cmSystemTools::GetEnv(DASHBOARD_TEST_FROM_CTEST) ||
  cmSystemTools::GetEnv(CTEST_INTERACTIVE_DEBUG_MODE))
 {
 // Avoid printing color escapes during dashboard builds.
-assumeTTY = 0;
-}
-
-  if(enabled)
-{
-cmsysTerminal_cfprintf(color | assumeTTY, stdout, %s%s,
-   message, newline? \n : );
+flags = 0;
 }
   else
 {
-// Color is disabled.  Print without color.
-fprintf(stdout, %s%s, message, newline? \n : );
+// Newer versions of GNU make (4.0+) have the ability to separate the
+// output of parallel builds so they don't interfere with each other.
+// However this means commands invoked by make don't appear to have a TTY.
+// GNU make 4.1+ exports an environment variable MAKE_TERMOUT which is
+// non-empty if make thinks that it is printing _it's_ output to stdout.
+// If this variable exists and is non-empty, then force color output.
+const char* var = cmSystemTools::GetEnv(MAKE_TERMOUT);
+if(var  var[0] != '\0')
+  {
+  flags |= cmsysTerminal_Color_ForceTTY;
+  }
 }
+
+  cmsysTerminal_cfprintf(color | flags, stdout, %s%s,
+ message, newline? \n : );
 }
 #endif
 
diff --git a/Source/cmcmd.cxx b/Source/cmcmd.cxx
index a0c67e0..af9b139 100644
--- a/Source/cmcmd.cxx
+++ b/Source/cmcmd.cxx
@@ -907,6 +907,7 @@ int cmcmd::ExecuteEchoColor(std::vectorstd::string args)
 
   bool enabled = true;
   int color = cmsysTerminal_Color_Normal;
+  int force = 0;
   bool