On Sun, Nov 22, 2020 at 1:55 PM Jonas Hahnfeld <hah...@hahnjo.de> wrote:
> Am Samstag, den 21.11.2020, 18:50 +0100 schrieb Jonas Hahnfeld: > > Am Samstag, den 21.11.2020, 09:46 +0100 schrieb Jonas Hahnfeld: > > > Am Freitag, den 20.11.2020, 17:55 -0500 schrieb Dan Eble: > > > > On Nov 20, 2020, at 12:22, Jonas Hahnfeld <hah...@hahnjo.de> wrote: > > > > > > > Despite these shortcomings, I think it would make sense to > enable > > > > > > > this first implementation in lilypond/lilypond. What do you > think? > > > > > > > > > > > > > > > > > > > yes, +1 . I am especially happy that this will likely have no > extra > > > > > > overhead. > > > > > > > > > > No extra overhead per pipeline in MRs, but you get an extra > pipeline > > > > > for every merge to master. We'll see if this poses a serious > problem… > > > > > > > > Given the positive feedback this change has already received, can we > > > > accelerate its progress? I'm eager to have it in place because I've > > > > got a branch with new some regression tests ready. > > > > > > Sure, I can merge !517 later today and then work on the second MR to > > > actually enable 'make check' in the pipelines. > > > > Something's not working correctly here: > > > https://hahnjo.gitlab.io/-/lilypond/-/jobs/864846942/artifacts/test-results/index.html > > The test-baseline was generated by frog (using parallelism), check > > executed on a shared runner. I thought I had tested using a test- > > baseline with different number of cores... > > What I probably checked is various values for CPU_COUNT, including just > one core, and all works correctly AFAICT. What gives differences is not > setting CPU_COUNT / job-count at all. > > I successfully traced this back to compiling, for example, footnote- > volta-spanner.ly and skyline-point-extent.ly (in that order!) with an > added '\include "lilypond-book-preamble.ly"' (attached). If I run > $ ./out/bin/lilypond footnote-volta-spanner.ly skyline-point-extent.ly > the skyline-point-extent.pdf has some added whitespace that vanishes > when passing -djob-count=1 or 2 (starting with 3, it won't fork because > there are less input files than jobs). > > footnote-volta-spanner.ly was added by > commit c1f44faf958aafe7802f432a5ae0f4c74a8c0146 > Author: Han-Wen Nienhuys <hanw...@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? > I'll try to have a look this week (likely Friday). It may be prudent to revert the change if 2.22 is imminent. Any ideas, comments? > Let's not reverse the arg list after fork, to avoid -djob-count confusing things more. I'd like to get rid of the fork call -that would also solve the problem with the options regtest. Let me try to have another look at !204. I am not too optimistic, but it seems worth trying another time. -- Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen