Re: [RFC] Automatic 'make check' in CI

[Jonas Hahnfeld]

Am Sonntag, den 22.11.2020, 21:12 +0100 schrieb Han-Wen Nienhuys:
> On Sun, Nov 22, 2020 at 1:55 PM Jonas Hahnfeld <hahnjo@hahnjo.de>
> wrote:
> > footnote-volta-spanner.ly was added by
> > commit c1f44faf958aafe7802f432a5ae0f4c74a8c0146
> > Author: Han-Wen Nienhuys <hanwenn@gmail.com>
> > Date:   Tue Jun 16 08:59:35 2020 +0200
> > 
> >     For footnotes on spanners, copy bounds from underlying spanner.
> > 
> >     This is likely more correct for spanners that run from non-musical
> >     columns (eg. volta brackets) or are created retroactively
> >     (eg. automatic beams).
> > 
> >     Demonstrate this by adding a regression test that puts a footnote on a
> >     VoltaBracket.
> > 
> >     Without adjustments of Footnote_engraver, the test produces the 
> > following:
> > 
> >       programming error: Spanner `FootnoteSpanner' is not fully contained 
> > in parent spanner.  Ignoring orphaned part
> >       continuing, cross fingers
> >       programming error: bounds of this piece aren't breakable.
> >       continuing, cross fingers
> > 
> > (https://gitlab.com/lilypond/lilypond/-/merge_requests/138). During
> > review, David had some concerns about Spanners that were dismissed on
> > the basis that there was no test case that showed wrong results (is
> > that really our standard for claiming that a change works correctly?!).
> > 
> > I tried reverting that commit and all other differences also went away.
> > Does this mean the change is indeed flawed?

Nope, that was a red herring: The reason is that the footnote creation
process in footnote-volta-spanner.ly messes with (point-stencil), which
is used by skyline-point-extent.ly and also the BendSpanner. As far as
I understand the Stencil class, its objects are not immutable...

One of the following two changes fixes the problem in my local test
scenario:

---
diff --git a/ly/music-functions-init.ly b/ly/music-functions-init.ly
index 13f5c37811..17f14b1f07 100644
--- a/ly/music-functions-init.ly
+++ b/ly/music-functions-init.ly
@@ -538,7 +538,7 @@ to the preceding note or rest as a post-event with 
@code{-}.")
                'X-offset (car offset)
                'Y-offset (cdr offset)
                'automatically-numbered (not mark)
-               'text (or mark (make-null-markup))
+               'text (or mark (ly:make-stencil "" '(0 . 0) '(0 . 0)))
                'footnote-text footnote)))
      (once (propertyTweak 'footnote-music mus item))))
 
diff --git a/scm/define-markup-commands.scm b/scm/define-markup-commands.scm
index f7280f7a58..1ab6eaafe3 100644
--- a/scm/define-markup-commands.scm
+++ b/scm/define-markup-commands.scm
@@ -1379,7 +1379,8 @@ An empty markup with extents of a single point.
   \\null
 }
 @end lilypond"
-  point-stencil)
+  ;; Create a new point-stencil every time, it might get modified...
+  (ly:make-stencil "" '(0 . 0) '(0 . 0)))
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; basic formatting.
---

My gut feeling is that we should fix null-markup because nothing should
ever translate / rotate / modify the global point-stencil, thoughts?



I'll try to have a look this week (likely Friday). It may be prudent to
revert the change if 2.22 is imminent.

Actually CG says that whatever code compiled with a beta release must
work with later releases of that series. I think that makes sense and
we should stick to it...

Jonas

signature.asc
Description: This is a digitally signed message part