Re: Address output-distance problems: (issue 563730043 by address@hidden)

[dak]

On 2020/03/12 09:52:31, hahnjo wrote:
> On 2020/03/12 09:33:43, hahnjo wrote:
> > On 2020/03/12 09:22:09, dak wrote:
> > > On 2020/03/12 08:01:03, hahnjo wrote:
> > > > This looks like bash-ism which might explain why it works for
Han-Wen and
> > me.
> > > I
> > > > agree with him that disabling the local-test invocation in
GNUmakefile.in
> is
> > > > probably the easiest solution for now. These tests haven't run
for years,
> so
> > > > we'll definitely be fine without them for a few more days.
> > > 
> > > dak@lola:/usr/local/tmp/lilypond$ dash
> > > $ echo {1,2,3}
> > > {1,2,3}
> > > $ 
> > > 
> > > Ah yes.  Since /bin/sh defaults to dash on Ubuntu (or doesn't it
any more?),
> I
> > > wonder how this escaped testing.
> > 
> > It wasn't tested, that's the point: The initial patch only received
a
> > 'test-baseline' and no 'check' which didn't trigger the python
tests.
> > 'patchy-staging' only runs 'test' as far as I understand, so that
patch landed
> > in master. Now this patch adds it to 'test' which means it's the
first time
> > somebody runs it on Ubuntu.
> > I'm for disabling it again until it receives sufficient testing.
Either in
> > current master removing 'local-check' from
'scripts/build/GNUmakefile' or
> taking
> > the updated patchset from here without the 'local-test' recursion in
> > GNUmakefile.in
> 
> To be clear: I'm not blaming anyone, least of all James;
'test-baseline' really
> was the best way possible to test the initial patch before Han-Wen
added
> compatibility code. IMO this just happens to be a bad coincidence of
two
> problems: The test not running from 'make test', but only 'make
check'; and the
> test not working on Ubuntu at all.

Well, there is not a whole lot to be gained from blaming anybody anyway
when there was no damage (not that the blame game makes a lot of sense
when there is damage).  Patch needs work (whether it contains a problem
itself or triggers a preexisting one that needs to be fixed in order for
the patch to go ahead), but it was caught before the problem affected
everyone.

https://codereview.appspot.com/563730043/