5824 cleanup of output-distance

commit 37c6ba676f4722a4e5a7b9965b64d46eff5e0bb1 (output-distance-test)
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Mar 6 16:47:26 2020 +0100

    Fix test target name in python/GNUmakefile

commit 79a1f4de2ac6035bbedf0e49841581d21533d3b7
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Mar 6 16:32:12 2020 +0100

    scripts/build: run self-test as part of 'check' suite.

commit 613d8478797784ecc9e7676639375e7f18d4abf1
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Mar 6 16:30:23 2020 +0100

    output-distance: avoid calling strip() on None

commit 80079c73090bdd7608567c76508bc1c9f1c83d33
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Mar 6 11:54:12 2020 +0100

    output-distance: make self test pass again:
    
    * Remove --create-images option. This is always true.
    
    * Remove --no-compare-images option. We always compare.
    
    * Drop dynamic table headings. before/after also works
    
    * Use same options as regtest in self-test.
    
    * Check that .pngs/.jpgs are created.

commit 973677710c4f7b4551a0283f1084dfa68c4357d0
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Mar 6 11:53:53 2020 +0100

    output-distance: remove support for stencil expression checks
    
    This must have been broken since commit c1a4e44 "don't do expression
    comparison." (Jun 2007).
    
    This also caused a self-test failure in output-distance, which means
    it hasn't been run for 13 years.
    
    Adapt for expression/origin removal.  Make test_compare_signatures pass,

commit 1918da8bd677dd7ec2efd4bc1280002d20a65fce
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Mar 6 10:40:27 2020 +0100

    output-distance: cleanups
    
    * Centralize imports at top
    
    * Use doc strings rather than comments
    
    * Add more comments/docstrings
    
    * Remove unused GrobSignature.origin
    
    * Clarify method names:
    
    * snippet_fn_re as TextFileCompareLink instance variable
    
    * use time.time() rather than time.clock()
    
    * use log_verbose() rather than print()
    
    * set the actual centroid in GrobSignature.
    
    * remove unused axis_centroid() method

[hanwenn, 2020-03-06 16:13:29]

output-distance refactor

[Dan Eble, 2020-03-07 17:34:03]

The title of the review is "5824 Fix test target name in python/GNUmakefile" but
the title of ticket 5824 is "output-distance refactor".  It's hard to tell what
you want us to review.  Everything?  Just the makefiles?  Just output-distance? 
Please update the title and description in both places to reflect the scope of
the changes more accurately.

Also, "refactor" is not the word to use when there are significant functional
changes.

[hanwenn, 2020-03-07 17:49:55]

On 2020/03/07 17:34:03, Dan Eble wrote:
> The title of the review is "5824 Fix test target name in python/GNUmakefile"
but
> the title of ticket 5824 is "output-distance refactor".  It's hard to tell
what
> you want us to review.  Everything?  Just the makefiles?  Just
output-distance? 
> Please update the title and description in both places to reflect the scope of
> the changes more accurately.
> 
> Also, "refactor" is not the word to use when there are significant functional
> changes.

it's a sequence of 6 commmits, which you can inspect here: 

  https://github.com/hanwen/lilypond/pull/2

the issue description here is a concatenation of all the commit messages, in
reverse chronological order.

I intend to push them as separate commits, but would prefer the whole thing to
take less than 6 weeks to push. How would you like to review them?

The ticket title is "output-distance refactor", because at some point "git-cl"
wanted to have a description for the patchset I was pushing. I think this is
because I had ^C'd a previous run, and git-cl isn't properly atomic.

[hanwenn, 2020-03-07 18:32:21]

make doc fix

[Dan Eble, 2020-03-07 18:40:57]

On 2020/03/07 17:49:55, hanwenn wrote:
> it's a sequence of 6 commmits, which you can inspect here: 
> 
>   https://github.com/hanwen/lilypond/pull/2 

Don't you think these two should be combined?  I don't see value in leaving the
first commit with an incorrect target name.

commit 37c6ba676f4722a4e5a7b9965b64d46eff5e0bb1
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Mar 6 16:47:26 2020 +0100

    Fix test target name in python/GNUmakefile

commit 79a1f4de2ac6035bbedf0e49841581d21533d3b7
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Mar 6 16:32:12 2020 +0100

    scripts/build: run self-test as part of 'check' suite.

[Dan Eble, 2020-03-07 18:47:00]

On 2020/03/07 17:49:55, hanwenn wrote:
> it's a sequence of 6 commmits, which you can inspect here: 
> 
>   https://github.com/hanwen/lilypond/pull/2

The fact that the HTML table now includes rows for removed files is worth
mentioning in this change.

commit 613d8478797784ecc9e7676639375e7f18d4abf1
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Mar 6 16:30:23 2020 +0100

    output-distance: avoid calling strip() on None

[hanwenn, 2020-03-07 18:53:21]

On 2020/03/07 18:47:00, Dan Eble wrote:
> On 2020/03/07 17:49:55, hanwenn wrote:
> > it's a sequence of 6 commmits, which you can inspect here: 
> > 
> >   https://github.com/hanwen/lilypond/pull/2
> 
> The fact that the HTML table now includes rows for removed files is worth
> mentioning in this change.
> 
> commit 613d8478797784ecc9e7676639375e7f18d4abf1
> Author: Han-Wen Nienhuys <mailto:hanwen@lilypond.org>
> Date:   Fri Mar 6 16:30:23 2020 +0100
> 
>     output-distance: avoid calling strip() on None

done.

[hanwenn, 2020-03-07 18:54:04]

On 2020/03/07 18:40:57, Dan Eble wrote:
> On 2020/03/07 17:49:55, hanwenn wrote:
> > it's a sequence of 6 commmits, which you can inspect here: 
> > 
> >   https://github.com/hanwen/lilypond/pull/2 
> 
> Don't you think these two should be combined?  I don't see value in leaving
the
> first commit with an incorrect target name.
> 
> commit 37c6ba676f4722a4e5a7b9965b64d46eff5e0bb1
> Author: Han-Wen Nienhuys <mailto:hanwen@lilypond.org>
> Date:   Fri Mar 6 16:47:26 2020 +0100
> 
>     Fix test target name in python/GNUmakefile


I'll send this separately. It has nothing to do with output-distance.

[hanwenn, 2020-03-07 18:54:58]

On 2020/03/07 18:54:04, hanwenn wrote:
> On 2020/03/07 18:40:57, Dan Eble wrote:
> > On 2020/03/07 17:49:55, hanwenn wrote:
> > > it's a sequence of 6 commmits, which you can inspect here: 
> > > 
> > >   https://github.com/hanwen/lilypond/pull/2 
> > 
> > Don't you think these two should be combined?  I don't see value in leaving
> the
> > first commit with an incorrect target name.
> > 
> > commit 37c6ba676f4722a4e5a7b9965b64d46eff5e0bb1
> > Author: Han-Wen Nienhuys <mailto:hanwen@lilypond.org>
> > Date:   Fri Mar 6 16:47:26 2020 +0100
> > 
> >     Fix test target name in python/GNUmakefile
> 
> 
> I'll send this separately. It has nothing to do with output-distance.

Issue created. URL: http://codereview.appspot.com/575790044
Ticket created at: https://sourceforge.net/p/testlilyissues/issues/5835/

[hanwenn, 2020-03-07 18:55:02]

On 2020/03/07 18:54:04, hanwenn wrote:
> On 2020/03/07 18:40:57, Dan Eble wrote:
> > On 2020/03/07 17:49:55, hanwenn wrote:
> > > it's a sequence of 6 commmits, which you can inspect here: 
> > > 
> > >   https://github.com/hanwen/lilypond/pull/2 
> > 
> > Don't you think these two should be combined?  I don't see value in leaving
> the
> > first commit with an incorrect target name.
> > 
> > commit 37c6ba676f4722a4e5a7b9965b64d46eff5e0bb1
> > Author: Han-Wen Nienhuys <mailto:hanwen@lilypond.org>
> > Date:   Fri Mar 6 16:47:26 2020 +0100
> > 
> >     Fix test target name in python/GNUmakefile
> 
> 
> I'll send this separately. It has nothing to do with output-distance.

Issue created. URL: http://codereview.appspot.com/575790044
Ticket created at: https://sourceforge.net/p/testlilyissues/issues/5835/

[Dan Eble, 2020-03-07 19:08:22]

The logging and diffing changes basically LGTM (see my questions).  I paid no
attention to other changes (self-test, stencil comparison, python idioms, etc.).

https://codereview.appspot.com/581770043/diff/571840081/scripts/build/output-...
File scripts/build/output-distance.py (left):

https://codereview.appspot.com/581770043/diff/571840081/scripts/build/output-...
scripts/build/output-distance.py:1074: print('writing %s' % filename)
Before, this would have been printed whether the output was set to verbose or
terse.  Now, it will only be printed when set to verbose (because
open_write_file() logs with log_verbose()).  (If that's what you want, OK.)

https://codereview.appspot.com/581770043/diff/571840081/scripts/build/output-...
File scripts/build/output-distance.py (right):

https://codereview.appspot.com/581770043/diff/571840081/scripts/build/output-...
scripts/build/output-distance.py:733: log_terse ('creating %s' % outfile)
open_write_file() now logs "writing ..." but you changed this to "creating ...".
 What's the reason for the distinction?

[Dan Eble, 2020-03-07 19:16:04]

On 2020/03/07 18:55:02, hanwenn wrote:
> > On 2020/03/07 18:40:57, Dan Eble wrote:
> > > Don't you think these two should be combined?  I don't see value in
leaving
> > > the first commit with an incorrect target name.
> > 
> > I'll send this separately. It has nothing to do with output-distance.

Oh, I misunderstood.  Yes, it's better to separate it.

[hanwenn, 2020-03-07 19:21:31]

drop python/ fix

[hanwenn, 2020-03-07 22:12:35]

https://codereview.appspot.com/581770043/diff/571840081/scripts/build/output-...
File scripts/build/output-distance.py (left):

https://codereview.appspot.com/581770043/diff/571840081/scripts/build/output-...
scripts/build/output-distance.py:1074: print('writing %s' % filename)
On 2020/03/07 19:08:22, Dan Eble wrote:
> Before, this would have been printed whether the output was set to verbose or
> terse.  Now, it will only be printed when set to verbose (because
> open_write_file() logs with log_verbose()).  (If that's what you want, OK.)

yes, that sounds right. It seems consistent, because we don't write progress for
the .html page either.

https://codereview.appspot.com/581770043/diff/571840081/scripts/build/output-...
File scripts/build/output-distance.py (right):

https://codereview.appspot.com/581770043/diff/571840081/scripts/build/output-...
scripts/build/output-distance.py:733: log_terse ('creating %s' % outfile)
On 2020/03/07 19:08:22, Dan Eble wrote:
> open_write_file() now logs "writing ..." but you changed this to "creating
...".
>  What's the reason for the distinction?

I found the phrasing confusing (it suggests a copying process as in reading and
writing), while it's actually generating the image from scratch.

[hanwenn, 2020-03-08 09:46:43]

backward compatibility; verify that make-check actually works

[hahnjo, 2020-03-11 10:16:52]

https://codereview.appspot.com/581770043/diff/563700043/scripts/build/GNUmake...
File scripts/build/GNUmakefile (right):

https://codereview.appspot.com/581770043/diff/563700043/scripts/build/GNUmake...
scripts/build/GNUmakefile:19: $(PYTHON) output-distance.py --test
This is wrong in 2 regards:
1) This should be local-test as also discussed for python/
2) output-distance.py is not copied over to the build tree, so my build fails. I
think patchy didn't see this problem because it's only running 'make test' and
James skipped 'make check' for the last patchset because you wrote "This means
you can't run a regression test between the state before and after this change."
(which I think is not true; at least it worked for me).

[hanwenn, 2020-03-11 11:49:17]

On Wed, Mar 11, 2020 at 11:16 AM <jonas.hahnfeld@gmail.com> wrote:
>
>
>
https://codereview.appspot.com/581770043/diff/563700043/scripts/build/GNUmake...
> File scripts/build/GNUmakefile (right):
>
>
https://codereview.appspot.com/581770043/diff/563700043/scripts/build/GNUmake...
> scripts/build/GNUmakefile:19: $(PYTHON) output-distance.py --test
> This is wrong in 2 regards:
> 1) This should be local-test as also discussed for python/

done.

> 2) output-distance.py is not copied over to the build tree, so my build
> fails. I think patchy didn't see this problem because it's only running

done.

> 'make test' and James skipped 'make check' for the last patchset because
> you wrote "This means you can't run a regression test between the state
> before and after this change." (which I think is not true; at least it
> worked for me).

I added backward compat code.

 http://codereview.appspot.com/563730043

> https://codereview.appspot.com/581770043/



-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

[hanwenn, 2020-03-15 14:27:55]

On 2020/03/11 11:49:17, hanwenn wrote:
> On Wed, Mar 11, 2020 at 11:16 AM <mailto:jonas.hahnfeld@gmail.com> wrote:
> >
> >
> >
>
https://codereview.appspot.com/581770043/diff/563700043/scripts/build/GNUmake...
> > File scripts/build/GNUmakefile (right):
> >
> >
>
https://codereview.appspot.com/581770043/diff/563700043/scripts/build/GNUmake...
> > scripts/build/GNUmakefile:19: $(PYTHON) output-distance.py --test
> > This is wrong in 2 regards:
> > 1) This should be local-test as also discussed for python/
> 
> done.
> 
> > 2) output-distance.py is not copied over to the build tree, so my build
> > fails. I think patchy didn't see this problem because it's only running
> 
> done.
> 
> > 'make test' and James skipped 'make check' for the last patchset because
> > you wrote "This means you can't run a regression test between the state
> > before and after this change." (which I think is not true; at least it
> > worked for me).
> 
> I added backward compat code.
> 
>  http://codereview.appspot.com/563730043
> 
> > https://codereview.appspot.com/581770043/
> 
> 
> 
> -- 
> Han-Wen Nienhuys - mailto:hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

commit 5e87f62c45263940151f46e94b1cd2cee3f0c601
Merge: 3baa13d1a5 b02d93b9e8
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Wed Mar 11 09:00:59 2020 +0100

    Merge branch 'output-distance-test' into staging
    
    https://sourceforge.net/p/testlilyissues/issues/5824
    http://codereview.appspot.com/581770043