T1055: Avoid using deprecated %module-public-interface in guile initialisation.

T1055: Avoid using deprecated %module-public-interface in guile initialisation. 
Also rename ly_make_anonymous_module to ly_scope_make_module

[Carl, 2010-05-11 23:42:22]

THanks, Ian.  I think this is a real contribution.  I appreciate your tackling
it.

The patch looks mostly good to me, but I've got a few comments.

I look forward to other reviewers on -devel chiming in.

Carl

http://codereview.appspot.com/1160044/diff/1/6
File lily/ly-module.cc (right):

http://codereview.appspot.com/1160044/diff/1/6#newcode57
lily/ly-module.cc:57: 
Why the blank line here?

http://codereview.appspot.com/1160044/diff/1/9
File scm/lily.scm (right):

http://codereview.appspot.com/1160044/diff/1/9#newcode52
scm/lily.scm:52: (aux-files #t
Whitespace errors (space at end of line in 52, 53).

http://codereview.appspot.com/1160044/diff/1/9#newcode774
scm/lily.scm:774: (exit 1))
This change undoes the success error message patch.  You need to rebase your
patch.

[hanwenn, 2010-05-12 02:24:19]

looks good overall. Some nits:

http://codereview.appspot.com/1160044/diff/1/3
File lily/include/ly-module.hh (right):

http://codereview.appspot.com/1160044/diff/1/3#newcode25
lily/include/ly-module.hh:25: SCM ly_scope_make_module (bool safe);
IIRC think the standard is ly_make_XXXX for these functions.  Why the 'scope' in
the name?  ly_make_module sounds fine to me

http://codereview.appspot.com/1160044/diff/1/6
File lily/ly-module.cc (right):

http://codereview.appspot.com/1160044/diff/1/6#newcode61
lily/ly-module.cc:61: SCM maker = ly_lily_module_constant ("make-module");
move to use, all of them

http://codereview.appspot.com/1160044/diff/1/9
File scm/lily.scm (right):

http://codereview.appspot.com/1160044/diff/1/9#newcode250
scm/lily.scm:250: (define-public guile-v2 #f)	; reset to #t when integrating
Guile V2.0 with Lilypond
you could use (guile-version) to set this?

[Ian Hulin, 2010-05-13 20:37:23]

I've answered all the comments bar one,

Han-Wen could you spell out what you meant in ly_module.cc:61?

Cheers,

Ian

http://www.codereview.appspot.com/1160044/diff/1/3
File lily/include/ly-module.hh (right):

http://www.codereview.appspot.com/1160044/diff/1/3#newcode25
lily/include/ly-module.hh:25: SCM ly_scope_make_module (bool safe);
On 2010/05/12 02:24:19, hanwenn wrote:
> IIRC think the standard is ly_make_XXXX for these functions.  Why the 'scope'
in
> the name?  ly_make_module sounds fine to me

It's generally called in the code to create a Lilypond scope, as you explained
in an e-mail.  I thought this name was more descriptive of its Lilypond usage,
but if it breaks a standard, I'll change it to ly_make_module.

http://www.codereview.appspot.com/1160044/diff/1/6
File lily/ly-module.cc (right):

http://www.codereview.appspot.com/1160044/diff/1/6#newcode57
lily/ly-module.cc:57: 
On 2010/05/11 23:42:22, Carl wrote:
> Why the blank line here?

Removed.

http://www.codereview.appspot.com/1160044/diff/1/6#newcode61
lily/ly-module.cc:61: SCM maker = ly_lily_module_constant ("make-module");
On 2010/05/12 02:24:19, hanwenn wrote:
> move to use, all of them

Sorry Han-Wen, I don't understand your feedback, could you clarify?

http://www.codereview.appspot.com/1160044/diff/1/9
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/1/9#newcode52
scm/lily.scm:52: (aux-files #t
On 2010/05/11 23:42:22, Carl wrote:
> Whitespace errors (space at end of line in 52, 53).
Sorted

http://www.codereview.appspot.com/1160044/diff/1/9#newcode250
scm/lily.scm:250: (define-public guile-v2 #f)	; reset to #t when integrating
Guile V2.0 with Lilypond
On 2010/05/12 02:24:19, hanwenn wrote:
> you could use (guile-version) to set this?

Will change to 
;;; Flag - are we using guile V.2 or higher?
;:: thunk - are we integrating Guile V2.0 with Lilypond
(define-public  (guile-v2 )
    (string>? (version) "1.8.7"))

http://www.codereview.appspot.com/1160044/diff/1/9#newcode774
scm/lily.scm:774: (exit 1))
On 2010/05/11 23:42:22, Carl wrote:
> This change undoes the success error message patch.  You need to rebase your
> patch.

Restored

[hanwenn, 2010-05-14 03:23:54]

http://www.codereview.appspot.com/1160044/diff/1/6
File lily/ly-module.cc (right):

http://www.codereview.appspot.com/1160044/diff/1/6#newcode61
lily/ly-module.cc:61: SCM maker = ly_lily_module_constant ("make-module");
On 2010/05/13 20:37:23, Ian Hulin wrote:
> On 2010/05/12 02:24:19, hanwenn wrote:
> > move to use, all of them
> 
> Sorry Han-Wen, I don't understand your feedback, could you clarify?


rather than  

  SCM x = ..;
  SCM y = ..;

  z = foo(x);
  call(y);

do


  SCM x = ..;
  z = foo(x);

  SCM y = ..;
  call(y);

ie. declare vars just before their first use.

[Ian Hulin, 2010-05-15 19:56:47]

http://www.codereview.appspot.com/1160044/diff/1/6
File lily/ly-module.cc (right):

http://www.codereview.appspot.com/1160044/diff/1/6#newcode61
lily/ly-module.cc:61: SCM maker = ly_lily_module_constant ("make-module");
On 2010/05/14 03:23:55, hanwenn wrote:
> On 2010/05/13 20:37:23, Ian Hulin wrote:
> > On 2010/05/12 02:24:19, hanwenn wrote:
> > > move to use, all of them
> > 
> > Sorry Han-Wen, I don't understand your feedback, could you clarify?
> 
> 
> rather than  
> 
>   SCM x = ..;
>   SCM y = ..;
> 
>   z = foo(x);
>   call(y);
> 
> do
> 
> 
>   SCM x = ..;
>   z = foo(x);
> 
>   SCM y = ..;
>   call(y);
> 
> i.e. declare vars just before their first use. 

I really don't like interlacing data declarations with executable code, and I'd
have felt happier if all the declarations were at the top of the function
separate from the code. . .
I've coded it as I have so only the relevant changes show up, even though I
personally disagree with the existing coding style.  
We obviously come from opposing schools of programming style so if it doesn't
affect the functionality we'll have to agree to differ on this one.

[Patrick McCarty, 2010-05-15 21:39:39]

On 2010/05/15 19:56:47, Ian Hulin wrote:
> On 2010/05/14 03:23:55, hanwenn wrote:
> > 
> > rather than  
> > 
> >   SCM x = ..;
> >   SCM y = ..;
> > 
> >   z = foo(x);
> >   call(y);
> > 
> > do
> > 
> > 
> >   SCM x = ..;
> >   z = foo(x);
> > 
> >   SCM y = ..;
> >   call(y);
> > 
> > i.e. declare vars just before their first use. 
> 
> I really don't like interlacing data declarations with
> executable code, and I'd have felt happier if all the
> declarations were at the top of the function separate from the
> code. . .  I've coded it as I have so only the relevant changes
> show up, even though I personally disagree with the existing
> coding style.  We obviously come from opposing schools of
> programming style so if it doesn't affect the functionality
> we'll have to agree to differ on this one.

Hi Ian,

If the location at which a variable is declared/initialized is so
close to its place of usage, why bother putting the declarations
at the top of a block?

IMO, it's not worth going through the trouble of separating
everything unless it's necessary.  From my experience, it
decreases code readability.  I would do it like this:

  SCM maker = ly_lily_module_constant ("make-module");
  mod = scm_call_0 (maker);
  
  SCM module_export_all_x = ly_lily_module_constant ("module-export-all!");
  scm_call_1 (module_export_all_x, mod);
  
  SCM scm_module = ly_lily_module_constant ("the-scm-module");
  ly_use_module (mod, scm_module);
  ly_use_module (mod, global_lily_module);

[Patrick McCarty, 2010-05-15 21:53:15]

Hi Ian,

I haven't tested your patch yet to make sure everything is okay, but I have a
few quick comments for you.

Thanks,
Patrick

http://www.codereview.appspot.com/1160044/diff/10001/11008
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode250
scm/lily.scm:250: (define-public  (guile-v2 )
(define-public (guile-v2)

http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode251
scm/lily.scm:251: (string>? (version) "1.8.7"))
What if the Guile team decides to release 1.8.8 ?

It would be safer to change this to some 1.9 version...

http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode303
scm/lily.scm:303: (cond-expand
Shouldn't this `cond-expand' block start in column 1, and then indented from
there?

[Patrick McCarty, 2010-05-15 21:55:16]

http://www.codereview.appspot.com/1160044/diff/10001/11008
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode251
scm/lily.scm:251: (string>? (version) "1.8.7"))
On 2010/05/15 21:53:15, Patrick McCarty wrote:
> What if the Guile team decides to release 1.8.8 ?
> 
> It would be safer to change this to some 1.9 version...

Oh yes, and please fix the indentation here too.

Which editor are you using that uses 4 spaces for indentation of Scheme code?

[Ian Hulin, 2010-05-16 10:39:14]

On 2010/05/15 21:39:39, Patrick McCarty wrote:
> On 2010/05/15 19:56:47, Ian Hulin wrote:
> > On 2010/05/14 03:23:55, hanwenn wrote:
> > > 
> > > rather than  
> > > 
> > >   SCM x = ..;
> > >   SCM y = ..;
> > > 
> > >   z = foo(x);
> > >   call(y);
> > > 
> > > do
> > > 
> > > 
> > >   SCM x = ..;
> > >   z = foo(x);
> > > 
> > >   SCM y = ..;
> > >   call(y);
> > > 
> > > i.e. declare vars just before their first use. 
> > 
> > I really don't like interlacing data declarations with
> > executable code, and I'd have felt happier if all the
> > declarations were at the top of the function separate from the
> > code. . .  I've coded it as I have so only the relevant changes
> > show up, even though I personally disagree with the existing
> > coding style.  We obviously come from opposing schools of
> > programming style so if it doesn't affect the functionality
> > we'll have to agree to differ on this one.
> 
> Hi Ian,
> 
> If the location at which a variable is declared/initialized is so
> close to its place of usage, why bother putting the declarations
> at the top of a block?
> 
> IMO, it's not worth going through the trouble of separating
> everything unless it's necessary.  From my experience, it
> decreases code readability.  I would do it like this:
> 
>   SCM maker = ly_lily_module_constant ("make-module");
>   mod = scm_call_0 (maker);
>   
>   SCM module_export_all_x = ly_lily_module_constant ("module-export-all!");
>   scm_call_1 (module_export_all_x, mod);
>   
>   SCM scm_module = ly_lily_module_constant ("the-scm-module");
>   ly_use_module (mod, scm_module);
>   ly_use_module (mod, global_lily_module);

I'll reluctantly conform if that's the house style, but it needs to be
documented as a standard in the Contributors Guide.

I've added the comments so hopefully another maintainer would find it simple to
see what's going on without having to bother Han-Wen or the developers' list.

Cheers,
Ian

[Ian Hulin, 2010-05-16 10:39:48]

http://www.codereview.appspot.com/1160044/diff/10001/11008
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode251
scm/lily.scm:251: (string>? (version) "1.8.7"))
On 2010/05/15 21:53:15, Patrick McCarty wrote:
> What if the Guile team decides to release 1.8.8 ?
> 
> It would be safer to change this to some 1.9 version...
Changed to "1.9.0"
... and fixed indentation

http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode303
scm/lily.scm:303: (cond-expand
On 2010/05/15 21:53:15, Patrick McCarty wrote:
> Shouldn't this `cond-expand' block start in column 1, and then indented from
> there?

Done.

[Neil Puttock, 2010-05-17 22:44:22]

http://www.codereview.appspot.com/1160044/diff/20001/16009
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/20001/16009#newcode250
scm/lily.scm:250: (define-public  (guile-v2 )
(define-public (guile-v2)

http://www.codereview.appspot.com/1160044/diff/20001/16009#newcode251
scm/lily.scm:251: (string>? (version) "1.9.0"))
This must at least be "1.9.10".

http://www.codereview.appspot.com/1160044/diff/20001/16009#newcode304
scm/lily.scm:304: ((not guile-2)
guile-v2

otherwise it's always expanded

[Ian Hulin, 2010-05-18 17:42:27]

Thanks for the catch, Neil,

Cheers,
Ian

http://www.codereview.appspot.com/1160044/diff/20001/16009
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/20001/16009#newcode251
scm/lily.scm:251: (string>? (version) "1.9.0"))
On 2010/05/17 22:44:22, Neil Puttock wrote:
> This must at least be "1.9.10".

Done.

http://www.codereview.appspot.com/1160044/diff/20001/16009#newcode304
scm/lily.scm:304: ((not guile-2)
On 2010/05/17 22:44:22, Neil Puttock wrote:
> guile-v2
> 
> otherwise it's always expanded

Ouch! Nice catch.

[Neil Puttock, 2010-05-19 22:42:17]

Hi Ian,

I've tried testing your latest patch, but it fails on clip-systems.ly.

`make-rhythmic-location' (defined in clip-region.scm) appears to be inaccessible
from a \layout block.

Cheers,
Neil

[Ian Hulin, 2010-05-22 19:52:01]

Hi Neil,

On 19/05/10 23:42, n.puttock@gmail.com wrote:
> Hi Ian,
>
> I've tried testing your latest patch, but it fails on clip-systems.ly.
>
> `make-rhythmic-location' (defined in clip-region.scm) appears to be
> inaccessible from a \layout block.
Rats. . .
I'll investigate but I may need some help with this.

Cheers,

Ian

[Ian Hulin, 2010-07-28 22:49:11]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all,

On 19/05/10 23:42, n.puttock@gmail.com wrote:
> Hi Ian,
> 
> I've tried testing your latest patch, but it fails on clip-systems.ly.
> 
> `make-rhythmic-location' (defined in clip-region.scm) appears to be
> inaccessible from a \layout block.
> 
> Cheers,
> Neil
>

A new patch-set is available for review on Rietveld which fixes this
problem.

> http://www.codereview.appspot.com/1160044/show

Cheers,

Ian

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJMULPbAAoJEBqidDirZqASEhwH/A414FW1yozKvhwhGFwTYPqB
KNRIFkZU5P77MxDLHhqZdn8tGkaVv7E0Qee+yShfW/P7g2prd/DW9loiHj0hucPS
VRHnQBe8QQ97d8aMsN9K+vXg9/p+JqP2mevzYGF6ZJIJHfXVBYOJlzfxjU8gUEWD
rIfqVqpw1gjoSZ0epCexz5Oac+saT6pzFa+kuJsJDnd5ZfuAbpWUBKU2X779AQIo
Kj7ny9swypkwgB+oTHkEF7+/t1goCy3+Y7CkL1eCsLhu4iN22YjfOfY4fw1ldhp5
z9KU0JUsC0NkbLL1EiQNzGmjFoz5srUd3wPIaoPo2cf4dQQXvZ6jmxUsK/3RPpU=
=RYS3
-----END PGP SIGNATURE-----

[Patrick McCarty, 2010-07-29 00:28:24]

Hi Ian,

On 2010/07/28 22:49:11, Ian Hulin wrote:
> On 19/05/10 23:42, mailto:n.puttock@gmail.com wrote:
> > 
> > I've tried testing your latest patch, but it fails on clip-systems.ly.
> > 
> > `make-rhythmic-location' (defined in clip-region.scm) appears to be
> > inaccessible from a \layout block.
> 
> A new patch-set is available for review on Rietveld which fixes this
> problem.

Hi Ian,

I'll be testing your patchset shortly and will let you know how it goes.

Can you fix all of the whitespace changes (tab->spaces) you made in these three
files?  For example, the majority of diff for scm/lily.scm consists of
whitespace changes.

IIUC, we (developers) haven't reached a consensus about code style and tabs vs.
spaces, so I think the best thing is to not make large whitespace changes that
conflict with the surrounding code.  That is, until we reach a consensus.

Thanks,
Patrick

[Patrick McCarty, 2010-07-29 00:55:35]

On 2010/07/29 00:28:24, Patrick McCarty wrote:
> On 2010/07/28 22:49:11, Ian Hulin wrote:
> > On 19/05/10 23:42, mailto:n.puttock@gmail.com wrote:
> > > 
> > > I've tried testing your latest patch, but it fails on clip-systems.ly.
> > > 
> > > `make-rhythmic-location' (defined in clip-region.scm) appears to be
> > > inaccessible from a \layout block.
> > 
> > A new patch-set is available for review on Rietveld which fixes this
> > problem.
> 
> Hi Ian,
> 
> I'll be testing your patchset shortly and will let you know how it goes.

With your patchset, I am getting a compile failure with the regression test
`profile-property-access.ly':

profile-property-access.ly:44:1: error: GUILE signaled an error for the
expression beginning here
#
 (display-stats 'prob)
FORMAT: Unsupported format option ~3 - use (ice-9 format) instead
profile-property-access.ly:45:1: error: GUILE signaled an error for the
expression beginning here
#
 (display-stats 'context)
FORMAT: Unsupported format option ~3 - use (ice-9 format) instead
profile-property-access.ly:46:1: error: GUILE signaled an error for the
expression beginning here
#
 (display-stats 'grob)
FORMAT: Unsupported format option ~3 - use (ice-9 format) instead


Thanks,
Patrick

[Neil Puttock, 2010-07-29 19:24:46]

On 2010/07/29 00:55:35, Patrick McCarty wrote:

> With your patchset, I am getting a compile failure with the regression test
> `profile-property-access.ly':

Same here.

+ = scm_list_3 (ly_symbol2scm ("module-use!"), mod, used);

This effectively exports all bindings, so all local defines are now exported.

In the case of `profile-property-access.ly', the format call ends up using
`ergonomic-simple-format' from lily.scm.

Cheers,
Neil

[Ian Hulin, 2010-08-01 00:11:44]

Hi Patrick, Neil and all,

On 29/07/10 20:24, n.puttock@gmail.com wrote:
> On 2010/07/29 00:55:35, Patrick McCarty wrote:
>
>> With your patchset, I am getting a compile failure with the regression
> test
>> `profile-property-access.ly':
>
> Same here.

How did you guys track it down to this module?  I only get from the
regression test is this message (this is the tail of the log file).
<snip>

Processing 59/lily-af159abe
Processing 4f/lily-cdefb8cf
Processing 52/lily-23a75a14
command failed: /home/ian/lilypond/out/bin/lilypond -I ./ -I ./out-test
-I ../../input -I /home/ian/lilypond/Documentation -I
/home/ian/lilypond/Documentation/snippets -I ../../input/regression/ -I
/home/ian/lilypond/Documentation/included/ -I /home/ian/lilypond/mf/out/
-I /home/ian/lilypond/mf/out/ -I
/home/ian/lilypond/Documentation/pictures -I
/home/ian/lilypond/Documentation/pictures/./out-test -dbackend=eps
--formats=ps  -dseparate-log-files -dinclude-eps-fonts
-dgs-load-lily-fonts --header=texidoc -I
/home/ian/lilypond/Documentation/included/ -ddump-profile
-dcheck-internal-types -ddump-signatures -danti-alias-factor=1 -I
"/home/ian/lilypond/out/lybook-testdb"  -I
"/home/ian/lilypond/input/regression"  -I
"/home/ian/lilypond/input/regression"  -I
"/home/ian/lilypond/input/regression/out-test"  -I
"/home/ian/lilypond/input"  -I  "/home/ian/lilypond/Documentation"  -I
"/home/ian/lilypond/Documentation/snippets"  -I
"/home/ian/lilypond/input/regression"  -I
"/home/ian/lilypond/Documentation/included"  -I
"/home/ian/lilypond/mf/out"  -I  "/home/ian/lilypond/mf/out"  -I
"/home/ian/lilypond/Documentation/pictures"  -I
"/home/ian/lilypond/Documentation/pictures/out-test" --formats=eps
-deps-box-padding=3.000000  -dread-file-list -dno-strip-output-dir
"/home/ian/lilypond/out/lybook-testdb/snippet-names--1834923873.ly"
Child returned 1
make[2]: *** [out-test/collated-files.texi] Error 1
make[1]: *** [local-test] Error 2
make: *** [test] Error 2


>
> + = scm_list_3 (ly_symbol2scm ("module-use!"), mod, used);
>
> This effectively exports all bindings, so all local defines are now
> exported.
>

Unless I misunderstood Han-Wen,that's why we were jerking around with
%module-public-interface in the first place.

> In the case of `profile-property-access.ly', the format call ends up
> using `ergonomic-simple-format' from lily.scm.
>
O.K. I reckon this will do the biz. in lily.scm

(use-modules (ice-9 regex)
               (ice-9 safe)
               (ice-9 format)
               (ice-9 rdelim)
               (ice-9 optargs)
               (oop goops)
               (srfi srfi-1)
               (srfi srfi-13)
               (srfi srfi-14)
               (scm clip-region)
               (scm memory-trace)
               (scm coverage))

(define-public fancy-format
   format)

(define (simple-format-handler dest . rest);
     (if (string? dest)
         (apply fancy-format (cons #f (cons dest . rest)))
         (apply fancy-format (cons dest . rest))))

(define-public (ergonomic-simple-format dest . rest)
   "Like ice-9 format, but without the memory consumption."
   (catch 'format
   (lambda () (if (string? dest)
       (apply simple-format (cons #f (cons dest rest)))
       (apply simple-format (cons dest rest))))
   (lambda (key dest . rest) (simple-format-handler dest . rest)))

(define format
   ergonomic-simple-format)

It works OK in guile REPL, I'll try another regression test run.

Cheers,

Ian

[Neil Puttock, 2010-08-01 11:50:13]

On 2010/08/01 00:11:44, Ian Hulin wrote:

> How did you guys track it down to this module?

Since Patrick had already reported the failure, I cheated by applying the patch
and compiling profile-property-access.ly. :)

> I only get from the
> regression test is this message (this is the tail of the log file).
> <snip>

You should find the failed file further up the log.  Here's what I get:

Processing ./snippet-map--4234561403288800728
Failed files: (a1/lily-624a475e.ly)
logfile lilypond-multi-run-0.log (exit 1):
3/lily-9bd4398b
Processing da/lily-cddc2009
Processing 7b/lily-66b6addb
Processing f2/lily-115c7a97
Processing 1f/lily-aee531b8
Processing af/lily-14cabdbc
Processing 69/lily-663ee4f1
Processing 3c/lily-cf6046fa
Processing a3/lily-71898ac9
Processing 2d/lily-ca1387bf
Processing 26/lily-79010cda
Processing 76/lily-3893c7bc
Processing 09/lily-799c5df9
Processing 3f/lily-d9e46c0b
Processing bf/lily-d81dd530
Processing 6e/lily-5044bcc8
Processing f7/lily-936b0942
Processing 98/lily-6a58999c
Processing f2/lily-18f644bf
Processing 54/lily-7f91ee15
Processing b5/lily-7a515599
Processing 99/lily-fd2ab7dd
Processing aa/lily-35c4032b
Processing b0/lily-ef53b86d
Processing ba/lily-fb56b7d4
Processing c7/lily-eafa763e
Processing 6b/lily-7194d224
Processing e5/lily-06b49319
Processing ee/lily-cd629890
Processing 45/lily-8d2b9d96
Processing 61/lily-e50cd566
Processing aa/lily-c55a6eaf
Processing d4/lily-51c86602
Processing c4/lily-11400a53
Processing 65/lily-ab78a8a7
Processing a8/lily-f8232be1
Processing 52/lily-23a75a14
error: Children (2 0) exited with errors.

The log file for the failed snippet is here:

out/lybook-testdb/a1/lily-624a475e.log

> Unless I misunderstood Han-Wen,that's why we were jerking around with
> %module-public-interface in the first place.

Sure, but we only want the public interface from (lily), otherwise all functions
defined in .scm are accessible anywhere.

For example, `get-outfile-name' in lily-library.scm isn't public, yet I can
access it from a .ly file:

#(display (defined? 'get-current-filename))

-> #t

#(display get-current-filename)

-> #<procedure get-current-filename (parser)>

Cheers,
Neil

[Patrick McCarty, 2010-08-01 20:49:22]

On Sun, Aug 1, 2010 at 1:03 PM, <pnorcks@gmail.com> wrote:
> On 2010/07/29 19:24:46, Neil Puttock wrote:
>>
>> + = scm_list_3 (ly_symbol2scm ("module-use!"), mod, used);
>>
>> This effectively exports all bindings, so all local defines are
>> now exported.
>>
>> In the case of `profile-property-access.ly', the format call
>> ends up using `ergonomic-simple-format' from lily.scm.
>
> Thanks for this observation, Neil.
>
> The reason why I suggested this change is to prevent a compile failure
> with Guile 1.9.  The backtrace is below.  Perhaps there is a bug in the
> `module-public-interface' procedure?  Not really sure how to test if
> this is the case though...

I think I found the problem.

With Guile 1.9, `module-public-interface' doesn't return an interface
for `the-scm-module', which we rely on:

  scheme@(guile-user)> (module-public-interface the-scm-module)
  $1 = #f
  scheme@(guile-user)>

I'll report this upstream.

Thanks,
Patrick

[Neil Puttock, 2010-08-01 21:02:31]

On 1 August 2010 21:49, Patrick McCarty <pnorcks@gmail.com> wrote:

> I think I found the problem.
>
> With Guile 1.9, `module-public-interface' doesn't return an interface
> for `the-scm-module', which we rely on:
>
>  scheme@(guile-user)> (module-public-interface the-scm-module)
>  $1 = #f
>  scheme@(guile-user)>
>
> I'll report this upstream.

OK.  I was just about to reply, since I've seen that error using 1.8
(trying to use `resolve-interface' instead of
module-public-interface).

Cheers,
Neil

[Patrick McCarty, 2010-08-01 21:37:32]

On Sun, Aug 1, 2010 at 2:02 PM, Neil Puttock <n.puttock@gmail.com> wrote:
> On 1 August 2010 21:49, Patrick McCarty <pnorcks@gmail.com> wrote:
>
>> I think I found the problem.
>>
>> With Guile 1.9, `module-public-interface' doesn't return an interface
>> for `the-scm-module', which we rely on:
>>
>>  scheme@(guile-user)> (module-public-interface the-scm-module)
>>  $1 = #f
>>  scheme@(guile-user)>
>>
>> I'll report this upstream.
>
> OK.  I was just about to reply, since I've seen that error using 1.8
> (trying to use `resolve-interface' instead of
> module-public-interface).

This is quite interesting.  :)

I just did a little more research, and these three calls all return
the same interface, but the last one doesn't work with Guile 1.9:

  guile> (resolve-interface '(guile))
  #<interface (guile) 7f3f91117840>
  guile> (module-public-interface the-root-module)
  #<interface (guile) 7f3f91117840>
  guile> (module-public-interface the-scm-module)
  #<interface (guile) 7f3f91117840>
  guile>

So, it looks like we might want to do this:

-      SCM scm_module = ly_lily_module_constant ("the-scm-module");
+      SCM scm_module = ly_lily_module_constant ("the-root-module");


Thanks,
Patrick

[Ian Hulin, 2010-08-04 23:13:59]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all,

On 01/08/10 22:37, Patrick McCarty wrote:
> On Sun, Aug 1, 2010 at 2:02 PM, Neil Puttock <n.puttock@gmail.com> wrote:
>> On 1 August 2010 21:49, Patrick McCarty <pnorcks@gmail.com> wrote:
>>
>>> I think I found the problem.
>>>
>>> With Guile 1.9, `module-public-interface' doesn't return an interface
>>> for `the-scm-module', which we rely on:
>>>
>>>  scheme@(guile-user)> (module-public-interface the-scm-module)
>>>  $1 = #f
>>>  scheme@(guile-user)>
>>>
>>> I'll report this upstream.
>>
>> OK.  I was just about to reply, since I've seen that error using 1.8
>> (trying to use `resolve-interface' instead of
>> module-public-interface).
> 
> This is quite interesting.  :)
> 
> I just did a little more research, and these three calls all return
> the same interface, but the last one doesn't work with Guile 1.9:
> 
>   guile> (resolve-interface '(guile))
>   #<interface (guile) 7f3f91117840>
>   guile> (module-public-interface the-root-module)
>   #<interface (guile) 7f3f91117840>
>   guile> (module-public-interface the-scm-module)
>   #<interface (guile) 7f3f91117840>
>   guile>
> 
> So, it looks like we might want to do this:
> 
> -      SCM scm_module = ly_lily_module_constant ("the-scm-module");
> +      SCM scm_module = ly_lily_module_constant ("the-root-module");
> 
> 
I'll ensure this change is in the next patch set for T1055.

À propos of which, I've just hacked things so it gets through the
regression tests without the call using %module-public-interface woot! :-).

However, to do this I've had to change one regression test and one other
.scm file used in initialization to add
(use modules (scm clip-region)).

It looks like the reason is that we have been using a side-effect of the
call to %module-public-interface to pick up a call to the above
use-modules in init.ly.  It looks like the side-effect was pick up not
only the public interface (all the (define-public) declarations etc. of
the current module, but also all the public interfaces on the uses list
for the current module [i.e. our sole and only (scm clip-region)]. This
allowed us to code as if all the public scheme declarations in
clip-region.scm had been declared in lily.scm - but they weren't.

I have hacked two .scm files input/regression/clip-systems.ly and
scm/define-grob-properties.scm to include the calls.

The downside of this is the change to the regression test.  The upside
is that this module is being used as a module where it is needed.

Another possibility is that we forget using the module, and hurl all the
code from  clip-region.scm into a section of lily.scm. This feels messy
to me, and Han-Wen collected a consistent related set of procedures and
predicates in the guile module.

Another possibility is add a scm_c_use_module ("scm clip-region") call
to ly_make_module e.g.

  if (!safe)
    {
      /* Look up (evaluate) Scheme make-module function and call it */

      SCM maker = ly_lily_module_constant("make-module");
      mod = scm_call_0 (maker);
      /* Look up and call Guile module-export-all! or Lilypond-defined
         compatible version when using Guile V1.8 */
      SCM module_export_all_x = ly_lily_module_constant
("module-export-all!");
      scm_call_1 (module_export_all_x, mod);
      scm_c_use_module ("scm clip-region")
//    ************************************
      /* Evaluate Guile module "the-root-module",
         and ensure we inherit definitions from it and the "lily" module
         N.B. this used to be "the-scm-module" and is deprecated in
         Guile V1.9/2.0
         */
      SCM scm_module = ly_lily_module_constant ("the-root-module");
      ly_use_module (mod, scm_module);
      ly_use_module (mod, global_lily_module);
    }
  else
    {
      /* Evaluate and call make-safe-lilypond-module */
      SCM proc = ly_lily_module_constant ("make-safe-lilypond-module");
      mod = scm_call_0 (proc);
    }
But this seems like untidy special-casing and we'd need a more extensive
and consistent mechanism in case we wanted to declare other modules
which were globally available to the lily code base, perhaps using an a
scheme list or alist.  Nice idea, but I think the subject of a separate
task and tracker if anyone thinks it's worth doing.

I'd like to go ahead with the hack I've got and I'll upload this for
review.  Please respond if you think this is a bad idea.

Cheers,

Ian



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJMWfQjAAoJEBqidDirZqASVLgH/13QZ7SG3quS3N2iifVzxRwd
RHPoPRPTURbTIMi7JoOgu/9BZWgPLZWw6qShJy1oN28Ds5+ASYAunzgML6993LHG
RDSpSlgs5oyXcOoM+JgNvqJvi5DWmYrUN324kb/XYBpaxcJSkU7T6lrXV8HOjZSY
qmnnFsGtVfo234RVIZKGsZYUfPXLW16dDEfvO+BJVX5xZyx9sh3fpeDq6Zx8PmRC
cNy91eesidxcvcyqYI2hmR/YKH402PpA8x65wTEAkSq7YQn1dggwsmDxKIqSUXV+
6HIX9pwkbZWuC7OW55f9Mx8yB2MNNAGJRfpiVzjpsx4JvDwJvRHxCBvgSaBUVJE=
=63Ti
-----END PGP SIGNATURE-----

[hanwenn, 2010-08-05 00:16:23]

On Wed, Aug 4, 2010 at 8:13 PM, Ian Hulin <ian@hulin.org.uk> wrote:
> On 01/08/10 22:37, Patrick McCarty wrote:
>> On Sun, Aug 1, 2010 at 2:02 PM, Neil Puttock <n.puttock@gmail.com> wrote:
>>> On 1 August 2010 21:49, Patrick McCarty <pnorcks@gmail.com> wrote:
>>>
>>>> I think I found the problem.
>>>>
>>>> With Guile 1.9, `module-public-interface' doesn't return an interface
>>>> for `the-scm-module', which we rely on:
>>>>
>>>>  scheme@(guile-user)> (module-public-interface the-scm-module)
>>>>  $1 = #f
>>>>  scheme@(guile-user)>
>>>>
>>>> I'll report this upstream.
>>>
>>> OK.  I was just about to reply, since I've seen that error using 1.8
>>> (trying to use `resolve-interface' instead of
>>> module-public-interface).
>>
>> This is quite interesting.  :)
>>
>> I just did a little more research, and these three calls all return
>> the same interface, but the last one doesn't work with Guile 1.9:
>>
>>   guile> (resolve-interface '(guile))
>>   #<interface (guile) 7f3f91117840>
>>   guile> (module-public-interface the-root-module)
>>   #<interface (guile) 7f3f91117840>
>>   guile> (module-public-interface the-scm-module)
>>   #<interface (guile) 7f3f91117840>
>>   guile>
>>
>> So, it looks like we might want to do this:
>>
>> -      SCM scm_module = ly_lily_module_constant ("the-scm-module");
>> +      SCM scm_module = ly_lily_module_constant ("the-root-module");
>>
>>
> I'll ensure this change is in the next patch set for T1055.
>
> À propos of which, I've just hacked things so it gets through the
> regression tests without the call using %module-public-interface woot! :-).
>
> However, to do this I've had to change one regression test and one other
> .scm file used in initialization to add
> (use modules (scm clip-region)).
>
> It looks like the reason is that we have been using a side-effect of the
> call to %module-public-interface to pick up a call to the above
> use-modules in init.ly.  It looks like the side-effect was pick up not
> only the public interface (all the (define-public) declarations etc. of
> the current module, but also all the public interfaces on the uses list
> for the current module [i.e. our sole and only (scm clip-region)]. This
> allowed us to code as if all the public scheme declarations in
> clip-region.scm had been declared in lily.scm - but they weren't.
>
> I have hacked two .scm files input/regression/clip-systems.ly and
> scm/define-grob-properties.scm to include the calls.
>
> The downside of this is the change to the regression test.  The upside
> is that this module is being used as a module where it is needed.

Your approach sounds good to me.  The regression test is not holy, we
can change it if there are good reasons.

Congrats on making this work. I had a todo item in the back of my mind
to resolve this, but you got me to it! :)

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

[Ian Hulin, 2010-08-05 11:32:08]

Hi all,
An Updated patch set is available for review.

It's undergone a regression test run here. 

Sorry for the whitespace noise - git-cl upload doesn't have a -b option like git
format-patch.

Cheers,

Ian

[Patrick McCarty, 2010-08-09 18:35:53]

http://www.codereview.appspot.com/1160044/diff/51001/52005
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode215
scm/lily.scm:215: (apply fancy-format (cons dest . rest))))
`cons' only works with two concrete arguments, like

  (cons dest rest)

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode219
scm/lily.scm:219: (catch 'format
Is this change supposed to be part of the patchset?

Also, I don't understand the logic here.  Is 'format ever thrown?  If so, which
procedure does this?

[Neil Puttock, 2010-08-09 19:08:57]

Hi Ian,

Can you remove the tab->space changes you've made (particularly in lily.scm)? 
Some of them don't improve the indentation.

Cheers,
Neil

http://www.codereview.appspot.com/1160044/diff/51001/52001
File input/regression/clip-systems.ly (right):

http://www.codereview.appspot.com/1160044/diff/51001/52001#newcode47
input/regression/clip-systems.ly:47: #(use-modules (scm clip-region))
This is fine for a regression test, but not very user-friendly (don't forget
you'd also have to add it to the docs snippet from LSR, otherwise a docs build
will fail.)

Rather than make clipping systems even more esoteric, I'd be happier moving the
rhythmic-location code out of clip-region.scm and into output-lib.scm or
music-functions.scm.

http://www.codereview.appspot.com/1160044/diff/51001/52002
File lily/lily-lexer.cc (right):

http://www.codereview.appspot.com/1160044/diff/51001/52002#newcode303
lily/lily-lexer.cc:303: scm_c_export (symstr.c_str(), NULL);
Can you give an example justifying this addition?

I can't think of any identifier set in a .ly file which would need this.

http://www.codereview.appspot.com/1160044/diff/51001/52003
File lily/ly-module.cc (right):

http://www.codereview.appspot.com/1160044/diff/51001/52003#newcode36
lily/ly-module.cc:36: SCM maker = ly_lily_module_constant("make-module");
ly_lily_module_constant (

http://www.codereview.appspot.com/1160044/diff/51001/52003#newcode46
lily/ly-module.cc:46: Guile V1.9/2.0
Is it?

http://www.codereview.appspot.com/1160044/diff/51001/52004
File scm/define-grob-properties.scm (right):

http://www.codereview.appspot.com/1160044/diff/51001/52004#newcode19
scm/define-grob-properties.scm:19: (use-modules (scm clip-region))
Why not leave this in lily.scm?

http://www.codereview.appspot.com/1160044/diff/51001/52005
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode219
scm/lily.scm:219: (catch 'format
On 2010/08/09 18:35:54, Patrick McCarty wrote:
> Is this change supposed to be part of the patchset?

I don't think it should be. :)
 
> Also, I don't understand the logic here.  Is 'format ever thrown?  If so,
which
> procedure does this?

There's no exception for 'format, so it's never thrown.  I think this would only
work if it were changed to 

(catch #t

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode223
scm/lily.scm:223: (lambda (key dest . rest) (simple-format-handler dest .
rest))))
Should be

(lambda (key . args) (apply simple-format-handler (cons dest rest)))

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode258
scm/lily.scm:258: (define-public  (guile-v2 )
(define-public (guile-v2)

[Patrick McCarty, 2010-08-09 19:15:44]

http://www.codereview.appspot.com/1160044/diff/51001/52003
File lily/ly-module.cc (right):

http://www.codereview.appspot.com/1160044/diff/51001/52003#newcode46
lily/ly-module.cc:46: Guile V1.9/2.0
On 2010/08/09 19:08:58, Neil Puttock wrote:
> Is it?

I don't know.  But I did file a bug report regarding this issue:

http://savannah.gnu.org/bugs/?30623

[Neil Puttock, 2010-08-09 19:20:50]

On 2010/08/09 19:15:44, Patrick McCarty wrote:

> I don't know.  But I did file a bug report regarding this issue:
> 
> http://savannah.gnu.org/bugs/?30623

Heh, that's why I asked, since nobody's replied yet. :)

[Ian Hulin, 2010-08-09 20:01:10]

http://www.codereview.appspot.com/1160044/diff/51001/52001
File input/regression/clip-systems.ly (right):

http://www.codereview.appspot.com/1160044/diff/51001/52001#newcode47
input/regression/clip-systems.ly:47: #(use-modules (scm clip-region))
On 2010/08/09 19:08:58, Neil Puttock wrote:
> This is fine for a regression test, but not very user-friendly (don't forget
> you'd also have to add it to the docs snippet from LSR, otherwise a docs build
> will fail.)
> 
Presumably this means make a parallel change to
Documentation/snippets/clip-systems.ly? OK.
> Rather than make clipping systems even more esoteric, I'd be happier moving
the
> rhythmic-location code out of clip-region.scm and into output-lib.scm or
> music-functions.scm.

Valid point, but I'd rather do the work for clip-region as a separate tracker
item. Raise the tracker and I'll start work on it as soon as this one's
finished.

http://www.codereview.appspot.com/1160044/diff/51001/52002
File lily/lily-lexer.cc (right):

http://www.codereview.appspot.com/1160044/diff/51001/52002#newcode303
lily/lily-lexer.cc:303: scm_c_export (symstr.c_str(), NULL);
On 2010/08/09 19:08:58, Neil Puttock wrote:
> Can you give an example justifying this addition?
> 
> I can't think of any identifier set in a .ly file which would need this.
It was an answer from Han-Wen about how we'd got into using
%module-public-interface in the first place:

"I think %public-interface hack is to force all of the definitions including
future ones to be public;  the code that executes the assignments just does

      scm_module_define (mod, sym, val);  -- lily-lexer.cc

without doing anything to export sym."

http://www.codereview.appspot.com/1160044/diff/51001/52003
File lily/ly-module.cc (right):

http://www.codereview.appspot.com/1160044/diff/51001/52003#newcode36
lily/ly-module.cc:36: SCM maker = ly_lily_module_constant("make-module");
On 2010/08/09 19:08:58, Neil Puttock wrote:
> ly_lily_module_constant (

Done.

http://www.codereview.appspot.com/1160044/diff/51001/52003#newcode46
lily/ly-module.cc:46: Guile V1.9/2.0
On 2010/08/09 19:08:58, Neil Puttock wrote:
> Is it?

If it isn't, they forgot to forward-port "the-scm-module" as a synonym for
"the-root-module".  
It's easier for us change our code base to avoid the problem than wait for guile
to change theirs.

http://www.codereview.appspot.com/1160044/diff/51001/52004
File scm/define-grob-properties.scm (right):

http://www.codereview.appspot.com/1160044/diff/51001/52004#newcode19
scm/define-grob-properties.scm:19: (use-modules (scm clip-region))
On 2010/08/09 19:08:58, Neil Puttock wrote:
> Why not leave this in lily.scm?

Because it doesn't work.  
It used to pick up the definition in init.ly as a beneficial side-effect of
using %module-public-interface in ly_make_module when defining scopes.  
This change makes things cleaner all round because only things needing this
module use it and reference it specifically.
The other alternative was to dump all the definitions from the module into
lily.scm, Han-Wen said this had been on his to-do list for a while. and favoured
this approach.

http://www.codereview.appspot.com/1160044/diff/51001/52005
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode215
scm/lily.scm:215: (apply fancy-format (cons dest . rest))))
On 2010/08/09 18:35:54, Patrick McCarty wrote:
> `cons' only works with two concrete arguments, like
> 
>   (cons dest rest)
Generally it will get two arguments, but I was modeling this on the
ergonomic-simple-format procedure.
See below for the reasons for doing this, but I don't think it will get here if
rest isn't defined, as 'format only gets thrown if there's a formatting string
to have a directive in.

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode219
scm/lily.scm:219: (catch 'format
On 2010/08/09 18:35:54, Patrick McCarty wrote:
> Is this change supposed to be part of the patchset?
> 
> Also, I don't understand the logic here.  Is 'format ever thrown?  If so,
which
> procedure does this?
Yes,  If we don't have this handler, scm/define-grob-properties barfs during
initialization.  Procedure simple-format only handles a subset of format
directives, and throws 'format if it sees one it can't handle.  It kills the
build and the regtests if we don't handle it here. The other option is to change
define-grob-properties to use fancy-format (a.k.a the definition from module
(ice-9 format)).

[Neil Puttock, 2010-08-09 21:34:55]

On 2010/08/09 20:01:10, Ian Hulin wrote:

> Presumably this means make a parallel change to
> Documentation/snippets/clip-systems.ly? OK.

snippets/new/clip-systems.ly

then run makelsr.py to update snippets/clip-systems.ly:

scripts/auxiliar/makelsr.py

> Valid point, but I'd rather do the work for clip-region as a separate tracker
> item. Raise the tracker and I'll start work on it as soon as this one's
> finished.

Why would you want to add something to a file, only to remove it soon after?

If we agree it's better from the user's perspective not to require

(use-modules (scm clip-region))

in a .ly file, then it doesn't make sense to defer the change.

> It was an answer from Han-Wen about how we'd got into using
> %module-public-interface in the first place:
> 
> "I think %public-interface hack is to force all of the definitions including
> future ones to be public;  the code that executes the assignments just does
> 
>       scm_module_define (mod, sym, val);  -- lily-lexer.cc
> 
> without doing anything to export sym."

Sure, but `mod' here is created by ly_make_module, which now uses
module-export-all!, so all defines will automatically be public.
 
> Because it doesn't work.

I meant: leave it in lily.scm, even if you still want to use (use-modules (scm
clip-region)) in clip-systems.ly.

> Yes,  If we don't have this handler, scm/define-grob-properties barfs during
> initialization.  Procedure simple-format only handles a subset of format
> directives, and throws 'format if it sees one it can't handle.  It kills the
> build and the regtests if we don't handle it here. The other option is to
change
> define-grob-properties to use fancy-format (a.k.a the definition from module
> (ice-9 format)).

I don't get any regtest failure without this code.  There's no format call
inside define-grob-properties.scm, and as I said earlier, the handler doesn't
work since there's no exception thrown specifically for `format' (+ Patrick's
comments and the incorrect args to the handler).

Try adding this to profile-property-access.ly:

#(define format ergonomic-simple-format)

You'll see the exception handler doesn't catch this.  If you change the code, it
gets caught:

(define (simple-format-handler dest . rest)
  (if (string? dest)
      (apply fancy-format (cons #f (cons dest rest)))
      (apply fancy-format (cons dest rest))))

(define-public (ergonomic-simple-format dest . rest)
  "Like ice-9 format, but without the memory consumption."
  (catch #t
	 (lambda () (if (string? dest)
			(apply simple-format (cons #f (cons dest rest)))
			(apply simple-format (cons dest rest))))
	 (lambda (key . args) (apply simple-format-handler (cons dest rest)))))

[Ian Hulin, 2010-08-10 19:00:01]

Hi Neil,

On 09/08/10 22:34, n.puttock@gmail.com wrote:
> On 2010/08/09 20:01:10, Ian Hulin wrote:
>
>> Presumably this means make a parallel change to
>> Documentation/snippets/clip-systems.ly? OK.
>
> snippets/new/clip-systems.ly
>
> then run makelsr.py to update snippets/clip-systems.ly:
>
> scripts/auxiliar/makelsr.py
>

OK, you've convinced me. Changing the reg test is just nasty.

>> Valid point, but I'd rather do the work for clip-region as a separate
> tracker
>> item. Raise the tracker and I'll start work on it as soon as this
> one's
>> finished.
>
> Why would you want to add something to a file, only to remove it soon
> after?
>
> If we agree it's better from the user's perspective not to require
>
> (use-modules (scm clip-region))
>
> in a .ly file, then it doesn't make sense to defer the change.

Well, it seemed like a distinct and separate task and like it was
getting this patch 'off-topic'.

>
>> It was an answer from Han-Wen about how we'd got into using
>> %module-public-interface in the first place:
>
>> "I think %public-interface hack is to force all of the definitions
> including
>> future ones to be public;  the code that executes the assignments just
> does
>
>>        scm_module_define (mod, sym, val);  -- lily-lexer.cc
>
>> without doing anything to export sym."
>
> Sure, but `mod' here is created by ly_make_module, which now uses
> module-export-all!, so all defines will automatically be public.
>
>> Because it doesn't work.
>
> I meant: leave it in lily.scm, even if you still want to use
> (use-modules (scm clip-region)) in clip-systems.ly.
>

OK.

>> Yes,  If we don't have this handler, scm/define-grob-properties barfs
> during
>> initialization.  Procedure simple-format only handles a subset of
> format
>> directives, and throws 'format if it sees one it can't handle.  It
> kills the
>> build and the regtests if we don't handle it here. The other option is
> to change
>> define-grob-properties to use fancy-format (a.k.a the definition from
> module
>> (ice-9 format)).
>
> I don't get any regtest failure without this code.

That's because I was talking rubbish and trying to answer your review in
a hurry.  I was confusing the crash with profile-property-access.ly
which I'd compiled separately as a test
.
>There's no format
> call inside define-grob-properties.scm, and as I said earlier, the
> handler doesn't work since there's no exception thrown specifically for
> `format' (+ Patrick's comments and the incorrect args to the handler).
>
> Try adding this to profile-property-access.ly:
>
> #(define format ergonomic-simple-format)
>
> You'll see the exception handler doesn't catch this.  If you change the
> code, it gets caught:

Errm, unless you're using an unpatched, vanilla lily.scm from git you
won't see it because the handler will already be defined in
ergonomic-simple-format.  Using the code as in the my patch certainly
stopped profile-property-access.ly from barfing here. I'll poke around
further with this.

>
> (define (simple-format-handler dest . rest)
>   (if (string? dest)
>       (apply fancy-format (cons #f (cons dest rest)))
>       (apply fancy-format (cons dest rest))))
>
> (define-public (ergonomic-simple-format dest . rest)
>   "Like ice-9 format, but without the memory consumption."
>   (catch #t
>      (lambda () (if (string? dest)
>             (apply simple-format (cons #f (cons dest rest)))
>             (apply simple-format (cons dest rest))))
>      (lambda (key . args) (apply simple-format-handler (cons dest rest)))))
>
See above.
> http://www.codereview.appspot.com/1160044/show

Cheers,
Ian

[Neil Puttock, 2010-08-10 21:24:48]

On 2010/08/10 19:00:01, Ian Hulin wrote:

> Errm, unless you're using an unpatched, vanilla lily.scm from git you
> won't see it because the handler will already be defined in
> ergonomic-simple-format.

It doesn't make any difference for me.  profile-property-access.ly uses (ice-9
format) unless it's rebound locally (as I did above for testing).  The only case
where it was already using ergonomic-simple-format arose from the removal of
module-public-interface (which you've restored in the latest patch.)

To reiterate: with your patch applied, if I redefine format to use your emended
ergonomic-simple-format, the exception handler doesn't work, and
profile-property-access.ly fails in the same way as Patrick originally reported.
 It works with the changes I suggested.

[Ian Hulin, 2010-08-11 09:27:41]

http://www.codereview.appspot.com/1160044/diff/51001/52005
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode215
scm/lily.scm:215: (apply fancy-format (cons dest . rest))))
On 2010/08/09 18:35:54, Patrick McCarty wrote:
> `cons' only works with two concrete arguments, like
> 
>   (cons dest rest)

Done.

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode219
scm/lily.scm:219: (catch 'format
On 2010/08/09 19:08:58, Neil Puttock wrote:
> On 2010/08/09 18:35:54, Patrick McCarty wrote:
> > Is this change supposed to be part of the patchset?
> 
> I don't think it should be. :)
>  
> > Also, I don't understand the logic here.  Is 'format ever thrown?  If so,
> which
> > procedure does this?
> 
> There's no exception for 'format, so it's never thrown.  I think this would
only
> work if it were changed to 
> 
> (catch #t

Done.

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode223
scm/lily.scm:223: (lambda (key dest . rest) (simple-format-handler dest .
rest))))
On 2010/08/09 19:08:58, Neil Puttock wrote:
> Should be
> 
> (lambda (key . args) (apply simple-format-handler (cons dest rest)))

Done.

http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode258
scm/lily.scm:258: (define-public  (guile-v2 )
On 2010/08/09 19:08:58, Neil Puttock wrote:
> (define-public (guile-v2)

Done.

[Ian Hulin (gmail), 2010-08-17 22:18:34]

Even though this is title T1055: this change is now being tracked as Issue 1224.

Ian Hulin

[Ian Hulin, 2010-08-28 19:16:18]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all,

This was the artist formerly known as tracker 1055.

Status so far.
The patch runs OK against guile V1.8,7.  I've now been able to run
against Guile 1.9.11 on a V M despite having to rebuild my machine
because the CPU ran hot and toasted itself.

Anyhow, building against V1.9.11 showed up Neil's concerns with the code
in lily.scm/simple-format-handler and /ergonomic-simple-format and I
have a fix for this.  It looks like the byte-compiler in Guile V1.9.11
is stricter than the interpreter in V1.8.7.

Anyhow, running up Lily with Guile V1.9.11 now barfs as follows in
when attempting to compile scm/display-lily.scm
the trace-back shows it doesn't like this code starting at
(define ((make-music (see below for error messages)

;;;
;;; music type predicate maker
;;;

(define (make-music-type-predicate . music-types)
  (define ((make-music-type-predicate-aux mtypes) expr)
    (if (null? mtypes)
	#f
	(or (eqv? (car mtypes) (ly:music-property expr 'name))
	    ((make-music-type-predicate-aux (cdr mtypes)) expr))))
  (make-music-type-predicate-aux music-types))

I'm a bit puzzled by this code.  Even if we move the nested definition
out from within make-music-predicate, how does it work?  Variable
make-music-type-predicate is not declared anywhere else, and yet here we
appear to ask guile to resolve and call it before it is declared and
bound to anything?

Trying this in guile V1.8.7 repl with a dummy for ly:music-property and
it will work as written, but in V1.9.11 it always fails with
ERROR: in procedure macro-expand:
ERROR: source expression failed to match any pattern in (define
((make-music-type-predicate-aux mtypes) expr)  (if (null? mtypes) #f (or
(eqv? (car mtypes) (ly:music-property expr 'name))
((make-music-type-predicate-aux (cdr mtypes)) expr))))

V1.9.11 throws the error whether or not the repl is called with
- --no-auto-compile.  It also doesn't help to try and un-nest the inner
(define and declare it at top-level.

If anyone can throw any light on why this procedure is written this way
and any work-rounds or fixes this would be good. Nicolas, your name
seems to be on the source module, can you shed any light?

Patrick and Neil, do I have to fix all the compatibility problems in all
the scm files loaded by lily.scm in order to push what we have so far?

I now seem to be finding compatibility problems not directly related to
replacing %module-public-interface, and I'd rather tackle these as
smaller, more discrete items of work.

Cheers,

Ian Hulin






-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJMeWBsAAoJEBqidDirZqASHmgIAL+9hEYrtv3T/HMqgM+V1iem
xZYFJYTXTEPMDtOqwgtI9J4A2mYZtn0az0T98Zqp5JZWR05JhxgAZ9iI7HWW91K/
4fuaREV8MnnKMcfy/inwOc/oZBnC8GVYkhQFRwuW48qBLvSgIt95Ds8ApyQdrJGK
M1p0Ygw34gHGNr/I4l8jU7rrKKP7VxCkkwX1knbm8Zo1ALFMbxW4dNuKOUZhAKmo
ofXU1bXSnsHx6iv45Y1vlFW49TjVP27BElGrufYcH8Oaj4qU9QqP1wRtw5O2OTQw
zrYw8aYOMUOUptg/b5Mlhf/4Ty9lWLYjCP4OcUrdBeqve3Ue/B6J4neEzM98A2g=
=7S/4
-----END PGP SIGNATURE-----

[Neil Puttock, 2010-08-28 19:36:27]

On 2010/08/28 19:16:18, Ian Hulin wrote:

> Anyhow, building against V1.9.11 showed up Neil's concerns with the code
> in lily.scm/simple-format-handler and /ergonomic-simple-format and I
> have a fix for this.  It looks like the byte-compiler in Guile V1.9.11
> is stricter than the interpreter in V1.8.7.

I still don't understand why you want to add the exception handler for `format'.
 It's not required in 1.8.7 if you're only exporting the public interface, and
you haven't justified its inclusion for 1.9.
 
>   (define ((make-music-type-predicate-aux mtypes) expr)

This curried expression won't work in 1.9 without using the

(ice-9 curried-definitions)

module.

> Patrick and Neil, do I have to fix all the compatibility problems in all
> the scm files loaded by lily.scm in order to push what we have so far?

No, though I'd like you to do the following before we can commit this patch:

-) remove the exception handler

-) remove this line from lily-lexer.cc:

+ scm_c_export (symstr.c_str(), NULL);

-) remove this line from define-grob-properties.scm:

+(use-modules (scm clip-region))

Cheers,
Neil

[Patrick McCarty, 2010-08-29 01:53:29]

On 2010/08/28 19:16:18, Ian Hulin wrote:
>
> Patrick and Neil, do I have to fix all the compatibility
> problems in all the scm files loaded by lily.scm in order
> to push what we have so far?

No.  Some of the other compatibility problems are quite complicated and deserve
their own separate issues.

Please see my `guile' branch for hints about what needs to happen in order for
the SCM code base to successfully compile against Guile 1.9:

http://repo.or.cz/w/lilypond/patrick.git/shortlog/refs/heads/guile

Keep in mind that these may not be proper "backward-compatible" fixes, but they
do work with Guile 1.9.  I do not have the time at the moment to investigate
proper fixes for these issues.

Thanks,
Patrick

[hanwenn, 2010-08-30 02:31:36]

On Sat, Aug 28, 2010 at 4:16 PM, Ian Hulin <ian@hulin.org.uk> wrote:
> (define (make-music-type-predicate . music-types)
>  (define ((make-music-type-predicate-aux mtypes) expr)
>    (if (null? mtypes)
>        #f
>        (or (eqv? (car mtypes) (ly:music-property expr 'name))
>            ((make-music-type-predicate-aux (cdr mtypes)) expr))))
>  (make-music-type-predicate-aux music-types))
>
> I'm a bit puzzled by this code.  Even if we move the nested definition
> out from within make-music-predicate, how does it work?  Variable
> make-music-type-predicate is not declared anywhere else, and yet here we
> appear to ask guile to resolve and call it before it is declared and
> bound to anything?

are you sure? I only see calls to the -aux helper.

> Trying this in guile V1.8.7 repl with a dummy for ly:music-property and
> it will work as written, but in V1.9.11 it always fails with
> ERROR: in procedure macro-expand:
> ERROR: source expression failed to match any pattern in (define
> ((make-music-type-predicate-aux mtypes) expr)  (if (null? mtypes) #f (or
> (eqv? (car mtypes) (ly:music-property expr 'name))
> ((make-music-type-predicate-aux (cdr mtypes)) expr))))
>
> V1.9.11 throws the error whether or not the repl is called with
> - --no-auto-compile.  It also doesn't help to try and un-nest the inner
> (define and declare it at top-level.

Where is the latest version of this patch?  I downloaded a version
from rietveld which required the following patch to get to
display-lily.scm

commit 57992da984aaf16d6161dc44e5d9f7cb290ac813
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Sun Aug 29 23:30:55 2010 -0300

    Remove stray dots.

diff --git a/scm/lily.scm b/scm/lily.scm
index 0aa2ad4..f898962 100644
--- a/scm/lily.scm
+++ b/scm/lily.scm
@@ -213,9 +213,9 @@ messages into errors.")
   format)

 (define (simple-format-handler dest . rest)
-    (if (string? dest)
-        (apply fancy-format (cons #f (cons dest . rest)))
-        (apply fancy-format (cons dest . rest))))
+  (if (string? dest)
+      (apply fancy-format (cons #f (cons dest rest)))
+      (apply fancy-format (cons dest rest))))

> Patrick and Neil, do I have to fix all the compatibility problems in all
> the scm files loaded by lily.scm in order to push what we have so far?

As long as the compile for 1.8 keeps working, I don't think it's a
problem incrementally tackling 1.9  compatibility problems.


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

[Ian Hulin (gmail), 2010-08-31 17:31:49]

Have new patch-set ready,
Message describing this patch set: Can handler and stray dots from lily.scm,
remove lily-lexer & define-grob-properties from patch,
but git-cl doesn't think I own this issue any more :-(.

Closing this issue and opening a new one with correct tracker reference.

Cheers,
Ian

[Ian Hulin, 2010-08-31 17:54:44]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Han-Wen and all,
I can't upload the patch-set to Rietveld, and I seem to have lost
ownership of the issue on Reitveld, so I can't close it and open a new
issue either.

On 30/08/10 03:31, Han-Wen Nienhuys wrote:
> On Sat, Aug 28, 2010 at 4:16 PM, Ian Hulin <ian@hulin.org.uk> wrote:
>> (define (make-music-type-predicate . music-types)
>>  (define ((make-music-type-predicate-aux mtypes) expr)
>>    (if (null? mtypes)
>>        #f
>>        (or (eqv? (car mtypes) (ly:music-property expr 'name))
>>            ((make-music-type-predicate-aux (cdr mtypes)) expr))))
>>  (make-music-type-predicate-aux music-types))
>>
>> I'm a bit puzzled by this code.  Even if we move the nested definition
>> out from within make-music-predicate, how does it work?  Variable
>> make-music-type-predicate is not declared anywhere else, and yet here we
>> appear to ask guile to resolve and call it before it is declared and
>> bound to anything?
> 
> are you sure? I only see calls to the -aux helper.
> 
I'd forgotten about currying functions. Neil picked up about the need to
use a new module in 1.9.  I've found a way of conditionally using it if
we're running with guile V1.9+.

>> Trying this in guile V1.8.7 repl with a dummy for ly:music-property and
>> it will work as written, but in V1.9.11 it always fails with
>> ERROR: in procedure macro-expand:
>> ERROR: source expression failed to match any pattern in (define
>> ((make-music-type-predicate-aux mtypes) expr)  (if (null? mtypes) #f (or
>> (eqv? (car mtypes) (ly:music-property expr 'name))
>> ((make-music-type-predicate-aux (cdr mtypes)) expr))))
>>
>> V1.9.11 throws the error whether or not the repl is called with
>> - --no-auto-compile.  It also doesn't help to try and un-nest the inner
>> (define and declare it at top-level.
> 
> Where is the latest version of this patch?  I downloaded a version
> from rietveld which required the following patch to get to
> display-lily.scm
> 
Stray dots removed in attached path - along with handler in
ergonomic-simple-format.
> commit 57992da984aaf16d6161dc44e5d9f7cb290ac813
> Author: Han-Wen Nienhuys <hanwen@lilypond.org>
> Date:   Sun Aug 29 23:30:55 2010 -0300
> 
>     Remove stray dots.
> 
> diff --git a/scm/lily.scm b/scm/lily.scm
> index 0aa2ad4..f898962 100644
> --- a/scm/lily.scm
> +++ b/scm/lily.scm
> @@ -213,9 +213,9 @@ messages into errors.")
>    format)
> 
>  (define (simple-format-handler dest . rest)
> -    (if (string? dest)
> -        (apply fancy-format (cons #f (cons dest . rest)))
> -        (apply fancy-format (cons dest . rest))))
> +  (if (string? dest)
> +      (apply fancy-format (cons #f (cons dest rest)))
> +      (apply fancy-format (cons dest rest))))
> 

>> Patrick and Neil, do I have to fix all the compatibility problems in all
>> the scm files loaded by lily.scm in order to push what we have so far?
> 
> As long as the compile for 1.8 keeps working, I don't think it's a
> problem incrementally tackling 1.9  compatibility problems.
> 
Cheers,
Ian



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJMfUHYAAoJEBqidDirZqASIM8IAIgvhpveVxuj4OpHEom4xSpX
xDe2zhrn575cC8dfg876RMET9Swa54q/6EAG2AxH9nSEynJOS5VEFMCzRrwkisfS
fFrpmguluydqZ6FDjc4q/Ia1h0GGiH0CHWSMRow2WhVILMQtBKBnQBhRkYfU265X
ZwTNINt4/alFnjhnO9oJv9qnbQbLWL15g6hwzRvOLBbrOVONeVMl3TQF/tbt00Po
DPngHhizsqguA7YkG8fgNaylo/5MbHphG+yLzkkkzm97I8DSlpvKhkfuGbSZU7p3
ILzp/yQjEZ7gqyuZY6uSyc8sbwSQN0HHq7zrBEchUI3fa8y51MvTvudKWy27c3U=
=cnFp
-----END PGP SIGNATURE-----