implementation of 'shell' in octave-3.6.1/share/octave/3.6.1/m/pkg/pkg.m makes package installation problems undebuggable

[Sergei Steshenko]

Hello,

we have already discussed the issue, but, alas, it needs attention yet again.

There are a lot of complaints in this list about problems installing packages 
and, of course, people installing packages and having problems with that 
publish screen output.

Alas, the screen output often makes no sense.

It makes no sense for a very simple and good reason:

1) 'configure' and 'make' are run through a function called 'shell';
2) the 'shell' function implementation is broken by design.

In order to understand the "broken by design" statement one has to remember 
that modern OSes have _both_ stderr and stdout streams. Programs like 
'configure' and 'make' call other programs (e.g. compilers, linkers, etc) and 
this whole set of processes outputs to _both_ stderr and stdout - not 
simultaneously to both of them but some output goes to stdout (e.g. compiler 
invocation  string by 'make') and some output goes to stderr (e.g. compiler 
warning and error messages).

For a human to understand what is happening it's of utmost importance to see 
those lines in the order they are produced.

Alas, 'shell' is implemented this way:


   2270 function [status, output] = shell (cmd)
   2271   persistent have_sh;
   2272
   2273   cmd = strrep (cmd, "\\", "/");
   2274   if (ispc () && ! isunix ())
   2275     if (isempty(have_sh))
   2276       if (system("sh.exe -c \"exit\""))
   2277         have_sh = false;
   2278       else
   2279         have_sh = true;
   2280       endif
   2281     endif
   2282     if (have_sh)
   2283       [status, output] = system (cstrcat ("sh.exe -c \"", cmd, "\""));
   2284     else
   2285       error ("Can not find the command shell");
   2286     endif
   2287   else
   2288     [status, output] = system (cmd);
   2289   endif
   2290 endfunction


let's for simplicity consider just line #2288 - this is the line does the job 
on UNIXish systems.

If one checks what 'system' does, he/she can see that _only_ stdout is 
captured. So, when, say, 'make' is called in 'configure_make' function:


   1380     ## Make.
   1381     if (exist (fullfile (src, "Makefile"), "file"))
   1382       [status, output] = shell (cstrcat (scenv, "make -C '", src, "'"));
   1383       if (status != 0)
   1384         rm_rf (desc.dir);
   1385         error ("'make' returned the following error: %s", output);
   1386       elseif (verbose)
   1387         printf("%s", output);
   1388       endif
   1389     endif


, the following happens:

1) 'make' runs, and its stderr is _not_ being captured into 'output', so stderr 
output appears on the screen _immediately_;
2) when 'make' completes, its stdout is output to screen either by line #1385 
or #1387 (if at all).


So, error messages appear _before_ the commands producing them, and this makes 
debugging virtually impossible. Exactly because stdout is captured and stderr 
is not, the 'shell' function implementation is broken by design. And AFAIK this 
broken implementation has been in Octave for years.


Below one can find a set of functions I wrote. The set resolves the problem of 
stderr not being captured. The '__shell_with_grabbed_stdout_stderr__' function 
from the set should be used instead of 'shell'.

Regards,
  Sergei.


The functions:


##################################################################
#
# The following '__shell_with_grabbed_stdout_stderr__' is added by
# Sergei Steshenko - the purpose is to have orderly screen output
# produced by 'cmd'. The built-in 'system' grabs only stdout.
# In this file thi causes out of order output during 'configure'
# and 'make'.
#
# According to
# 
http://www.microsoft.com/resources/documentation/windows/xp/all/proddocs/en-us/redirection.mspx?mfr=true
# some_cmd >stdout.log 2>&1
# - I don't have Windows Octave to check that the whole thing
# works.
#
# The function is based on 'shell' function originally present in
# this file.
#
##################################################################

function [tmp_dir_for_stdout_stderr, shell_stdout_stderr_file] = 
__create_tmp_dir_for_stdout_stderr___()
  TMP = getenv("TMP");
  if(numel(TMP) ~= 0)
    if(exist(TMP, "dir") ~= 7)
      error("TMP=%s directory does not exist - either create it or unset TMP 
environment variable", TMP);
    endif

    tmp_dir_for_stdout_stderr = tmpnam(TMP, "oct-");
    last_dir_path_element = 
tmp_dir_for_stdout_stderr(rindex(tmp_dir_for_stdout_stderr, filesep) + 1:end); 
#fprintf(stderr, "__shell_with_grabbed_stdout_stderr__: 
last_dir_path_element=%s\n", last_dir_path_element);
  else
    tmp_dir_for_stdout_stderr = tmpnam;
  endif

  shell_stdout_stderr_file = cstrcat(tmp_dir_for_stdout_stderr, 
"/shell_stdout_stderr.log"); # fprintf(stderr, 
"__shell_with_grabbed_stdout_stderr__: shell_stdout_stderr_file=%s\n", 
shell_stdout_stderr_file);

  if(numel(TMP) == 0)
    [st, msg, msgid] = mkdir(tmp_dir_for_stdout_stderr);

    if(st == 0)
      error("could not create '%s' directory, system message: %s, msgid=%d", 
tmp_dir_for_stdout_stderr, msg, msgid);
    endif
  else
    [st, msg, msgid] = mkdir(TMP, last_dir_path_element);

    if(st == 0)
      error("could not create '%s' directory, system message: %s, msgid=%d", 
tmp_dir_for_stdout_stderr, msg, msgid);
    endif
  endif

  #fprintf(stderr, "__shell_with_grabbed_stdout_stderr__: 
shell_stdout_stderr_file=%s\n", shell_stdout_stderr_file);
endfunction

function [status, output] = __execute_command_in_shell__(shell_and_command)
  [tmp_dir_for_stdout_stderr, shell_stdout_stderr_file] = 
__create_tmp_dir_for_stdout_stderr___();
  [status, output] = system(cstrcat(shell_and_command, " >", 
shell_stdout_stderr_file, " 2>&1"));
  [fid, msg] = fopen(shell_stdout_stderr_file, "r");
  if(fid == -1)
    error("cannot open '$s' file for reading, system message: %s", 
shell_stdout_stderr_file, msg);
  endif

  output = char(fread(fid));

  fclose(fid);
  [err, msg] = unlink(shell_stdout_stderr_file)
  if(err)
    warning("__shell_with_grabbed_stdout_stderr__: could not remove '%s' 
file\n", shell_stdout_stderr_file);
  endif

  [st, msg, msgid] = rmdir(tmp_dir_for_stdout_stderr);
  if(st == 0)
    warning("__shell_with_grabbed_stdout_stderr__: could not remove '%' 
directory\n", tmp_dir_for_stdout_stderr);
  endif
endfunction

function [status, output] = __shell_with_grabbed_stdout_stderr__(cmd)
  persistent have_sh;

  cmd = strrep (cmd, "\\", "/");
  # Sergei - the above is essentially wrong. On a UNIXish system '\'
  # can appear as escape character, and in such a case it shouldn't
  # be replaced with '/'. But I just copy-pasted the line from
  # 'shell' function originally present in this file.

  if(ispc() && !isunix())
    if(isempty(have_sh))
      if(system("sh.exe -c \"exit\""))
        have_sh = false;
      else
        have_sh = true;
      endif
    endif

    if(have_sh)
      [status, output] = __execute_command_in_shell__(cstrcat("sh.exe -c \"", 
cmd, "\""));
    else
      error ("Can not find the command shell");
    endif
  else
    [status, output] = __execute_command_in_shell__(cmd);
  endif
endfunction