Issue 4664060: Adds redirect-lilypond-output option to lilypond-book

Rietveld

Please review the patch set to add the redirect option.
could you check the permissions or directory of
http://www.holmessoft.co.uk/homepage/private/lilypond/lilygood.lytex

I can't download any of the .lytex files.  I can get the .txt files just fine.
----- Original Message ----- 
From: <percival.music.ca@gmail.com>
To: <PhilEHolmes@googlemail.com>
Cc: <lilypond-devel@gnu.org>; <reply@codereview.appspotmail.com>
Sent: Sunday, July 03, 2011 2:30 PM
Subject: Re: Adds redirect-lilypond-output option to lilypond-book 
(issue4664060)

> could you check the permissions or directory of
> http://www.holmessoft.co.uk/homepage/private/lilypond/lilygood.lytex
>
> I can't download any of the .lytex files.  I can get the .txt files just
> fine.
>
> http://codereview.appspot.com/4664060/

Looks like my service provider doesn't allow those file types to be served. 
Easiest solution was to add .txt to their filenames, and all should now be 
visible.

--
Phil Holmes

LGTM, and fantastic website with testing info.

I have three requests for future work, but this patch should get pushed first.

- the final "Child returned 1" doesn't tell me (as a user/doc-writer, rather
than phd student) anything.  How hard would it be to put the "Look in
logfile..." line after that one?
(a very quick skim suggests that this would be icky, so maybe forget about it)
Or maybe just make the "child returned" into "Error: could not complete
command", or maybe "Error: child returned __insert_error_code_interpretation__"
?

- it's a bit weird to see tons of output even when using the
--redirect-lilypond-output.  Would it be possible to make a
  --use-logfiles
option which automatically turns on --redirect-lilypond-output, but also
captures stderr+stdlog and writes *those* to a file?
(and maybe in addition to writing the log file, it would display the "Error:
child returned __blah_blah__" line)

- it would be great to automatically show the last X lines of the logfile.  (but
that's definitely something to do later on)
----- Original Message ----- 
From: <percival.music.ca@gmail.com>
To: <PhilEHolmes@googlemail.com>; <email@philholmes.net>
Cc: <lilypond-devel@gnu.org>; <reply@codereview.appspotmail.com>
Sent: Sunday, July 03, 2011 7:25 PM
Subject: Re: Adds redirect-lilypond-output option to lilypond-book 
(issue4664060)

> LGTM, and fantastic website with testing info.
>
>
> I have three requests for future work, but this patch should get pushed
> first.
>
> - the final "Child returned 1" doesn't tell me (as a user/doc-writer,
> rather than phd student) anything.  How hard would it be to put the
> "Look in logfile..." line after that one?
> (a very quick skim suggests that this would be icky, so maybe forget
> about it)
> Or maybe just make the "child returned" into "Error: could not complete
> command", or maybe "Error: child returned
> __insert_error_code_interpretation__" ?

AFAICS this is a doddle to change.  I'll do this as a follow up piece of 
work unless you want it earlier.

> - it's a bit weird to see tons of output even when using the
> --redirect-lilypond-output.  Would it be possible to make a
>   --use-logfiles
> option which automatically turns on --redirect-lilypond-output, but also
> captures stderr+stdlog and writes *those* to a file?
> (and maybe in addition to writing the log file, it would display the
> "Error: child returned __blah_blah__" line)

Don't think so, at present.  From my reading of the Python, progress and 
error messages mostly go to stderr, as does the error message.  To make it 
so that we don't see progress but do see the error message would be 
something of a rewrite of how progress and errors are handled in lilylib and 
lilypond-book.  So, we need the GOP debate on stderr versus stdout.

> - it would be great to automatically show the last X lines of the
> logfile.  (but that's definitely something to do later on)

It needs to be a bit cleverer than that.  The last 5 lines are:

Converting to `/home/phil/TestFolder/BookTest/c4/lily-4139797f-2.pdf'...
Writing /home/phil/TestFolder/BookTest/c4/lily-4139797f-systems.texi...
Writing /home/phil/TestFolder/BookTest/c4/lily-4139797f-systems.tex...
Writing /home/phil/TestFolder/BookTest/c4/lily-4139797f-systems.count...
error: failed files: "c4/lily-4139797f.ly"

In this case, it needs line 32 out of 52.... (which says):

rest-note-collision.ly:9:0: error: unknown escaped string: `\PhilWasHere'

I'll have a think.

--
Phil Holmes

On Mon, Jul 04, 2011 at 04:23:06PM +0100, Phil Holmes wrote:
> ----- Original Message ----- From: <percival.music.ca@gmail.com>
> To: <PhilEHolmes@googlemail.com>; <email@philholmes.net>
> >Or maybe just make the "child returned" into "Error: could not complete
> >command", or maybe "Error: child returned
> >__insert_error_code_interpretation__" ?
> 
> AFAICS this is a doddle to change.  I'll do this as a follow up
> piece of work unless you want it earlier.

Later piece of work is fine.  I guess that now we'll wait for this
patch to get onto the countdown.

> >- it's a bit weird to see tons of output even when using the
> >--redirect-lilypond-output.  Would it be possible to make a
> >  --use-logfiles
> >option which automatically turns on --redirect-lilypond-output, but also
> >captures stderr+stdlog and writes *those* to a file?
> >(and maybe in addition to writing the log file, it would display the
> >"Error: child returned __blah_blah__" line)
> 
> Don't think so, at present.  From my reading of the Python, progress
> and error messages mostly go to stderr, as does the error message.
> To make it so that we don't see progress but do see the error
> message would be something of a rewrite of how progress and errors
> are handled in lilylib and lilypond-book.  So, we need the GOP
> debate on stderr versus stdout.

ok, that makes sense.

Cheers,
- Graham

in light of the growing consensus for "combined" logfiles for the build system
-- and given that lilypond only produces stuff on stderr and apparently this
isn't going to change -- I wonder if it might be better to redirect both stdout
and sterr to a single .log file.

I'm happy with pushing this patch as-is and changing this later, though.
----- Original Message ----- 
From: <percival.music.ca@gmail.com>
To: <PhilEHolmes@googlemail.com>; <email@philholmes.net>; 
<graham@percival-music.ca>
Cc: <lilypond-devel@gnu.org>; <reply@codereview.appspotmail.com>
Sent: Tuesday, July 12, 2011 3:39 PM
Subject: Re: Adds redirect-lilypond-output option to lilypond-book 
(issue4664060)

> in light of the growing consensus for "combined" logfiles for the build
> system -- and given that lilypond only produces stuff on stderr and
> apparently this isn't going to change -- I wonder if it might be better
> to redirect both stdout and sterr to a single .log file.
>
> I'm happy with pushing this patch as-is and changing this later, though.
>
> http://codereview.appspot.com/4664060/
>

I'd like to propose as a GOP topic that we _do_ change how LilyPond produces 
output.  Only using stderr is by no means common in the world of GNU-type 
programs.

--
Phil Holmes

On Tue, Jul 12, 2011 at 03:56:59PM +0100, Phil Holmes wrote:
> ----- Original Message ----- From: <percival.music.ca@gmail.com>
> >in light of the growing consensus for "combined" logfiles for the build
> >system -- and given that lilypond only produces stuff on stderr and
> >apparently this isn't going to change -- I wonder if it might be better
> >to redirect both stdout and sterr to a single .log file.
> 
> I'd like to propose as a GOP topic that we _do_ change how LilyPond
> produces output.  Only using stderr is by no means common in the
> world of GNU-type programs.

I'm totally willing to have such a discussion, but please make
sure that you have specific examples of GNU-type programs which
behave in this manner.  A few very experienced unix people
apparently think otherwise.

Fortunately, this is a simple empirical question; looking at half
a dozen GNU programs should settle it once and for all.  Since
you're the one working on the proposal, the onus is on you to find
examples which support your case.  (and to be fair in the ensuing
discussion, if you find examples which detract from your case, it
would be nice to include those too)

Cheers,
- Graham

Hey all,

During a recent sudo make install I got this error.  I'm not sure if it has to
do with recent pushes in the python code, but I figured I'd pass it along:

Compiling
/Users/mikesolomon/devel/lilypond/Documentation/out/notation/notation.texi...
/Users/mikesolomon/devel/lilypond/Documentation/out/notation/notation.texi is up
to date.
Processing include: notation/pitches.itely
Reading notation/pitches.itely...
Dissecting...
Writing snippets...
Processing...
Traceback (most recent call last):
  File "../scripts/lilypond-book.py", line 693, in <module>
    main ()
  File "../scripts/lilypond-book.py", line 675, in main
    chunks = do_file (files[0])
  File "../scripts/lilypond-book.py", line 585, in do_file
    chunks))
  File "../scripts/lilypond-book.py", line 581, in process_include
    return do_file (name, included=True)
  File "../scripts/lilypond-book.py", line 585, in do_file
    chunks))
  File "../scripts/lilypond-book.py", line 581, in process_include
    return do_file (name, included=True)
  File "../scripts/lilypond-book.py", line 569, in do_file
    do_process_cmd (chunks, input_fullname, global_options)
  File "../scripts/lilypond-book.py", line 437, in do_process_cmd
    options.formatter, options.lily_output_dir)
  File "../scripts/lilypond-book.py", line 390, in process_snippets
    logfile)
  File "../scripts/lilypond-book.py", line 367, in system_in_directory
    progress_p=1)
TypeError: subprocess_system() got an unexpected keyword argument
'redirect_output'
make[1]: *** [out/notation.texi] Error 1
make: *** [install] Error 2

Cheers,
MS

On Jul 3, 2011, at 2:02 PM, PhilEHolmes@googlemail.com wrote:

> Reviewers: Graham Percival,
> 
> Message:
> Please review the patch set to add the redirect option.
> 
> Description:
> Adds a new option to lilypond-book that causes the lilypond
> output to be directed to logfiles.  The option is:
> --redirect-lilypond-output.
> 
> This is a possible pre-cursor to further build work, but
> only possible.
> 
> Test cases and further information at:
> http://www.holmessoft.co.uk/homepage/private/lilypond/
> 
> Please review this at http://codereview.appspot.com/4664060/
> 
> Affected files:
>  M python/lilylib.py
>  M scripts/lilypond-book.py
> 
> 
> Index: python/lilylib.py
> diff --git a/python/lilylib.py b/python/lilylib.py
> index
dac53c16cf4dd5d83c2111e4ba49e28701f410b0..06bb5c361b835a78779c2bb1038c2b033cea99e2
100644
> --- a/python/lilylib.py
> +++ b/python/lilylib.py
> @@ -23,6 +23,7 @@ import re
> import shutil
> import sys
> import optparse
> +import time
> 
> ################################################################
> # Users of python modules should include this snippet
> @@ -118,6 +119,7 @@ def subprocess_system (cmd,
>                        ignore_error=False,
>                        progress_p=True,
>                        be_verbose=False,
> +                       redirect_output=False,
>                        log_file=None):
>     import subprocess
> 
> @@ -125,34 +127,52 @@ def subprocess_system (cmd,
>     name = command_name (cmd)
>     error_log_file = ''
> 
> -    if be_verbose:
> -	show_progress = 1
> -	progress (_ ("Invoking `%s\'") % cmd)
> +    if redirect_output:
> +        progress (_ ("Processing %s.ly") % log_file)
> +        progress ('\n')
>     else:
> -	progress ( _("Running %s...") % name)
> -
> +        if be_verbose:
> +            show_progress = 1
> +            progress (_ ("Invoking `%s\'") % cmd)
> +        else:
> +            progress ( _("Running %s...") % name)
> 
>     stdout_setting = None
> +    stderr_setting = None
>     if not show_progress:
> -	stdout_setting = subprocess.PIPE
> +        stdout_setting = subprocess.PIPE
> +
> +    if redirect_output:
> +        stdout_filename = ''.join([log_file, '.log'])
> +        stderr_filename = ''.join([log_file, '.err.log'])
> +        stdout_setting = open(stdout_filename, 'w')
> +        stderr_setting = open(stderr_filename, 'w')
> 
>     proc = subprocess.Popen (cmd,
>                              shell=True,
>                              universal_newlines=True,
>                              stdout=stdout_setting,
> -                             stderr=stdout_setting)
> +                             stderr=stderr_setting)
> 
>     log = ''
> 
> -    if show_progress:
> -	retval = proc.wait()
> +    if redirect_output:
> +        while proc.poll()==None:
> +            time.sleep(1)
> +        retval = proc.returncode
> +        stdout_setting.close()
> +        stderr_setting.close()
>     else:
> -	log = proc.communicate ()
> -	retval = proc.returncode
> -
> +        if show_progress:
> +            retval = proc.wait()
> +        else:
> +            log = proc.communicate ()
> +            retval = proc.returncode
> 
>     if retval:
>         print >>sys.stderr, 'command failed:', cmd
> +        if redirect_output:
> +            print >>sys.stderr, "Look in logfile", stderr_filename
>         if retval < 0:
>             print >>sys.stderr, "Child was terminated by signal", -retval
>         elif retval > 0:
> Index: scripts/lilypond-book.py
> diff --git a/scripts/lilypond-book.py b/scripts/lilypond-book.py
> index
9617f3666d3a4c878325a089b3d898def9771a8d..cb3ff4de3269861c09f0e5d83bb46a2bbdd39ca4
100644
> --- a/scripts/lilypond-book.py
> +++ b/scripts/lilypond-book.py
> @@ -179,6 +179,11 @@ def get_option_parser ():
>                   action='store',
>                   dest='process_cmd', default='')
> 
> +    p.add_option ('--redirect-lilypond-output',
> +                  help = _ ("Redirect the lilypond output"),
> +                  action='store_true',
> +                  dest='redirect_output', default=False)
> +
>     p.add_option ('-s', '--safe', help=_ ("Compile snippets in safe mode"),
>                   action="store_true",
>                   default=False,
> @@ -347,7 +352,7 @@ def find_toplevel_snippets (input_string, formatter):
> 
>     return snippets
> 
> -def system_in_directory (cmd, directory):
> +def system_in_directory (cmd, directory, logfile):
>     """Execute a command in a different directory.
> 
>     Because of win32 compatibility, we can't simply use subprocess.
> @@ -355,7 +360,10 @@ def system_in_directory (cmd, directory):
> 
>     current = os.getcwd()
>     os.chdir (directory)
> -    ly.system(cmd, be_verbose=global_options.verbose,
> +    ly.system(cmd,
> +              be_verbose=global_options.verbose,
> +              redirect_output=global_options.redirect_output,
> +              log_file=logfile,
>               progress_p=1)
>     os.chdir (current)
> 
> @@ -374,11 +382,12 @@ def process_snippets (cmd, snippets,
>                           + list (set ([snip.basename() + '.ly' for snip in
snippets])))
>     name = os.path.join (lily_output_dir,
>                          'snippet-names-%d.ly' % checksum)
> +    logfile = name.replace('.ly', '')
>     file (name, 'wb').write (contents)
> 
>     system_in_directory (' '.join ([cmd, ly.mkarg (name)]),
> -                         lily_output_dir)
> -
> +                         lily_output_dir,
> +                         logfile)
> 
> def snippet_list_checksum (snippets):
>     return hash (' '.join([l.basename() for l in snippets]))
> 
> 
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel

----- Original Message ----- 
From: <mike@apollinemike.com>
To: <PhilEHolmes@googlemail.com>; <percival.music.ca@gmail.com>; 
<lilypond-devel@gnu.org>; <reply@codereview.appspotmail.com>
Sent: Sunday, July 17, 2011 3:43 PM
Subject: Re: Adds redirect-lilypond-output option to 
lilypond-book(issue4664060)

> Hey all,
>
> During a recent sudo make install I got this error.  I'm not sure if it 
> has to do with recent pushes in the python code, but I figured I'd pass it 
> along:

[snip]

Mike - is this only with make install?  (i.e. have you tried make, or make 
doc, or anything else like that?  I'm not suggesting you do, just gathering 
information).

--
Phil Holmes

----- Original Message ----- 
From: "Phil Holmes" <mail@philholmes.net>
To: <mike@apollinemike.com>; <PhilEHolmes@googlemail.com>; 
<percival.music.ca@gmail.com>; <lilypond-devel@gnu.org>; 
<reply@codereview.appspotmail.com>
Sent: Sunday, July 17, 2011 3:59 PM
Subject: Re: Adds redirect-lilypond-output option to 
lilypond-book(issue4664060)

> ----- Original Message ----- 
> From: <mike@apollinemike.com>
> To: <PhilEHolmes@googlemail.com>; <percival.music.ca@gmail.com>; 
> <lilypond-devel@gnu.org>; <reply@codereview.appspotmail.com>
> Sent: Sunday, July 17, 2011 3:43 PM
> Subject: Re: Adds redirect-lilypond-output option to 
> lilypond-book(issue4664060)
>
>
>> Hey all,
>>
>> During a recent sudo make install I got this error.  I'm not sure if it 
>> has to do with recent pushes in the python code, but I figured I'd pass 
>> it along:
>
> [snip]
>
> Mike - is this only with make install?  (i.e. have you tried make, or make 
> doc, or anything else like that?  I'm not suggesting you do, just 
> gathering information).
>
> --
> Phil Holmes

Mike - I'm thinking that lilylib.py and lilypond-book might have got out of 
step in your build directory.  Does build/python/out/lilylib.py contain the 
lines:

def subprocess_system (cmd,
                       ignore_error=False,
                       progress_p=True,
                       be_verbose=False,
                       redirect_output=False,
                       log_file=None):

?

--
Phil Holmes

On Jul 17, 2011, at 4:59 PM, Phil Holmes wrote:

> ----- Original Message ----- From: <mike@apollinemike.com>
> To: <PhilEHolmes@googlemail.com>; <percival.music.ca@gmail.com>;
<lilypond-devel@gnu.org>; <reply@codereview.appspotmail.com>
> Sent: Sunday, July 17, 2011 3:43 PM
> Subject: Re: Adds redirect-lilypond-output option to
lilypond-book(issue4664060)
> 
> 
>> Hey all,
>> 
>> During a recent sudo make install I got this error.  I'm not sure if it has
to do with recent pushes in the python code, but I figured I'd pass it along:
> 
> [snip]
> 
> Mike - is this only with make install?  (i.e. have you tried make, or make
doc, or anything else like that?  I'm not suggesting you do, just gathering
information).

This is only with make install and only after the version change - I think it
may have something to do with where lilypond-book gets installed on my machine. 
I'll do my best to track it down on my machine in a couple days.

Cheers,
MS
On Sun, Jul 17, 2011 at 03:59:37PM +0100, Phil Holmes wrote:
> ----- Original Message ----- From: <mike@apollinemike.com>
> >During a recent sudo make install I got this error.  I'm not sure
> >if it has to do with recent pushes in the python code, but I
> >figured I'd pass it along:
> 
> Mike - is this only with make install?  (i.e. have you tried make,
> or make doc, or anything else like that?  I'm not suggesting you do,
> just gathering information).

Also - is this with a build completely from scratch?  That's the
first thing to try.

Cheers,
- Graham

----- Original Message ----- 
From: <percival.music.ca@gmail.com>
To: <PhilEHolmes@googlemail.com>; <email@philholmes.net>; 
<graham@percival-music.ca>
Cc: <reply@codereview.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Tuesday, July 12, 2011 3:39 PM
Subject: Re: Adds redirect-lilypond-output option to 
lilypond-book(issue4664060)

> in light of the growing consensus for "combined" logfiles for the build
> system -- and given that lilypond only produces stuff on stderr and
> apparently this isn't going to change -- I wonder if it might be better
> to redirect both stdout and sterr to a single .log file.
>
> I'm happy with pushing this patch as-is and changing this later, though.
>
> http://codereview.appspot.com/4664060/

This was pushed as 23cdda9506931d5b9a1e75ee8be8b74f9084a7c0

--
Phil Holmes

Issue 4664060: Adds redirect-lilypond-output option to lilypond-book (Closed)

Description

Patch Set 1 #

Messages