[PATCH v2] Fix crash when a stencil routine is missing

[Patrick McCarty-3]

Hello,

I have updated my patchset on Rietveld:

http://codereview.appspot.com/83046/show

Thank you all for your reviews.  I have a few questions/concerns:

1.) I used Jan's suggestion for -d option name, but I'm not sure this
is the best name.  What should the option be called?

2.) Is backend-library.scm a suitable place for the "backend-testing" procedure?

3.) I don't know how to enable the -d option by default for regression
test compilation, so should I delegate this task?


Thanks,
Patrick


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Graham Percival-3]

On Sun, Jul 05, 2009 at 02:29:44PM -0700, Patrick McCarty wrote:
> 1.) I used Jan's suggestion for -d option name, but I'm not sure this
> is the best name.  What should the option be called?

-dwarning-as-error
looks fine to me.

> 3.) I don't know how to enable the -d option by default for regression
> test compilation, so should I delegate this task?

You can enable any -d option with something like
  #(set warning-as-error)
but I'm not certain if that's the exact command.

... oh wait, here it is:
#(ly:set-option 'point-and-click #f)

Cheers,
- Graham


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Patrick McCarty-3]

On Sun, Jul 05, 2009 at 05:39:45PM -0700, Graham Percival wrote:
> On Sun, Jul 05, 2009 at 02:29:44PM -0700, Patrick McCarty wrote:
> > 1.) I used Jan's suggestion for -d option name, but I'm not sure this
> > is the best name.  What should the option be called?
>
> -dwarning-as-error
> looks fine to me.

Okay.  On second thought, it looks decent to me too.

> > 3.) I don't know how to enable the -d option by default for regression
> > test compilation, so should I delegate this task?
>
> You can enable any -d option with something like
>   #(set warning-as-error)
> but I'm not certain if that's the exact command.
>
> ... oh wait, here it is:
> #(ly:set-option 'point-and-click #f)

I think Han-Wen suggested to enable this option for the entire
regression test suite, so adding this option to every single
regression test would not be reasonable.

In other words, some makefile hacking is needed, which I would
probably need to delegate.

Thanks,
Patrick


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Graham Percival-3]

On Sun, Jul 05, 2009 at 06:01:21PM -0700, Patrick McCarty wrote:
> On Sun, Jul 05, 2009 at 05:39:45PM -0700, Graham Percival wrote:
> > On Sun, Jul 05, 2009 at 02:29:44PM -0700, Patrick McCarty wrote:
> > > 1.) I used Jan's suggestion for -d option name, but I'm not sure this
> > > is the best name.  What should the option be called?
> >
> > -dwarning-as-error
> > looks fine to me.
>
> Okay.  On second thought, it looks decent to me too.

Wasn't that your initial proposal?  Did I misread the patch?

> > ... oh wait, here it is:
> > #(ly:set-option 'point-and-click #f)
>
> I think Han-Wen suggested to enable this option for the entire
> regression test suite, so adding this option to every single
> regression test would not be reasonable.
>
> In other words, some makefile hacking is needed, which I would
> probably need to delegate.

I doubt that the regtests compile without warnings, and I'm not
certain that we have enough Frogs to eat all the issues that would
be added if we went this route.

(I'd love to be proven wrong about this, though!)

Cheers,
- Graham


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Patrick McCarty-3]

On Sun, Jul 05, 2009 at 08:22:02PM -0700, Graham Percival wrote:
> On Sun, Jul 05, 2009 at 06:01:21PM -0700, Patrick McCarty wrote:
> > On Sun, Jul 05, 2009 at 05:39:45PM -0700, Graham Percival wrote:
> > >
> > > -dwarning-as-error
> > > looks fine to me.
> >
> > Okay.  On second thought, it looks decent to me too.
>
> Wasn't that your initial proposal?  Did I misread the patch?

I was reusing Jan's name for the option, and initially I was not sure
I liked the name.  Now I think the name looks okay (semantically).

> > > ... oh wait, here it is:
> > > #(ly:set-option 'point-and-click #f)
> >
> > I think Han-Wen suggested to enable this option for the entire
> > regression test suite, so adding this option to every single
> > regression test would not be reasonable.
> >
> > In other words, some makefile hacking is needed, which I would
> > probably need to delegate.
>
> I doubt that the regtests compile without warnings, and I'm not
> certain that we have enough Frogs to eat all the issues that would
> be added if we went this route.
>
> (I'd love to be proven wrong about this, though!)

The patch will enable warnings by default for every missing stencil,
but the new option will enable *errors*, causing the regression test
suite to fail.

Since I don't know how to enable the new option for the entire test
suite, I'm presently compiling the docs and will examine the logs for
the new warnings.

Thanks,
Patrick


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Werner LEMBERG]

>> In other words, some makefile hacking is needed, which I would
>> probably need to delegate.

I agree.

> I doubt that the regtests compile without warnings,

Correct.  Some of them are even expected to emit warning or error messages.

> and I'm not certain that we have enough Frogs to eat all the issues
> that would be added if we went this route.

Perhaps it makes sense to classify regression tests like this:

  a) intentionally emit warnings and errors

  b) unintentionally emit warnings

  c) run fine

Item c) should be the default, this is, such regression tests
shouldn't be mentioned specially.

Regression tests from item a) must be collected, of course.

Everything else should go to item b), and the job of volunteers would
be to make this list empty.


    Werner


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[John Mandereau-2]

Le dimanche 05 juillet 2009 à 18:01 -0700, Patrick McCarty a écrit :

> On Sun, Jul 05, 2009 at 05:39:45PM -0700, Graham Percival wrote:
> > -dwarning-as-error
> > looks fine to me.
>
> I think Han-Wen suggested to enable this option for the entire
> regression test suite, so adding this option to every single
> regression test would not be reasonable.
>
> In other words, some makefile hacking is needed, which I would
> probably need to delegate.

What about the following?

Best,
John


diff --git a/input/regression/GNUmakefile b/input/regression/GNUmakefile
index 753d483..f1a8179 100644
--- a/input/regression/GNUmakefile
+++ b/input/regression/GNUmakefile
@@ -7,3 +7,5 @@ include $(depth)/make/stepmake.make
 TITLE=LilyPond Regression Tests
 
 SUBDIRS=musicxml
+
+LILYPOND_BOOK_LILYPOND_FLAGS += -dwarning-as-error





_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Patrick McCarty-3]

On Mon, Jul 06, 2009 at 09:37:19AM +0200, John Mandereau wrote:

> Le dimanche 05 juillet 2009 à 18:01 -0700, Patrick McCarty a écrit :
> > On Sun, Jul 05, 2009 at 05:39:45PM -0700, Graham Percival wrote:
> > > -dwarning-as-error
> > > looks fine to me.
> >
> > I think Han-Wen suggested to enable this option for the entire
> > regression test suite, so adding this option to every single
> > regression test would not be reasonable.
> >
> > In other words, some makefile hacking is needed, which I would
> > probably need to delegate.
>
> What about the following?
>
> Best,
> John
>
>
> diff --git a/input/regression/GNUmakefile b/input/regression/GNUmakefile
> index 753d483..f1a8179 100644
> --- a/input/regression/GNUmakefile
> +++ b/input/regression/GNUmakefile
> @@ -7,3 +7,5 @@ include $(depth)/make/stepmake.make
>  TITLE=LilyPond Regression Tests
>  
>  SUBDIRS=musicxml
> +
> +LILYPOND_BOOK_LILYPOND_FLAGS += -dwarning-as-error

This looks great!  I'll save it and apply if/when the patchset is
finalized and approved.

Thanks,
Patrick


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Graham Percival-3]

On Mon, Jul 06, 2009 at 07:09:11AM +0200, Werner LEMBERG wrote:
> > and I'm not certain that we have enough Frogs to eat all the issues
> > that would be added if we went this route.
>
> Perhaps it makes sense to classify regression tests like this:
>
>   a) intentionally emit warnings and errors
>   b) unintentionally emit warnings
>   c) run fine

I don't see a huge urgency for this patch, so I would rather wait
until b) was non-existent.  I totally agree that we should split
a) and c), though.

Let's wait until Patrick reports how many regtests unintentionally
emit warnings, and see how much work is involved in fixing those.

Cheers,
- Graham


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Patrick McCarty-3]

On Mon, Jul 06, 2009 at 03:08:50PM -0700, Graham Percival wrote:

> On Mon, Jul 06, 2009 at 07:09:11AM +0200, Werner LEMBERG wrote:
> > > and I'm not certain that we have enough Frogs to eat all the issues
> > > that would be added if we went this route.
> >
> > Perhaps it makes sense to classify regression tests like this:
> >
> >   a) intentionally emit warnings and errors
> >   b) unintentionally emit warnings
> >   c) run fine
>
> I don't see a huge urgency for this patch, so I would rather wait
> until b) was non-existent.  I totally agree that we should split
> a) and c), though.

I think there is a slight confusion about what my patch implements.

The new option, -dwarning-as-error, does *not* turn every warning into
an error.  It only turns specific warnings (that don't exist without
this patch) into errors.

The purpose is more of a long-term solution, so you're right that it's
not at all urgent.

For example, with this patch, if you compile a file containing three
"embedded-ps" stencil expressions with the SVG backend, you'll get
this output instead of a crash:

  $ lilypond -dbackend=svg test.ly
  GNU LilyPond 2.13.4
  Processing `ps.ly'
  Parsing...
  Interpreting music...
  Preprocessing graphical objects...
  Solving 1 page-breaking chunks...[1: 1 pages]
  Drawing systems...
  Layout output to `ps.svg'...
  warning: missing stencil expression `embedded-ps'
  warning: missing stencil expression `embedded-ps'
  warning: missing stencil expression `embedded-ps'

With the -d option, you get this:

  $ lilypond -dbackend=svg -dwarning-as-error test.ly
  GNU LilyPond 2.13.4
  Processing `ps.ly'
  Parsing...
  Interpreting music...
  Preprocessing graphical objects...
  Solving 1 page-breaking chunks...[1: 1 pages]
  Drawing systems...
  Layout output to `ps.svg'...
  error: missing stencil expression `embedded-ps'

and an invalid SVG file.

But, since the regression test suite only exercises the PostScript
backend, which doesn't currently have any missing any *defined*
stencil expressions, we won't be seeing any failures right now.

> Let's wait until Patrick reports how many regtests unintentionally
> emit warnings, and see how much work is involved in fixing those.

There are no regtests that emit the new warning.  There are plenty of
regtests that emit warnings, but not the warning in question.

If we want to implement an option to turn *all* warnings into errors,
that would require a different patch.

So, maybe we should change the -d option name after all.  :-)

Thanks,
Patrick


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Patrick McCarty-3]

On Mon, Jul 06, 2009 at 03:37:06PM -0700, Patrick McCarty wrote:
>
> But, since the regression test suite only exercises the PostScript
> backend, which doesn't currently have any missing any *defined*
> stencil expressions, we won't be seeing any failures right now.

I should emphasize the *right now* part, because I want to add an
"embedded-svg" stencil expression that would only be valid for the SVG
backend, thus causing crashes in the PostScript backend.

In the future, I hope we can have real regression tests for the SVG
backend.  This would require an output-preview-framework and an
external tool to convert SVG->PNG.

In this case, any regtest that uses "embedded-svg" that accidentally
uses the PostScript backend will fail, and the appropriate changes can
be made to the regtest.

-Patrick


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Graham Percival-3]

On Mon, Jul 06, 2009 at 03:37:06PM -0700, Patrick McCarty wrote:

> On Mon, Jul 06, 2009 at 03:08:50PM -0700, Graham Percival wrote:
> > I don't see a huge urgency for this patch, so I would rather wait
> > until b) was non-existent.  I totally agree that we should split
> > a) and c), though.
>
> I think there is a slight confusion about what my patch implements.
>
> The new option, -dwarning-as-error, does *not* turn every warning into
> an error.  It only turns specific warnings (that don't exist without
> this patch) into errors.

Oh!  Yes, I was definitely confused.  (that said, I _do_ like the
notion of a "complete" -dwarning-as-error, for regtests, *if* we
can check them all)

OTOH, since you only want to warning-error-check certain files,
why not make -dwarning-as-error as I misunderstood, then enable it
in those specific svg-related regtests?  We can work towards
adding it as a general makefile rule later.

I'm not certain if a -dsvg-warning-as-error patch is particularly
useful, even for the regtests.

Cheers,
- Graham


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Werner LEMBERG]

> The new option, -dwarning-as-error, does *not* turn every warning into
> an error.  It only turns specific warnings (that don't exist without
> this patch) into errors.

Then the name is indeed too generic and should be temporarily renamed
to something more specific until the code has been improved to convert
all warnings to errors.


    Werner


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Patrick McCarty-3]

On Tue, Jul 07, 2009 at 08:16:45AM +0200, Werner LEMBERG wrote:
>
> > The new option, -dwarning-as-error, does *not* turn every warning into
> > an error.  It only turns specific warnings (that don't exist without
> > this patch) into errors.
>
> Then the name is indeed too generic and should be temporarily renamed
> to something more specific until the code has been improved to convert
> all warnings to errors.

I think it would be better to work on creating a generic
-dwarning-as-error now instead.

I can see how to do it on the Scheme side (a simple rebinding).  Would
I need to write preprocessor macros to achieve this for the C++ code?


Thanks,
Patrick


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Joe Neeman-2]

On Tue, 2009-07-07 at 01:24 -0700, Patrick McCarty wrote:

> On Tue, Jul 07, 2009 at 08:16:45AM +0200, Werner LEMBERG wrote:
> >
> > > The new option, -dwarning-as-error, does *not* turn every warning into
> > > an error.  It only turns specific warnings (that don't exist without
> > > this patch) into errors.
> >
> > Then the name is indeed too generic and should be temporarily renamed
> > to something more specific until the code has been improved to convert
> > all warnings to errors.
>
> I think it would be better to work on creating a generic
> -dwarning-as-error now instead.
>
> I can see how to do it on the Scheme side (a simple rebinding).  Would
> I need to write preprocessor macros to achieve this for the C++ code?

On the C++ side, all warnings and programming_errors go through
Grob::programming_error, Grob::warning, Input::programming_error or
Input::warning. So you'd just need to modify those 4 functions.

Joe



_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel