T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2,

[Ian Hulin (gmail), 2010-09-24 23:29:07]

Patch available for review.

Cheers,
Ian Hulin

[Carl, 2010-09-25 15:57:09]

I like the code for choosing Guile V2.

I'm concerned about a bunch of the re-indentation changes.

Thanks,

Carl

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

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode197
scm/lily.scm:197: ;(set-debug-cell-accesses! 1000)
Why is this shoved over to the right?

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode199
scm/lily.scm:199: ;;; Boolean thunk - are we integrating Guile V2.0 or higher
with Lilypond?
It should either be LilyPond (the application) or lilypond (the executable).

I don't think it should ever be Lilypond.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode204
scm/lily.scm:204: (ice-9 safe)
These should all be aligned with (ice-9 regex) as in the old code.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode300
scm/lily.scm:300: (vector-ref (uname) 0) char-set:letter+digit)))
(vector-ref should be indented; it's an argument to string-tokenize.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode337
scm/lily.scm:337: iface))
Looks like there are trailing spaces here?

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode568
scm/lily.scm:568: r5rs-secondary-predicates
Why the move from spaces to tabs?  As far as I know, the tabs-vs-spaces
discussion is not settled.  Given that it's not, I don't think we should be
changing existing code when that's the only change.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode829
scm/lily.scm:829: (ly:stderr-redirect (format "~a.log" base) "w"))
The previous indentation here was correct.  Then clause should be aligned with
if clause

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode831
scm/lily.scm:831: (format ping-log "Processing ~a\n" base))
Same here

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode848
scm/lily.scm:848: (ly:reset-all-fonts))))
Else clause should be aligned with if clause.

[Ian Hulin (gmail), 2010-09-25 21:27:06]

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

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode197
scm/lily.scm:197: ;(set-debug-cell-accesses! 1000)
On 2010/09/25 15:57:09, Carl wrote:
> Why is this shoved over to the right?
Reverted
Done.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode199
scm/lily.scm:199: ;;; Boolean thunk - are we integrating Guile V2.0 or higher
with Lilypond?
On 2010/09/25 15:57:09, Carl wrote:
> It should either be LilyPond (the application) or lilypond (the executable).
> 
> I don't think it should ever be Lilypond.

Done.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode199
scm/lily.scm:199: ;;; Boolean thunk - are we integrating Guile V2.0 or higher
with Lilypond?
On 2010/09/25 15:57:09, Carl wrote:
> It should either be LilyPond (the application) or lilypond (the executable).
> 
> I don't think it should ever be Lilypond.

Done.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode204
scm/lily.scm:204: (ice-9 safe)
On 2010/09/25 15:57:09, Carl wrote:
> These should all be aligned with (ice-9 regex) as in the old code.

Done.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode300
scm/lily.scm:300: (vector-ref (uname) 0) char-set:letter+digit)))
On 2010/09/25 15:57:09, Carl wrote:
> (vector-ref should be indented; it's an argument to string-tokenize.

Done.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode337
scm/lily.scm:337: iface))
On 2010/09/25 15:57:09, Carl wrote:
> Looks like there are trailing spaces here?

Couldn't see any in the buffer in the editor, have got it set to suppress
trailing spaces.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode568
scm/lily.scm:568: r5rs-secondary-predicates
On 2010/09/25 15:57:09, Carl wrote:
> Why the move from spaces to tabs?  As far as I know, the tabs-vs-spaces
> discussion is not settled.  Given that it's not, I don't think we should be
> changing existing code when that's the only change.

git gui showed this as having changed and pushed way over to the right.  Using
the tabs got it back to looking visually correct versus the way git gui said I'd
changed it.

Will manually revert this to spaces.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode829
scm/lily.scm:829: (ly:stderr-redirect (format "~a.log" base) "w"))
On 2010/09/25 15:57:09, Carl wrote:
> The previous indentation here was correct.  Then clause should be aligned with
> if clause

Done.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode831
scm/lily.scm:831: (format ping-log "Processing ~a\n" base))
On 2010/09/25 15:57:09, Carl wrote:
> Same here

Done.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode848
scm/lily.scm:848: (ly:reset-all-fonts))))
On 2010/09/25 15:57:09, Carl wrote:
> Else clause should be aligned with if clause.
> 
> 

Done.

[Neil Puttock, 2010-09-25 21:45:21]

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

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode226
scm/lily.scm:226: (ly:progress  (_ "Using (ice-9 curried-definitions)
module\n"))
  (if (ly:get-option 'verbose)
      (ly:message (_ "Using (ice-9 curried-definitions) module")))
  (use-modules (ice-9 curried-definitions)))

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode231
scm/lily.scm:231: (_ "module (ice-9 curried-definitions) not in Guile
1.8\n")))))
I don't think ths message is necessary.

[Ian Hulin (gmail), 2010-09-25 23:39:33]

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

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode226
scm/lily.scm:226: (ly:progress  (_ "Using (ice-9 curried-definitions)
module\n"))
On 2010/09/25 21:45:21, Neil Puttock wrote:
>   (if (ly:get-option 'verbose)
>       (ly:message (_ "Using (ice-9 curried-definitions) module")))
>   (use-modules (ice-9 curried-definitions)))
Looking at warn.cc I can see ly:progress and ly:message are synonymous. Does
this mean there's a standard we should use ly:message for all informational (non
error/warning) messages? Is it documented?
I'll change to use ly:message.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode231
scm/lily.scm:231: (_ "module (ice-9 curried-definitions) not in Guile
1.8\n")))))
On 2010/09/25 21:45:21, Neil Puttock wrote:
> I don't think ths message is necessary.

I felt some indication was useful at run-time that we weren't trying to use the
V2-specific modules when running with --verbose using Guile V1.8.

[Neil Puttock, 2010-09-26 00:00:27]

On 2010/09/25 23:39:33, ianhulin44 wrote:

> Looking at warn.cc I can see ly:progress and ly:message are synonymous.

ly:message always starts on a new line.

ly:progress calls progress_indication () directly, which has this comment above
it:

> I felt some indication was useful at run-time that we weren't trying to use
the
> V2-specific modules when running with --verbose using Guile V1.8.

Why is that useful?  It's irrelevant for anybody using 1.8.

Cheers,
Neil

[Ian Hulin (gmail), 2010-09-26 10:19:59]

On 2010/09/26 00:00:27, Neil Puttock wrote:
> On 2010/09/25 23:39:33, ianhulin44 wrote:
> 
> > Looking at warn.cc I can see ly:progress and ly:message are synonymous.
> 
> ly:message always starts on a new line.
> 
> ly:progress calls progress_indication () directly, which has this comment
above
> it:
> 
> > I felt some indication was useful at run-time that we weren't trying to use
> the
> > V2-specific modules when running with --verbose using Guile V1.8.
> 
> Why is that useful?  It's irrelevant for anybody using 1.8.
> 
> Cheers,
> Neil

--verbose messages are used for development and maintenance purposes as well as
for advanced user hacking.
E.g. messages are output for each file loaded by lily,scm when it's run up
during initialization.
Files loaded like this are dynamically byte-compiled by Guile V2.
Guile V2 also dynamically compiles any files accessed via (use-modules(...))
both by lily.scm and any .scm files loaded by lily.scm: it's a PITA keeping
track of all this.
This patch adds a new module solely for use by Guile V2.0.  I have work coming
up (T1249) which implies we will need to use modules under V1.8 which will be
part of the Guile V2.0 base code, i.e. we will need to conditionally load them
when running with Guile V1.8 and not V2 (e.g. (ice-9 syncase) so we can fix a V2
compilation problem in ly-syntax-contructors.scm).
At the moment, I have a use for such messages while working on the Guile
migration stuff.  If you feel they are a serious nuisance for users running V1.8
and using --verbose by all means open an issue and I'll take them out once all
the issues spawned by T1055 are fixed and LilyPond is running up without
problems using both Guile V1.8 and V2.
Cheers,
Ian

[Patrick McCarty, 2010-09-30 21:15:21]

On 2010/09/26 10:19:59, ianhulin44 wrote:
> On 2010/09/26 00:00:27, Neil Puttock wrote:
> > 
> > Why is that useful?  It's irrelevant for anybody using 1.8.
> 
> --verbose messages are used for development and maintenance purposes as well
as
> for advanced user hacking.

Yes.

> E.g. messages are output for each file loaded by lily,scm when it's run up
> during initialization.
> Files loaded like this are dynamically byte-compiled by Guile V2.

Are they?  Last time I tested this, Guile only compiled modules loaded via
(use-modules ...).

I had to change `primitive-load' to `primitive-load-path' (in ly:load) to enable
automatic compilation.

> Guile V2 also dynamically compiles any files accessed via (use-modules(...))
> both by lily.scm and any .scm files loaded by lily.scm: it's a PITA keeping
> track of all this.

Yes, but only while you debug this particular issue (see below)...

> This patch adds a new module solely for use by Guile V2.0.  I have work coming
> up (T1249) which implies we will need to use modules under V1.8 which will be
> part of the Guile V2.0 base code, i.e. we will need to conditionally load them
> when running with Guile V1.8 and not V2 (e.g. (ice-9 syncase) so we can fix a
V2
> compilation problem in ly-syntax-contructors.scm).
> At the moment, I have a use for such messages while working on the Guile
> migration stuff.  If you feel they are a serious nuisance for users running
V1.8
> and using --verbose by all means open an issue and I'll take them out once all
> the issues spawned by T1055 are fixed and LilyPond is running up without
> problems using both Guile V1.8 and V2.

Okay, I agree that things are getting complicated quite fast...

But will there really be a use for this ly:message call *after* your patch is
committed to git?

-Patrick

[Ian Hulin (gmail), 2010-10-20 20:54:04]

New patchset uploaded

Ian

http://codereview.appspot.com/2219044/diff/10001/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/2219044/diff/10001/scm/lily.scm#newcode231
scm/lily.scm:231: (_ "module (ice-9 curried-definitions) not in Guile
1.8\n")))))
Change --verbose ly:message to say just "Guile V1.8"

http://codereview.appspot.com/2219044/diff/10001/scm/lily.scm#newcode293
scm/lily.scm:293: (primitive-load file-name)
(primitive-load-path file-name)
;; for Guile V2.0 autocompile

[Ian Hulin (gmail), 2010-10-20 21:42:06]

OK to remove offending line even when using Guile V1.8.7?

Ian

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode271
scm/lily.scm:271: (debug-enable 'debug)
This causes a deprecation warning from Guile V1.9.13 with
$ declare -x GUILE_WARN_DEPRECATED="detailed" 

[/home/ian/lilypond/out/share/lilypond/current/scm/lily.scm]
`(debug-enable 'debug)' is obsolete and has no effect.
Remove it from your code.

[Patrick McCarty, 2010-10-20 21:58:06]

Hi Ian,

I will test your patch shortly.

Thanks,
Patrick

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode231
scm/lily.scm:231: (_ "Guile 1.8\n")))))
Okay, I can live with this.  :)

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode271
scm/lily.scm:271: (debug-enable 'debug)
On 2010/10/20 21:42:06, ianhulin44 wrote:
> This causes a deprecation warning from Guile V1.9.13 with
> $ declare -x GUILE_WARN_DEPRECATED="detailed" 
> 
> [/home/ian/lilypond/out/share/lilypond/current/scm/lily.scm]
> `(debug-enable 'debug)' is obsolete and has no effect.
> Remove it from your code.

Go ahead.  There is another instance of `(debug-enable 'debug)' earlier in this
file, so you can remove that too.

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode332
scm/lily.scm:332: (set-module-name! iface (module-name mod))
Please change this so that it uses a tab again.

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode338
scm/lily.scm:338: (fresh-interface!))))
This line too.

[Patrick McCarty, 2010-10-21 00:12:21]

Hi Ian,

I just tested your patch.

In addition to the small tweak that is needed (see my comment below), it seems
that the `(use-modules (ice-9 curried-definitions))' statement does not carry
over to display-lily.scm.  I am a bit puzzled by this.

This is the error message, in context:

;;; compiling /home/pnorcks/usr/share/lilypond/2.13.37/scm/music-functions.scm
;;; compiling /home/pnorcks/usr/share/lilypond/2.13.37/scm/display-lily.scm
;;; WARNING: compilation of
/home/pnorcks/usr/share/lilypond/2.13.37/scm/display-lily.scm failed:
;;; key syntax-error, throw args (macroexpand "~a in ~a" ("source expression
failed to match any pattern" (define ((make-music-type-predicate-aux mtypes)
expr) (if (null? mtypes) #f (or (eqv? (car mtypes) (ly:music-property expr
(quote name))) ((make-music-type-predicate-aux (cdr mtypes)) expr))))) #f)
;;; WARNING: compilation of
/home/pnorcks/usr/share/lilypond/2.13.37/scm/music-functions.scm failed:
;;; key wrong-type-arg, throw args (#f "Wrong type to apply: ~S" (#f) (#f))


Fortunately, it seems that the Scheme interpreter in Guile 1.9 is used as a
fallback when compilation fails, since this doesn't interpret the make process.

Can you reproduce this with Guile 1.9.13?

Thanks,
Patrick

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode227
scm/lily.scm:227: (use-modules (ice-9 curried-definitions))))
In this section, the parenthesis nesting needs some adjustment.

It should be

  ((guile-v2)
   (if (ly:get-option 'verbose)
       (ly:message  (_ "Using (ice-9 curried-definitions) module\n")))
   (use-modules (ice-9 curried-definitions)))

[Ian Hulin (gmail), 2010-10-21 20:11:55]

Hi Patrick,


On 21/10/10 01:12, pnorcks@gmail.com wrote:
> Hi Ian,
>
> I just tested your patch.
>
> In addition to the small tweak that is needed (see my comment below), it
> seems that the `(use-modules (ice-9 curried-definitions))' statement
> does not carry over to display-lily.scm.  I am a bit puzzled by this.
>
> This is the error message, in context:
>
> ;;; compiling
> /home/pnorcks/usr/share/lilypond/2.13.37/scm/music-functions.scm
> ;;; compiling
> /home/pnorcks/usr/share/lilypond/2.13.37/scm/display-lily.scm
> ;;; WARNING: compilation of
> /home/pnorcks/usr/share/lilypond/2.13.37/scm/display-lily.scm failed:
> ;;; key syntax-error, throw args (macroexpand "~a in ~a" ("source
> expression failed to match any pattern" (define
> ((make-music-type-predicate-aux mtypes) expr) (if (null? mtypes) #f (or
> (eqv? (car mtypes) (ly:music-property expr (quote name)))
> ((make-music-type-predicate-aux (cdr mtypes)) expr))))) #f)
> ;;; WARNING: compilation of
> /home/pnorcks/usr/share/lilypond/2.13.37/scm/music-functions.scm failed:
> ;;; key wrong-type-arg, throw args (#f "Wrong type to apply: ~S" (#f)
> (#f))
>
>
> Fortunately, it seems that the Scheme interpreter in Guile 1.9 is used
> as a fallback when compilation fails, since this doesn't interpret the
> make process.
>
> Can you reproduce this with Guile 1.9.13?
>

Rats!  I had a patched version of display-lily.scm with this as the star
of the module on my VM with guile 1.9 installed:

(define-module (scm display-lily)
  #:use-module (ice-9 optargs)
  #:use-module (ice-9 format)
  #:use-module (ice-9 regex)
  #:use-module (ice-9 pretty-print)
  #:use-module (srfi srfi-1)
  #:use-module (srfi srfi-13)
  #:use-module (srfi srfi-39)
  #:use-module (lily)
  #:use-syntax (srfi srfi-39)
  #:use-syntax (ice-9 optargs))

;;; (ice-9 curried-definitions) does not exist in Guile V1.8,7
;;;   TODO change this to
;;;   #:use-module (ice-9 curried-definitions)
;;;   once Guile V1.8 support is dropped so it's consistent

(if (string>? (version) "1.9.10")
    (use-modules (ice-9 curried-definitions)))

I'd obviously got to this bit and forgot to copy it back to my main
machine's environment and retest before my recent dose of flu. 

> Thanks,
> Patrick
>
>
> http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm
> File scm/lily.scm (right):
>
> http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode227
> scm/lily.scm:227: (use-modules (ice-9 curried-definitions))))
> In this section, the parenthesis nesting needs some adjustment.
>
> It should be
>
>   ((guile-v2)
>    (if (ly:get-option 'verbose)
>        (ly:message  (_ "Using (ice-9 curried-definitions) module\n")))
>    (use-modules (ice-9 curried-definitions)))
>
> http://codereview.appspot.com/2219044/
Nice catch, ta.

Cheers,

Ian

[Ian Hulin (gmail), 2010-10-27 09:01:32]

New patch-set uploads (well two actually, but please review the latest).

Code in display-lily.scm to support Guile V2 now tested on Guile 1.8.7 system.

Cheers,

Ian

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode227
scm/lily.scm:227: (use-modules (ice-9 curried-definitions))))
On 2010/10/21 00:12:21, Patrick McCarty wrote:
> In this section, the parenthesis nesting needs some adjustment.
> 
> It should be
> 
>   ((guile-v2)
>    (if (ly:get-option 'verbose)
>        (ly:message  (_ "Using (ice-9 curried-definitions) module\n")))
>    (use-modules (ice-9 curried-definitions)))

Done.

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode231
scm/lily.scm:231: (_ "Guile 1.8\n")))))
On 2010/10/20 21:58:07, Patrick McCarty wrote:
> Okay, I can live with this.  :)

Ta

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode271
scm/lily.scm:271: (debug-enable 'debug)
On 2010/10/20 21:58:07, Patrick McCarty wrote:
> On 2010/10/20 21:42:06, ianhulin44 wrote:
> > This causes a deprecation warning from Guile V1.9.13 with
> > $ declare -x GUILE_WARN_DEPRECATED="detailed" 
> > 
> > [/home/ian/lilypond/out/share/lilypond/current/scm/lily.scm]
> > `(debug-enable 'debug)' is obsolete and has no effect.
> > Remove it from your code.
> 
> Go ahead.  There is another instance of `(debug-enable 'debug)' earlier in
this
> file, so you can remove that too.

Done.

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode332
scm/lily.scm:332: (set-module-name! iface (module-name mod))
On 2010/10/20 21:58:07, Patrick McCarty wrote:
> Please change this so that it uses a tab again.

Done.

http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode338
scm/lily.scm:338: (fresh-interface!))))
On 2010/10/20 21:58:07, Patrick McCarty wrote:
> This line too.

Done.

[Patrick McCarty, 2010-11-02 21:53:54]

Hi Ian,

This patch still isn't working for me with Guile 1.9.13.  Guile 1.8 is fine.

I posted some inline comments for you below.

Unfortunately, I'm not sure what to suggest for a fix.  Could this be a bug in
Guile 1.9 ?

Thanks,
Patrick

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
File scm/display-lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode30
scm/display-lily.scm:30: #:use-module (srfi srfi-39)
If I say

  #:use-module (ice-9 curried-definitions)

this file compiles fine (with Guile 1.9.13).

Instead, see below for the error message.

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode41
scm/display-lily.scm:41: (use-modules (ice-9 curried-definitions)))
This doesn't work for me with Guile 1.9.13.

I see this in the log as the Scheme files are compiling:

;;; compiling /home/pnorcks/usr/share/lilypond/2.13.39/scm/music-functions.scm
;;; compiling /home/pnorcks/usr/share/lilypond/2.13.39/scm/display-lily.scm
;;; WARNING: compilation of
/home/pnorcks/usr/share/lilypond/2.13.39/scm/display-lily.scm failed:
;;; key syntax-error, throw args (macroexpand "~a in ~a" ("source expression
failed to match any pattern" (define ((make-music-type-predicate-aux mtypes)
expr) (if (null? mtypes) #f (or (eqv? (car mtypes) (ly:music-property expr
(quote name))) ((make-music-type-predicate-aux (cdr mtypes)) expr))))) #f)
;;; WARNING: compilation of
/home/pnorcks/usr/share/lilypond/2.13.39/scm/music-functions.scm failed:
;;; key wrong-type-arg, throw args (#f "Wrong type to apply: ~S" (#f) (#f))

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode308
scm/display-lily.scm:308: 
Remove the last line of this file, since it just has trailing whitespace.

[Ian Hulin (gmail), 2010-11-25 15:56:26]

Another thought re the conditional (define-module) idea, if it's (if) making the
guile compilation barf, we could try using (cond) or (cond-expand) instead.

Ian

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
File scm/display-lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode41
scm/display-lily.scm:41: (use-modules (ice-9 curried-definitions)))
On 2010/11/02 21:53:54, Patrick McCarty wrote:
> This doesn't work for me with Guile 1.9.13.
> 
> I see this in the log as the Scheme files are compiling:
> 
> ;;; compiling /home/pnorcks/usr/share/lilypond/2.13.39/scm/music-functions.scm
> ;;; compiling /home/pnorcks/usr/share/lilypond/2.13.39/scm/display-lily.scm
> ;;; WARNING: compilation of
> /home/pnorcks/usr/share/lilypond/2.13.39/scm/display-lily.scm failed:
> ;;; key syntax-error, throw args (macroexpand "~a in ~a" ("source expression
> failed to match any pattern" (define ((make-music-type-predicate-aux mtypes)
> expr) (if (null? mtypes) #f (or (eqv? (car mtypes) (ly:music-property expr
> (quote name))) ((make-music-type-predicate-aux (cdr mtypes)) expr))))) #f)
> ;;; WARNING: compilation of
> /home/pnorcks/usr/share/lilypond/2.13.39/scm/music-functions.scm failed:
> ;;; key wrong-type-arg, throw args (#f "Wrong type to apply: ~S" (#f) (#f))

Hmm... can you patch out the (if string>? .... block and implement the TODO, as
I had this working for Guile 1,9.12.
If this works we can do
(if (string<= (version)  "1.9.10")
(  *** current (define-module statement ***)
(  *** (define-module with additional #use-module (ice-9 curried-definitions) )
I know it's vile, but I think it will work.  I'll also try it at home later when
I have access to both guile 1.8.7 and 1.9.13.

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode308
scm/display-lily.scm:308: 
On 2010/11/02 21:53:54, Patrick McCarty wrote:
> Remove the last line of this file, since it just has trailing whitespace.

Will do

[Patrick McCarty, 2010-11-28 20:36:17]

Hi Ian,

On 2010/11/25 15:56:26, ianhulin44 wrote:
> Another thought re the conditional (define-module) idea, if it's
> (if) making the guile compilation barf, we could try using
> (cond) or (cond-expand) instead.

I just tried all of these options, and nothing seems to work.  I'm
pretty baffled.

In fact, the display-lily.scm module *loads* the top-level lily
module with

  #:use-module (lily)

so why isn't (ice-9 curried-definitions) picked up in the first
place?

> Hmm... can you patch out the (if string>? .... block and
> implement the TODO, as I had this working for Guile 1,9.12.

Yes, the only thing that works for me is to add

  #:use-module (ice-9 curried-definitions)

to the (define-module (scm display-lily) ...) block at the top of
display-lily.scm.

But, of course, this only works with Guile 1.9, since the
curried-definitions module doesn't exist in Guile 1.8.

---

Looking back at your email from the Frogs list (subject
"display-lily.scm question"), I tried some of those options too,
such as (a) adding display-lily.scm to the use-modules list in
lily.scm, (b) adding display-lily.scm to the ly:load list in
lily.scm, but those don't work either.

I'm at a loss, so I'll revisit this at a later time...

Any ideas? :)


Thanks,
Patrick

[Patrick McCarty, 2011-02-17 06:50:21]

Hi Ian,

Please see my new comment regarding this patch (below).

Thanks,
Patrick

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
File scm/display-lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode34
scm/display-lily.scm:34: 
Jan recently removed all of the curried definitions that were affected by this
conditional (use-modules ...) call.

I just compiled LilyPond (and the patch queue from my "guile" branch) against
Guile 2.0 *without* this part of your patch, and I didn't run into any issues.

In other words, you can safely remove this chunk from your patch.

[Ian Hulin (gmail), 2011-02-17 15:06:40]

Hi Patrick,
On 2011/02/17 06:50:21, Patrick McCarty wrote:
> Hi Ian,
> 
> Please see my new comment regarding this patch (below).
> 
> Thanks,
> Patrick
> 
> http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
> File scm/display-lily.scm (right):
> 
>
http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode34
> scm/display-lily.scm:34: 
> Jan recently removed all of the curried definitions that were affected by this
> conditional (use-modules ...) call.
> 
> I just compiled LilyPond (and the patch queue from my "guile" branch) against
> Guile 2.0 *without* this part of your patch, and I didn't run into any issues.
> 
> In other words, you can safely remove this chunk from your patch.

I can easily remove display-lily.scm from the patch if Jan's change does the
biz.  However, is the display-lily stuff the only currying we have in the Lily
Scheme files?  If so, do we need this patch at all as its sole purpose is to
enable currying when using V2.0?

See more detailed comments below.

Cheers,
Ian

[Ian Hulin (gmail), 2011-02-17 15:06:59]

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
File scm/display-lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode34
scm/display-lily.scm:34: 
On 2011/02/17 06:50:21, Patrick McCarty wrote:
> Jan recently removed all of the curried definitions that were affected by this
> conditional (use-modules ...) call.
> 
> I just compiled LilyPond (and the patch queue from my "guile" branch) against
> Guile 2.0 *without* this part of your patch, and I didn't run into any issues.
> 
> In other words, you can safely remove this chunk from your patch.

In which case, do we even need lily.scm to pull in (ice-9 curried-definitions)
at all if we aren't currying in our code?

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm#newcode222
scm/lily.scm:222: (cond
This block is the whole point of the patch and the tracker.
Jan has just re-written code in display-lily.scm so it doesn't curry.  If
there's no currying in Lily Scheme code do we need this, or should we defend
against users using currying their Scheme code?

Opinions please?

Ian

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm#newcode291
scm/lily.scm:291: (primitive-load-path file-name)  ;; to support Guile V2
autocompile
When we move to generating our own compiled Scheme files ly:load will need a
significant re-write. We will also need routines to do the compilation and some
extra  changes in the Guile initialisation code  This change makes no difference
using Guile V1.8 but is only temporary debug code until Tracker 1349 is fixed,
and the code to support compiling out scheme files to scm/out.

[Patrick McCarty, 2011-02-17 16:17:08]

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
File scm/display-lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode34
scm/display-lily.scm:34: 
On 2011/02/17 15:07:00, ianhulin44 wrote:
> In which case, do we even need lily.scm to pull in (ice-9 curried-definitions)
> at all if we aren't currying in our code?  

We are in musicxml2ly and a few snippets:

$ git grep -l '(define (('
Documentation/music-glossary.tely
Documentation/snippets/compound-time-signatures.ly
Documentation/snippets/heavily-customized-polymetric-time-signatures.ly
Documentation/snippets/new/compound-time-signatures.ly
scripts/musicxml2ly.py

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode308
scm/display-lily.scm:308: 
Keep this chunk, because we want to use ly:load (even if the semantics of
ly:load have to change).

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm#newcode222
scm/lily.scm:222: (cond
On 2011/02/17 15:07:00, ianhulin44 wrote:
> This block is the whole point of the patch and the tracker.
> Jan has just re-written code in display-lily.scm so it doesn't curry.  If
> there's no currying in Lily Scheme code do we need this, or should we defend
> against users using currying their Scheme code?
> 
> Opinions please?

See my comment above.  I think we should keep it.

Since we're still supporting Guile 1.8 and 2.0 simultaneously, and Guile 1.8
supports currying out of the box, IMO it would not be smart to start
discouraging it.

Once everyone is using Guile 2.0 and people realize it doesn't support currying
out of the box, then I think people will naturally stop using currying.  Even
then, I don't think we would need to actively discourage it.

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm#newcode291
scm/lily.scm:291: (primitive-load-path file-name)  ;; to support Guile V2
autocompile
On 2011/02/17 15:07:00, ianhulin44 wrote:
> When we move to generating our own compiled Scheme files ly:load will need a
> significant re-write. We will also need routines to do the compilation and
some
> extra  changes in the Guile initialisation code  This change makes no
difference
> using Guile V1.8 but is only temporary debug code until Tracker 1349 is fixed,
> and the code to support compiling out scheme files to scm/out.

Yes, but without this change, SCM files are not autocompiled.  Is this still the
case?

[Reinhold, 2011-02-17 17:05:25]

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
File scm/display-lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode34
scm/display-lily.scm:34: 

> We are in musicxml2ly and a few snippets:
> 
> $ git grep -l '(define (('

Is define-public relevant, too? There are lots of hits for "(define-public (("
in our scm/ directory...

[Ian Hulin (gmail), 2011-02-17 19:06:52]

OK all, I've worked out what to keep and what to nuke.

I'll prepare a new patch-set once I've rebased and tested with Guile V2 on my
VM.

Cheers,
Ian

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
File scm/display-lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode23
scm/display-lily.scm:23: (define-module (scm display-lily)
I will need to change the argument list here so we don't lose Jan's changes.

Ian

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode41
scm/display-lily.scm:41: (use-modules (ice-9 curried-definitions)))
On 2010/11/02 21:53:54, Patrick McCarty wrote:
> This doesn't work for me with Guile 1.9.13.
> 
> I see this in the log as the Scheme files are compiling:
> 
> ;;; compiling /home/pnorcks/usr/share/lilypond/2.13.39/scm/music-functions.scm
> ;;; compiling /home/pnorcks/usr/share/lilypond/2.13.39/scm/display-lily.scm
> ;;; WARNING: compilation of
> /home/pnorcks/usr/share/lilypond/2.13.39/scm/display-lily.scm failed:
> ;;; key syntax-error, throw args (macroexpand "~a in ~a" ("source expression
> failed to match any pattern" (define ((make-music-type-predicate-aux mtypes)
> expr) (if (null? mtypes) #f (or (eqv? (car mtypes) (ly:music-property expr
> (quote name))) ((make-music-type-predicate-aux (cdr mtypes)) expr))))) #f)
> ;;; WARNING: compilation of
> /home/pnorcks/usr/share/lilypond/2.13.39/scm/music-functions.scm failed:
> ;;; key wrong-type-arg, throw args (#f "Wrong type to apply: ~S" (#f) (#f))

Removing this change.

Ian

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm#newcode222
scm/lily.scm:222: (cond
On 2011/02/17 16:17:08, Patrick McCarty wrote:
> On 2011/02/17 15:07:00, ianhulin44 wrote:
> > This block is the whole point of the patch and the tracker.
> > Jan has just re-written code in display-lily.scm so it doesn't curry.  If
> > there's no currying in Lily Scheme code do we need this, or should we defend
> > against users using currying their Scheme code?
> > 
> > Opinions please?
> 
> See my comment above.  I think we should keep it.
> 
> Since we're still supporting Guile 1.8 and 2.0 simultaneously, and Guile 1.8
> supports currying out of the box, IMO it would not be smart to start
> discouraging it.
> 
> Once everyone is using Guile 2.0 and people realize it doesn't support
currying
> out of the box, then I think people will naturally stop using currying.  Even
> then, I don't think we would need to actively discourage it.

OK, lets keep it, then.

Cheers,
Ian

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm#newcode291
scm/lily.scm:291: (primitive-load-path file-name)  ;; to support Guile V2
autocompile
On 2011/02/17 16:17:08, Patrick McCarty wrote:
> On 2011/02/17 15:07:00, ianhulin44 wrote:
> > When we move to generating our own compiled Scheme files ly:load will need a
> > significant re-write. We will also need routines to do the compilation and
> some
> > extra  changes in the Guile initialisation code  This change makes no
> difference
> > using Guile V1.8 but is only temporary debug code until Tracker 1349 is
fixed,
> > and the code to support compiling out scheme files to scm/out.
> 
> Yes, but without this change, SCM files are not autocompiled.  Is this still
the
> case?

Yes, but autocompile is evil, until we work out how to fiddle things like
%compile-fallback-path and  
%load-compiled-path in our initialization code.  I can see how this is done in
compile-file in the guile code, but I'll need to dig around in their code some a
bit to find out how this affects load etc.

In the meantime, this will be a good debugging and diagnostic tool for what will
need compiling in which order, so let's keep it for the interim.

Ian

[Ian Hulin (gmail), 2011-02-17 21:08:05]

New patchset uploaded to Rietveld Issue 2219044.

Please review.

Cheers,

Ian

[Patrick McCarty, 2011-02-18 02:06:00]

On 2011/02/17 17:05:25, Reinhold wrote:
> http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
> File scm/display-lily.scm (right):
> 
>
http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode34
> scm/display-lily.scm:34: 
> 
> > We are in musicxml2ly and a few snippets:
> > 
> > $ git grep -l '(define (('
> 
> Is define-public relevant, too? There are lots of hits for "(define-public (("
> in our scm/ directory...

Yes, I thought of that too after sending my email.  :)

Thanks,
Patrick

[Patrick McCarty, 2011-02-18 02:13:25]

LGTM.

Can you email me your patch so I can apply it?

Thanks,
Patrick

[Ian Hulin (gmail), 2011-02-20 12:38:59]

Hi Patrick,

On 18/02/11 02:13, pnorcks@gmail.com wrote:
> LGTM.
>
> Can you email me your patch so I can apply it?
>
> Thanks,
> Patrick
>
> http://codereview.appspot.com/2219044/
Here's the patch.

Cheers,

Ian

[Patrick McCarty, 2011-02-21 07:31:24]

On Sun, Feb 20, 2011 at 4:38 AM, Ian Hulin <ianhulin44@gmail.com> wrote:
> Hi Patrick,
>
> On 18/02/11 02:13, pnorcks@gmail.com wrote:
>> LGTM.
>>
>> Can you email me your patch so I can apply it?
>
> Here's the patch.

I retested everything, and the patch checks out just fine.  I've pushed it.

Thanks,
Patrick