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

Is it possible to leave stderr messages from the compiler on stderr and not redirect them to stdout? #1537

Open
tpimh opened this issue Mar 12, 2019 · 87 comments

Comments

@tpimh
Copy link

tpimh commented Mar 12, 2019

I understand that default ninja behavior is to only print to stdout, but I wonder if it's possible to implement this as an option like done in this branch: https://github.com/dendy/ninja/tree/stderr

@tpimh tpimh changed the title Is it possible to leave stderr messages from build tool on stderr and not redirect them to stdout? Is it possible to leave stderr messages from the compiler on stderr and not redirect them to stdout? Mar 12, 2019
@jhasse
Copy link
Collaborator

jhasse commented Mar 20, 2019

What do you need it for?

@tpimh
Copy link
Author

tpimh commented Mar 20, 2019

I would like to separate build output into too files: build.log for informational output and build.err for warnings and errors. Redirecting stdout to one file and stderr to another works for GNU Make, but not for Ninja.

@jhasse
Copy link
Collaborator

jhasse commented Mar 20, 2019

You won't know which warning or error belongs to which build output line. What's the reason behind the two files?

@tpimh
Copy link
Author

tpimh commented Mar 20, 2019

Mostly just warning detection. If there are warnings, they should be either fixed or disabled. The warning usually contains source filename and line number, so there is no need to find corresponding lines of ninja's own output.

@jhasse
Copy link
Collaborator

jhasse commented Mar 20, 2019

I see. Would #480 be enough for your use-case?

The reason I'm asking is: We're trying to keep the number of options as low as possible.

@tpimh
Copy link
Author

tpimh commented Mar 20, 2019

I'm not sure how that could possibly be the solution for my problem. Probably there may be a very hacky solution with NINJA_STATUS variable, but I can't think of one right now.
On the other hand if split stderr is implemented, --quiet that disables all ninja's own output and only prints compiler output would be as easy as ninja --split-stderr >/dev/null.

@jhasse
Copy link
Collaborator

jhasse commented Mar 20, 2019

I'm not sure how that could possibly be the solution for my problem.

You would run ninja --quiet and could detect all the warnings. (I'm aware that --quiet isn't implemented yet)

@tpimh
Copy link
Author

tpimh commented Mar 20, 2019

But in this case I will not get any ninja's own output. I would like to keep both, but split them.

@jhasse
Copy link
Collaborator

jhasse commented Mar 20, 2019

Why?

@tpimh
Copy link
Author

tpimh commented Mar 20, 2019

In current build script I print a dot for every line on stdout and an x for every line on stderr. Saving both stdout and stderr into separate files. This keeps the CI from stopping because of the lack of output, gives a pretty visual output that helps to detect new warnings and errors and provides separate readable build logs with warning descriptions. This works for other build systems, but not for ninja as there is no way to tell if a line came from stderr or stdout. This is how I did it: tpimh/nenuzhnix-build@722ece3 (still to be rewritten properly)

@jhasse
Copy link
Collaborator

jhasse commented Mar 20, 2019

Set NINJA_STATUS="[ninja][%f/%t] " and then you grep for "^[ninja]" in process_out to see if it's ninja output or compiler output.

@tpimh
Copy link
Author

tpimh commented Mar 20, 2019

Yes, can be done this way. Thanks!

@tpimh
Copy link
Author

tpimh commented Jul 19, 2019

This is my script for restoring justice over stdout and stderr:

filter() {
  local l n
  while read -r l; do
    n=${l#\[ninja\]}
    [ "x$l" != "x$n" ] && printf "%s\n" "$n" || printf "%s\n" "$l" >&2
  done
}

NINJA_STATUS="[ninja][%f/%t] " ninja | filter

It may be a bit slow, but it works for me. Feel free to use and improve.

UPD: replaced my obviously shitty code with a much better version by Vladimir Oleynik [email protected]

@sjg20
Copy link

sjg20 commented Oct 30, 2019

For me the way that ninja ignores sfderr causes 'kate' to ignore all warnings/errors, which means that the build plugin doesn't work.

@subdiff
Copy link

subdiff commented Mar 14, 2020

Same for QtCreator.

Just a simple script to wrap ninja with stdout > stderr is enough. But it would be nice if it would work out of the box.

@kloczek
Copy link

kloczek commented Mar 14, 2020

Can you share what and how you are actually using as the wrpaper?

@tpimh
Copy link
Author

tpimh commented Mar 15, 2020

I have my own ninja wrapper. It's really bad, but it works. Written in POSIX shell, could be much easier in bash, but it's unfortunately not an option for me.

@subdiff
Copy link

subdiff commented May 2, 2020

$ cat /usr/bin/ninja-stderr

#!/bin/sh
/usr/bin/ninja "$@" 1>&2

@sjg20
Copy link

sjg20 commented May 6, 2020

Is there any chance this might be 'fixed' in ninja itself? If ninja considers all of its output to be errors, then perhaps it should just move to stderr. If it considers some of it to be informational, not indicating warnings/failures, then that part should go to stdout IMO.

@jhasse
Copy link
Collaborator

jhasse commented Jul 20, 2020

@sjg20 #1210 would allow this to be implemented in a cleaner way.

@dendy
Copy link

dendy commented Oct 26, 2020

Hello, I am the author of the patch for this issue mentioned in the first comment: https://github.com/dendy/ninja/tree/stderr

Patch is currently updated with Linux/Mac/Windows support and rebased onto master branch.

I consider this issue as critical. Without separation of the stdout and stderr usage of the ninja is limited mostly to the CLI. IDEs and tools which expect the proper separation of output, like it was in the Makefiles, will not work with ninja at all.

Here is the thing. Decision where to put an output should be made by the executing command, not the ninja itself. If some command decides to put portion of output to the stderr, that should be possible and properly pop up at the ninja's end in the stderr. Generally speaking stdout and stderr supposed to be completely independent, and it is up to the command itself to ensure that both streams are contiguous. And whoever is parsing the output should expect that stdout and stderr are not connected together.

Now, I understand why ninja is combining both outputs, to keep them in sync with printing the execution command, so it is easier to see whole output in the solid chunk. Although such approach has it is own benefits in some cases, it is not the only one that is used in the build environments.

Particularly using ninja from IDE requires to parse stderr. Not combined stdout and stderr, but stderr alone. Each line printed by the executing command into stderr is considered as a separate issue. The executing command arguments usually don't matter in the IDE context, and even if they do there are other ways to connect errors and command line, rather than eliminating the stderr completely. In worst case it will be the same experience as with Makefiles.

I strongly believe that this feature is absolutely necessary. It should be up to the user, which method is preferable in particular case. It could be environment variable, command line argument, or some extra parameter in the build.ninja, or combination of those. But user should have control over this, whether output of the stdout and stderr should be merged together or not.

For the 5 good years I never use combined output in any of my projects, either they are built from the command line or from IDE. And I am totally happy with the results.

@sjg20
Copy link

sjg20 commented May 10, 2021

+1 we have a hundred lines of Python code trying to clean up the ninja output and it is still not right. It messes up downstream things also - e.g. Zephyr is quite broken in terms of what is a warning/error and what is normal behaviour.

I wonder if the problem is that this tool came from Windows?

Do you have a link to the stdout/stderr patch? We could perhaps just use that.

@dendy
Copy link

dendy commented May 10, 2021

@sjg20 You are totally right, this issue should be fixed at the Ninja level. Generally there is no way to process Ninja output in attempt to differentiate between stdout/stderr after outputs were merged.

I believe the problem came out because of misunderstanding what outputs really are, and how contract between process output writers are readers should be. Probably in an attempt to solve the messed output because of the single pipe shared across multiple processes, this principle was explicitly highlighted in the Ninja documentation. But it has nothing to do with this issue.

Link to the patch is in the initial comment to this ticket, let me know it works for you.

@sjg20
Copy link

sjg20 commented May 10, 2021

@dendy OK thank you. I filed https://issuetracker.google.com/187728036 so we'll see what we end up doing. Someone also suggesting switching Cmake to generate plain Makefiles.

@CarloWood
Copy link

This is a critical problem for me too - since @jhasse seems to refuse to add a simple option for it - where is the ninja fork that DOES implement this currently? Thanks.

@dendy
Copy link

dendy commented Oct 4, 2022

@orgads
Copy link
Contributor

orgads commented Jan 15, 2023

@jhasse My proposal is to enable this feature in experimental mode. I am quite happy with the current patch (see link at the top).

With some caveats:

  1. Default behavior should stay as in current implementation, to combine stdout and stderr.
  2. Add environment variable to override the default behavior, e.g.: NINJA_USE_STDERR=1. This is needed in case it is convenient for the user to set behavior globally.
  3. Add command line argument to also override the default behavior, which will have more priority than environment variable, e.g.: ninja --use-stderr=1. For instance, this is needed to use this feature in the IDE, without changing behavior globally.
  4. (optionally) Add new binding, e.g. "use_stderr". This will allow to enable feature per individual command. Could be useful for generators.

Advertise this update in the new ninja version and collect feedback from the users about issues.

@jhasse can you please explain why do you object to having this as opt-in, as suggested here? This is a real pain to many users.

@jhasse
Copy link
Collaborator

jhasse commented Jan 15, 2023

I'm no longer maintaining Ninja.

@jhasse jhasse closed this as completed Jan 15, 2023
@jhasse
Copy link
Collaborator

jhasse commented Jan 15, 2023

misclicked, sorry

@jhasse jhasse reopened this Jan 15, 2023
@orgads
Copy link
Contributor

orgads commented Jan 16, 2023

I'm no longer maintaining Ninja.

Then who can approve this?

@kloczek
Copy link

kloczek commented Aug 29, 2023

Any update? 🤔

@dendy
Copy link

dendy commented Aug 29, 2023

@jhasse Do you know who is the current maintainer of the ninja? We are still looking to accept this feature as optional, without changing the default behavior. Who could help to proceed with this?

@jhasse
Copy link
Collaborator

jhasse commented Aug 29, 2023

No one really.

Ninja cannot dictate policy to the executing commands how they should use stdout/stderr, nor to the caller how they should process output from the commands.

Well, it can at least discourage the use of stderr by not providing an option to split stdout/stderr.

@kloczek
Copy link

kloczek commented Aug 29, 2023

Combining stdout with stderr by default and lack of ability to not combine those two streams is really big problem on automatically analysing warnings and errors 😞
IMO current behaviour should stay as default but it should be provided command line option to keep separated stdout and stderr.

@sjg20
Copy link

sjg20 commented Aug 29, 2023

+1 we need a proper stderr. This is such a pain. Why is this so controversial??

@dendy
Copy link

dendy commented Aug 29, 2023

@jhasse Ninja is only executing other tools, and behavior of those tools how to produce data for stdout and stderr already well defined. There are billions of those tools, Ninja may not change they behavior.

In the case when parser of the Ninja output wants proper separation of stderr — there should be such an option.

My proposal above is to add this feature. And keep original behavior (combined output) as default. So everybody will win and be happy.

Have to rebase my patch onto recent master branch. Though first I will need your agreement in general to accept this feature.

@kloczek
Copy link

kloczek commented Feb 16, 2024

Gentle ping .. any update? 🤔

@tpimh
Copy link
Author

tpimh commented Feb 16, 2024

@kloczek just use Daniel's fork, @jhasse is never going to merge this.

@kloczek
Copy link

kloczek commented Feb 16, 2024

@kloczek just use Daniel's fork, @jhasse is never going to merge this.

OK so where is that fork and/or where I can find exact patch/commit? 😋

@kloczek
Copy link

kloczek commented Feb 16, 2024

https://github.com/jhasse/ninja seems is no longer maintained 🤔

@dendy
Copy link

dendy commented Feb 16, 2024

@kloczek Look at the veeeery top message, you will find a link to my branch: https://github.com/dendy/ninja/tree/stderr
I will check if it needs to be updated by rebasing it onto the latest ninja mainline.

@kloczek
Copy link

kloczek commented Feb 21, 2024

@kloczek Look at the veeeery top message, you will find a link to my branch: https://github.com/dendy/ninja/tree/stderr I will check if it needs to be updated by rebasing it onto the latest ninja mainline.

Can you resync your fork with ninja master? 🤔
Currently that commit is not possible to apply on top of last version

+ /usr/lib/rpm/rpmuncompress /home/tkloczko/rpmbuild/SOURCES/ninja-build-Add-option-to-split-stdout-and-stderr.patch
6 out of 8 hunks FAILED -- saving rejects to file src/build.cc.rej
2 out of 3 hunks FAILED -- saving rejects to file src/build.h.rej
1 out of 11 hunks FAILED -- saving rejects to file src/subprocess-posix.cc.rej

@jhasse jhasse added this to the 2.0.0 milestone Apr 14, 2024
@Ext3h
Copy link

Ext3h commented Jul 29, 2024

Not really happy with the approach of passing through stdout / stderr either. It's an option - sure - but it's not even close to actually fixing the issue at hand, only introducing a new bug. It's doing the same stuff wrong as makes --line-buffer does.

The problem we have, is that called applications end up assuming that full buffering was permissible, even when it clearly wasn't.

FWIW ninja is corrupting the output from Doxygen (via CMake's doxygen_add_docs()). The issue doesn't happen when using GNU Make (or Doxygen directly).

E.g. in this case, the patch with split stderr only looks like it does something sensible at first glance, because when forwarding stderr the terminal has enforced line buffering for ninjas output buffers, but the output from the original application still had a fully buffered stdout due to being attached to a pipe rather than a pty.

That's not working properly at all with applications that split time correlated output onto different output streams, such as Doxygen which is splitting progress and warning messages. You still get warning messages completely out-of-sync with the progress messages they should have been embedded in between.

Doxygen actually has a -b command line argument to force un-buffered output in order to work correctly when embedded into other applications.

Actually I am back to using ninja. By prefixing my commands with stdbuf -oL -eL (from GNU coreutils) everything works as it should.

That did do something sensible. Partially. The hack of preloading libstdbuf unfortunately isn't exactly portable.

In case of Doxygen, the actual sane solution is to specifiy WARN_LOGFILE = - or alike - telling Doxygen that it should stop splitting messages between stdout and stderr. (See https://gitlab.kitware.com/cmake/cmake/-/issues/21692 - it's not working with CMake either, unfortunately.)

@CarloWood
Copy link

I just wrote this at the time and never looked back:

#include <signal.h>
#include <stdlib.h>
#include <sysexits.h>
#include <unistd.h>
#include <stdio.h>
#include <pty.h>
#include <string.h>
#include <errno.h>
#include <ctype.h>

pid_t child = 0;

void sighandler(int signum)
{
  if (child > 0)
  {
    killpg(child, signum);
    exit(signum);
  }
}

int last_is_esc_K = 0;

char const* find_eom(char const* start, char const* end)
{
  for (char const* ptr = start; ptr < end; ++ptr)
  {
    if (*ptr == '\033')
    {
      if (ptr < end - 2 && ptr[1] == '[' && ptr[2] == 'K')
      {
        last_is_esc_K = 1;
        return ptr + 3;
      }
    }
    if (*ptr == '\n')
    {
      last_is_esc_K = 0;
      return ptr + 1;
    }
  }
  return NULL;
}

int errors = 0;

void handle_msg(char const* start, size_t len)
{
  char const* p = start;
  char const* e = start + len;
  int print_stdout = 0;
  if (len >= 6)
  {
    if (strncmp(start, "ninja:", 6) == 0)
      print_stdout = 1;
    else if (len >= 17 && strncmp(start, "\e[31mFAILED: \e[0m", 17) == 0)
      ++errors;
    else
    {
      // \r[12/42]
      //  ^  ^
      //  |  |
      //  p  +3
      if (p[0] == '\r' && p[1] == '[' && isdigit(p[2]))
      {
        p += 3;
        while (p < e && isdigit(*p))
          ++p;
        // \r[12/42]
        //      ^ ^
        //      | |
        //      p +2
        if (p + 2 < e)
        {
          if (p[0] == '/' && isdigit(p[1]))
          {
            p += 2;
            while (p < e && isdigit(*p))
              ++p;
            // \r[12/42]
            //         ^
            //         |
            //         p
            if (p < e && *p == ']')
              print_stdout = 1;
          }
        }
      }
    }
  }
  else
  {
    while (!print_stdout && p < e && (*p == '\r' || *p == '\n'))
    {
      if (*p == '\n')
        print_stdout = 1;
      ++p;
    }
  }
  if (print_stdout)
  {
    fflush(stderr);
    if (write(STDOUT_FILENO, start, len) != len)
    {
      perror("failed to write to stdout");
      exit(EX_OSERR);
    }
  }
  else if (errors <= 1)
  {
    fflush(stdout);
    if (write(STDERR_FILENO, start, len) != len)
    {
      perror("failed to write to stderr");
      exit(EX_OSERR);
    }
  }
#if 0
  else
  {
    fprintf(stderr, "Suppressed:\n");
    fflush(stderr);
    if (write(STDERR_FILENO, start, len) != len)
    {
      perror("failed to write to stderr");
      exit(EX_OSERR);
    }
  }
#endif
}

// Run a command in a pty.
// Usage: /path/to/this/binary command to run.
int main(int argc, char *argv[])
{
  if (argc < 2)
    return EX_USAGE;

  int master;
  child = forkpty(&master, NULL, NULL, NULL);

  if (child == -1)
  {
    perror("failed to fork pty");
    return EX_OSERR;
  }

  if (child == 0)
  {
    // We're in the child process, so replace it with the command.
    execvp(argv[1], argv + 1);
    perror("failed to execute command");
    return EX_OSERR;
  }

  // Trap kill signals and forward them to child process.
  signal(SIGHUP, sighandler);
  signal(SIGINT, sighandler);
  signal(SIGTERM, sighandler);

  int const buf_size = 1024;
  char buf[buf_size + 1];
  buf[buf_size] = 0;
  char record[buf_size];
  fd_set fds;
  char* pptr = buf;                             // Begin writing at the start of the buffer.
  char const* const epptr = buf + buf_size;     // One past the end of the buffer.
  char const* egptr = pptr;                     // The buffer is empty.
  char const* gptr = egptr;                     // There is nothing to get.

  // Forward the output continuously.
  while (1)
  {
    FD_ZERO(&fds);
    FD_SET(master, &fds);

    if (select(master + 1, &fds, NULL, NULL, NULL) > 0 && FD_ISSET(master, &fds))
    {
      ssize_t bytes_read = read(master, pptr, epptr - pptr);
      if (bytes_read == 0 || (bytes_read == -1 && errno == EIO))
      {
        if (last_is_esc_K)
          printf("\r\n");
        return EXIT_SUCCESS;
      }
      if (bytes_read < 0)
      {
        perror("error reading input");
        return EX_OSERR;
      }

      pptr += bytes_read;
      egptr += bytes_read;

      // Parse the input.
      //                                                           pptr  epptr
      //                                                            |      |
      //                                                            v      v
      // [<old message><new message1><new message2><partial message><junk>]
      // ^             ^                                            ^
      // |             |                                            |
      //buf           gptr                                        egptr
      for (;;)
      {
        char const* eom = find_eom(gptr, egptr);
        if (eom)
        {
          handle_msg(gptr, eom - gptr);
          gptr = eom;
          continue;
        }
        else if (pptr == epptr)
        {
          eom = egptr;
          handle_msg(gptr, egptr - gptr);
          gptr = egptr;
        }
        break;
      }
      if (gptr != buf)
      {
        size_t size = egptr - gptr;
        size_t shift = gptr - buf;
        if (shift > 0 && size > 0)
          memmove(buf, gptr, size);
        gptr = buf;
        egptr = gptr + size;
        pptr -= shift;
      }
    }
  }
}

This reads from stdin, and fakes being a tty - so that the program that is passed doesn't buffer.
It then splits the input, on a line by line basis, to stdout or stderr, preserving the order by flushing whenever it switches
between the two. The decision on whether to write stdout or stderr is made based on the content
of the line.

I'm using it like this:

                    if [[ $CMAKE_CONFIGURE_OPTIONS == *"-GNinja"* ]]; then
                        /usr/local/bin/ninja-filter cmake --build "$BUILDDIR" --config "$CMAKE_CONFIG" --parallel $CPUS -- $@ 2> >(awk 'BEGIN { have_match = 0 } /operator<</ { if (have_match == 2) { have_match = 1; next }} /^\/usr\/.*(no known conversion from|candidate template ignored: |template argument deduction\/substitution failed:)/ { have_match = 2; keep = $0; next } { if (have_match == 2) { print keep; have_match = 0 } if (have_match == 0) print; else { have_match = 0 }}' | sed -e 's%'"$REPOBASE"'/%%;s/symbolic:://g' 1>&2);
                    else
                        cmake --build "$BUILDDIR" --config "$CMAKE_CONFIG" --parallel $CPUS -- $@;
                    fi;

from a bash script.

@dendy
Copy link

dendy commented Aug 26, 2024

@kloczek Apologies for the late response, just rebased stderr branch onto recent master. Please check it out.

@macdew
Copy link

macdew commented Nov 26, 2024

I'd like to add a "me too" here as well, the merging of the output streams completely destroys our handling of the output of the applications that ninja is calling. We've now switched back to Make due to this. I'm very sad to read as well that Ninja is no longer being maintained, it was a good build engine for a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests