Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Neil Puttock
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





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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
Hi Patrick, Neil and all,

On 29/07/10 20:24, [hidden email] 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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Neil Puttock
In reply to this post by Neil Puttock
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

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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Patrick McCarty
In reply to this post by Neil Puttock
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...


Processing
`/home/pnorcks/git/lilypond/ly/generate-documentation.ly'/home/pnorcks/git/lilypond/out/share/lilypond/current/scm/lily.scmBacktrace:
In ice-9/boot-9.scm:
  170: 14 [catch #t #<catch-closure 22a71e0> ...]
In unknown file:
    ?: 13 [catch-closure]
In /home/pnorcks/git/lilypond/scm/lily.scm:
  752: 12 [lilypond-main #<variable 294e830 value: #>]
  776: 11 [lilypond-all
("/home/pnorcks/git/lilypond/ly/generate-documentation")]
In unknown file:
    ?: 10 [for-each #<procedure 2bc2080 at
/home/pnorcks/git/lilypond/scm/lily.scm:776:5 (x)> ...]
In /home/pnorcks/git/lilypond/scm/lily.scm:
  789: 9 [#<procedure 2bc2080 at
/home/pnorcks/git/lilypond/scm/lily.scm:776:5 (x)>
"/home/pnorcks/git/lilypond/ly/generate-documentation"]
In ice-9/boot-9.scm:
  170: 8 [catch ly-file-failed ...]
In unknown file:
    ?: 7 [ly:parse-file
"/home/pnorcks/git/lilypond/ly/generate-documentation"]
In ice-9/boot-9.scm:
1949: 6 [module-use! #<module (#{\ g446}#) 2deac60> #<interface (lily)
22b6ea0>]
In unknown file:
    ?: 5 [filter #<procedure 2a330e0 at ice-9/boot-9.scm:1948:42 (m)>
({#f})]
In ice-9/boot-9.scm:
1951: 4 [#<procedure 2a330e0 at ice-9/boot-9.scm:1948:42 (m)> {#f}]
2158: 3 [#<procedure 214c980 at ice-9/boot-9.scm:2156:4 (mod)> {#f}]
  759: 2 [#<procedure 20f1930 at ice-9/boot-9.scm:757:4 (obj)> {#f}]
  115: 1 [#<procedure 22508c0 at ice-9/boot-9.scm:109:6 (thrown-k .
args)> wrong-type-arg ...]
In unknown file:
    ?: 0 [catch-closure wrong-type-arg "struct_vtable" ...]

ERROR: In procedure struct_vtable:
ERROR: Wrong type argument in position 1 (expecting struct): #f


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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Patrick McCarty
On Sun, Aug 1, 2010 at 1:03 PM, <[hidden email]> 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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Neil Puttock
On 1 August 2010 21:49, Patrick McCarty <[hidden email]> 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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Patrick McCarty
On Sun, Aug 1, 2010 at 2:02 PM, Neil Puttock <[hidden email]> wrote:

> On 1 August 2010 21:49, Patrick McCarty <[hidden email]> 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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
-----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 <[hidden email]> wrote:
>> On 1 August 2010 21:49, Patrick McCarty <[hidden email]> 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-----

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
In reply to this post by Neil Puttock
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

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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Patrick McCarty
In reply to this post by Neil Puttock

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?

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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Neil Puttock
In reply to this post by Neil Puttock
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)

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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Patrick McCarty
In reply to this post by Neil Puttock

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

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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Neil Puttock
In reply to this post by Neil Puttock
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. :)


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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
In reply to this post by Neil Puttock

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)).

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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Neil Puttock
In reply to this post by Neil Puttock
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)))))

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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
Hi Neil,

On 09/08/10 22:34, [hidden email] 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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Neil Puttock
In reply to this post by Neil Puttock
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.

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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
In reply to this post by Neil Puttock

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.

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

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1224: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
In reply to this post by Ian Hulin
-----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-----

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: T1224: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
-----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 <[hidden email]> 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 <[hidden email]>
> 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-----

0001-T1224-Avoid-using-deprecated-module-public-interface.patch (11K) Download Attachment