Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

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

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

David Kastrup

https://codereview.appspot.com/569390043/diff/551480043/aclocal.m4
File aclocal.m4 (right):

https://codereview.appspot.com/569390043/diff/551480043/aclocal.m4#newcode760
aclocal.m4:760: guile2.2-config guile-2.2-config guile-config-2.2
guile-config2.2 \
I think that at the current point of time, guile1.8-config should be
preferred over guile2.2-config.  The main advantage that Guile2 offers
currently is that it is more likely to be the installed version.  If
anybody goes to the pain these days of actually installing a Guile-1.8
development package, it is pretty safe to assume that it is for the sake
of compiling LilyPond.

Also, I cannot imagine guile-1.9-config to be recommendable for anything
(I mean, it is somewhere between 1.8 and 2.0) and would not look
explicitly for it.

Yes, this is old code, but when we touch it in order to update to
current realities, we should update to current realities.

Do we even have comparative speed and memory use measurements of Guile2
for make all test doc and some large scores?

https://codereview.appspot.com/569390043/diff/551480043/configure.ac
File configure.ac (right):

https://codereview.appspot.com/569390043/diff/551480043/configure.ac#newcode204
configure.ac:204: case "$GUILE_MAJOR_VERSION" in
How is GUILE_MAJOR_VERSION determined?  A cursory read of aclocal.m4 did
not really help.  Guile executable and the Guile development library
will not necessarily be the same version, particularly since more than
one Guile executable can be installed at the same time.

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

jonas.hahnfeld
guile-config since 2.0 has the following comment:
"This script has been deprecated. Just use pkg-config."

Mabye it makes sense to completely turn to pkg-config? My system has $
ll /usr/lib/pkgconfig/guile-*
-rw-r--r-- 1 root root 453 11. Jan 2019  /usr/lib/pkgconfig/guile-1.8.pc
-rw-r--r-- 1 root root 849 10. Jan 2019  /usr/lib/pkgconfig/guile-2.0.pc
-rw-r--r-- 1 root root 886 10. Jul 2019  /usr/lib/pkgconfig/guile-2.2.pc

So this would also work for Guile 1.8 (which should either be preferred
as David suggested or completely dropped; not sure if we're already
there).

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

Dev mailing list
In reply to this post by David Kastrup
> Mabye it makes sense to completely turn to pkg-config?

Sounds sensible.  In particular, it eases cross-compilation.


    Werner


https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

David Kastrup
In reply to this post by David Kastrup
On 2020/02/21 16:02:14, hahnjo wrote:
> guile-config since 2.0 has the following comment:
> "This script has been deprecated. Just use pkg-config."

It is not something it outputs but rather a script-internal comment.

> Mabye it makes sense to completely turn to pkg-config? My system has $
ll
> /usr/lib/pkgconfig/guile-*
> -rw-r--r-- 1 root root 453 11. Jan 2019
/usr/lib/pkgconfig/guile-1.8.pc
> -rw-r--r-- 1 root root 849 10. Jan 2019
/usr/lib/pkgconfig/guile-2.0.pc
> -rw-r--r-- 1 root root 886 10. Jul 2019
/usr/lib/pkgconfig/guile-2.2.pc
>
> So this would also work for Guile 1.8 (which should either be
preferred as David
> suggested or completely dropped; not sure if we're already there).

How does it work?  With GUILE_CONFIG, I pass the executable as an
environment variable.  Admittedly, my non-system-wide Guile-1.8 (Ubuntu
no longer has guile-1.8-dev) contains a file
/usr/local/tmp/guile-1.8/lib/pkgconfig/guile-1.8.pc but I don't have an
idea how to use it.

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

jonas.hahnfeld
In reply to this post by David Kastrup
On 2020/02/21 16:22:52, lemzwerg wrote:
> > Mabye it makes sense to completely turn to pkg-config?
>
> Sounds sensible.  In particular, it eases cross-compilation.

Reading my mind ;-)

On 2020/02/21 16:25:02, dak wrote:
> > So this would also work for Guile 1.8 (which should either be
preferred as
> David
> > suggested or completely dropped; not sure if we're already there).
>
> How does it work?  With GUILE_CONFIG, I pass the executable as an
environment
> variable.  Admittedly, my non-system-wide Guile-1.8 (Ubuntu no longer
has
> guile-1.8-dev) contains a file
> /usr/local/tmp/guile-1.8/lib/pkgconfig/guile-1.8.pc but I don't have
an idea how
> to use it.

 $ man pkg-config
ENVIRONMENT
     PKG_CONFIG_PATH
             List of secondary directories where ‘.pc’ files are looked
up.

     PKG_CONFIG_LIBDIR
             List of primary directories where ‘.pc’ files are looked
up.

You'd want PKG_CONFIG_PATH, in your case add
/usr/local/tmp/guile-1.8/lib/pkgconfig/. Setting PKG_CONFIG_LIBDIR makes
pkg-config exclude the system-wide directories like /usr/lib/pkgconfig
(which is very convenient for cross-compilation!).

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by David Kastrup
On 2020/02/21 16:02:14, hahnjo wrote:
> guile-config since 2.0 has the following comment:
> "This script has been deprecated. Just use pkg-config."
>
> Mabye it makes sense to completely turn to pkg-config? My system has $
ll
> /usr/lib/pkgconfig/guile-*
> -rw-r--r-- 1 root root 453 11. Jan 2019
/usr/lib/pkgconfig/guile-1.8.pc
> -rw-r--r-- 1 root root 849 10. Jan 2019
/usr/lib/pkgconfig/guile-2.0.pc
> -rw-r--r-- 1 root root 886 10. Jul 2019
/usr/lib/pkgconfig/guile-2.2.pc
>
> So this would also work for Guile 1.8 (which should either be
preferred as David
> suggested or completely dropped; not sure if we're already there).

this is an excellent idea; see the latest patchset.

Unfortunately,

[hanwen@localhost lilypond]$ make
Making lily/out/lilypond
/usr/bin/ld: ./out/accidental-engraver.o: undefined reference to symbol
'pthread_getspecific@@GLIBC_2.2.5'
/usr/bin/ld: /usr/lib64/libpthread.so.0: error adding symbols: DSO
missing from command line
collect2: error: ld returned 1 exit status
make[1]: *** [out/lilypond] Error 1
make: *** [all] Error 2
[hanwen@localhost lilypond]$ pkg-config --libs guile-1.8
-lguile


https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by David Kastrup
On 2020/02/21 16:36:57, hanwenn wrote:
> On 2020/02/21 16:02:14, hahnjo wrote:
> > guile-config since 2.0 has the following comment:
> > "This script has been deprecated. Just use pkg-config."
> >
> > Mabye it makes sense to completely turn to pkg-config? My system has
$ ll
> > /usr/lib/pkgconfig/guile-*
> > -rw-r--r-- 1 root root 453 11. Jan 2019
/usr/lib/pkgconfig/guile-1.8.pc
> > -rw-r--r-- 1 root root 849 10. Jan 2019
/usr/lib/pkgconfig/guile-2.0.pc
> > -rw-r--r-- 1 root root 886 10. Jul 2019
/usr/lib/pkgconfig/guile-2.2.pc
> >
> > So this would also work for Guile 1.8 (which should either be
preferred as

> David
> > suggested or completely dropped; not sure if we're already there).
>
> this is an excellent idea; see the latest patchset.
>
> Unfortunately,
>
> [hanwen@localhost lilypond]$ make
> Making lily/out/lilypond
> /usr/bin/ld: ./out/accidental-engraver.o: undefined reference to
symbol
> mailto:
> /usr/bin/ld: /usr/lib64/libpthread.so.0: error adding symbols: DSO
missing from
> command line
> collect2: error: ld returned 1 exit status
> make[1]: *** [out/lilypond] Error 1
> make: *** [all] Error 2
> [hanwen@localhost lilypond]$ pkg-config --libs guile-1.8
> -lguile

$ guile1.8-config link
 -pthread  -lguile


https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

jonas.hahnfeld
In reply to this post by David Kastrup
On 2020/02/21 16:36:57, hanwenn wrote:
> On 2020/02/21 16:02:14, hahnjo wrote:
> > guile-config since 2.0 has the following comment:
> > "This script has been deprecated. Just use pkg-config."
> >
> > Mabye it makes sense to completely turn to pkg-config? My system has
$ ll
> > /usr/lib/pkgconfig/guile-*
> > -rw-r--r-- 1 root root 453 11. Jan 2019
/usr/lib/pkgconfig/guile-1.8.pc
> > -rw-r--r-- 1 root root 849 10. Jan 2019
/usr/lib/pkgconfig/guile-2.0.pc
> > -rw-r--r-- 1 root root 886 10. Jul 2019
/usr/lib/pkgconfig/guile-2.2.pc
> >
> > So this would also work for Guile 1.8 (which should either be
preferred as

> David
> > suggested or completely dropped; not sure if we're already there).
>
> this is an excellent idea; see the latest patchset.
>
> Unfortunately,
>
> [hanwen@localhost lilypond]$ make
> Making lily/out/lilypond
> /usr/bin/ld: ./out/accidental-engraver.o: undefined reference to
symbol
> mailto:
> /usr/bin/ld: /usr/lib64/libpthread.so.0: error adding symbols: DSO
missing from
> command line
> collect2: error: ld returned 1 exit status
> make[1]: *** [out/lilypond] Error 1
> make: *** [all] Error 2
> [hanwen@localhost lilypond]$ pkg-config --libs guile-1.8
> -lguile

$ pkg-config --cflags guile-1.8
-D_FORTIFY_SOURCE=2 -pthread

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by David Kastrup
On 2020/02/21 15:43:38, dak wrote:
> https://codereview.appspot.com/569390043/diff/551480043/aclocal.m4
> File aclocal.m4 (right):
>
>
https://codereview.appspot.com/569390043/diff/551480043/aclocal.m4#newcode760
> aclocal.m4:760: guile2.2-config guile-2.2-config guile-config-2.2
> guile-config2.2 \
> I think that at the current point of time, guile1.8-config should be
preferred
> over guile2.2-config.  The main advantage that Guile2 offers currently
is that
> it is more likely to be the installed version.  If anybody goes to the
pain
> these days of actually installing a Guile-1.8 development package, it
is pretty
> safe to assume that it is for the sake of compiling LilyPond.
>
> Also, I cannot imagine guile-1.9-config to be recommendable for
anything (I
> mean, it is somewhere between 1.8 and 2.0) and would not look
explicitly for it.
>
> Yes, this is old code, but when we touch it in order to update to
current
> realities, we should update to current realities.
>
> Do we even have comparative speed and memory use measurements of
Guile2 for make
> all test doc and some large scores?

We did the carver score. See commit
a19aed147bf1605b21cbe7b1909ff6cbf519fb64. It's 52 sec with GUILE 2.2,
and 40 sec (if memory serves) with GUILE 1.8.

I'll check the cost of "make test-baseline"




https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

Dev mailing list
In reply to this post by David Kastrup
LGTM.  However, the use of the pkgconfig environment variable(s) should
be documented.

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by David Kastrup
make baseline:

1.8:
real 3m41.714s
user 6m52.414s
sys 0m36.662s

2.2:
real 6m8.344s
user 12m51.799s
sys 0m34.875s

I think this warrants some deeper look, though: I have the impression
that the lilypond-book regression tests aren't parallelized. They also
suffer from the higher start up costs (there are some 60 of them, so
that's 90 seconds right there).

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by David Kastrup
On 2020/02/21 16:39:20, hahnjo wrote:
> On 2020/02/21 16:36:57, hanwenn wrote:
> > On 2020/02/21 16:02:14, hahnjo wrote:
> > > guile-config since 2.0 has the following comment:
> > > "This script has been deprecated. Just use pkg-config."
> > >
> > > Mabye it makes sense to completely turn to pkg-config? My system
has $ ll
> > > /usr/lib/pkgconfig/guile-*
> > > -rw-r--r-- 1 root root 453 11. Jan 2019
/usr/lib/pkgconfig/guile-1.8.pc
> > > -rw-r--r-- 1 root root 849 10. Jan 2019
/usr/lib/pkgconfig/guile-2.0.pc
> > > -rw-r--r-- 1 root root 886 10. Jul 2019
/usr/lib/pkgconfig/guile-2.2.pc
> > >
> > > So this would also work for Guile 1.8 (which should either be
preferred as

> > David
> > > suggested or completely dropped; not sure if we're already there).
> >
> > this is an excellent idea; see the latest patchset.
> >
> > Unfortunately,
> >
> > [hanwen@localhost lilypond]$ make
> > Making lily/out/lilypond
> > /usr/bin/ld: ./out/accidental-engraver.o: undefined reference to
symbol
> > mailto:
> > /usr/bin/ld: /usr/lib64/libpthread.so.0: error adding symbols: DSO
missing

> from
> > command line
> > collect2: error: ld returned 1 exit status
> > make[1]: *** [out/lilypond] Error 1
> > make: *** [all] Error 2
> > [hanwen@localhost lilypond]$ pkg-config --libs guile-1.8
> > -lguile
>
> $ pkg-config --cflags guile-1.8
> -D_FORTIFY_SOURCE=2 -pthread

Done. (It's ugly to put CFLAGS into the LIBS variable, but it works)

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

David Kastrup
In reply to this post by David Kastrup
On 2020/02/21 17:03:07, hanwenn wrote:

> make baseline:
>
> 1.8:
> real 3m41.714s
> user 6m52.414s
> sys 0m36.662s
>
> 2.2:
> real 6m8.344s
> user 12m51.799s
> sys 0m34.875s
>
> I think this warrants some deeper look, though: I have the impression
that the
> lilypond-book regression tests aren't parallelized. They also suffer
from the
> higher start up costs (there are some 60 of them, so that's 90 seconds
right
> there).

I recently found some "could not contact jobserver" in the logs from
make and googling it suggested that this is a consequence of using
$(MAKE) rather than $$(MAKE) inside of macros (causing premature or too
much expansion?).  I have not investigated yet and don't know what
counts as "macro" here.  Or whether this is all nonsense.  Either way,
it should affect Guile-2 just as much as Guile-1.8, shouldn't it?

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
On Fri, Feb 21, 2020 at 6:13 PM <[hidden email]> wrote:

> > 1.8:
> > real  3m41.714s
> > user  6m52.414s
> > sys   0m36.662s
> >
> > 2.2:
> > real  6m8.344s
> > user  12m51.799s
> > sys   0m34.875s
> >
> > I think this warrants some deeper look, though: I have the impression
> that the
> > lilypond-book regression tests aren't parallelized. They also suffer
> from the
> > higher start up costs (there are some 60 of them, so that's 90 seconds
> right
> > there).
>
> I recently found some "could not contact jobserver" in the logs from
> make and googling it suggested that this is a consequence of using
> $(MAKE) rather than $$(MAKE) inside of macros (causing premature or too
> much expansion?).  I have not investigated yet and don't know what
> counts as "macro" here.  Or whether this is all nonsense.  Either way,
> it should affect Guile-2 just as much as Guile-1.8, shouldn't it?

The startup costs of parsing .scm files of 1.5 secs gets added to the
process 60 times. If you remove that,

>>> (368 - 90)  /221.
1.257918552036199

we're back to a 25% slowdown.


>
> https://codereview.appspot.com/569390043/



--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

jonas.hahnfeld
In reply to this post by David Kastrup
On 2020/02/21 17:08:34, hanwenn wrote:
> On 2020/02/21 16:39:20, hahnjo wrote:
> > $ pkg-config --cflags guile-1.8
> > -D_FORTIFY_SOURCE=2 -pthread
>
> Done. (It's ugly to put CFLAGS into the LIBS variable, but it works)

Uh, we don't add CFLAGS when linking? That sounds ... dangerous.

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

jonas.hahnfeld
In reply to this post by David Kastrup
A few nits on m4 escapes, but otherwise LGTM - works very well on my
system with three versions of Guile installed!


https://codereview.appspot.com/569390043/diff/571710044/aclocal.m4
File aclocal.m4 (right):

https://codereview.appspot.com/569390043/diff/571710044/aclocal.m4#newcode693
aclocal.m4:693: if [[ -n "$GUILE_FLAVOR" ]] ; then
I first thought this was bash syntax and not portable, but m4
substitution comes into play here and removes one set of [ ]. Could we
write this as
test -n "$GUILE_FLAVOR"
instead?

https://codereview.appspot.com/569390043/diff/571710044/aclocal.m4#newcode694
aclocal.m4:694: PKG_CHECK_MODULES(GUILE, $GUILE_FLAVOR, true, true)
Please escape all arguments with [ ... ], here and in other locations.

Additionally we should probably reset GUILE_FLAVOR in the false case to
trigger the error below.

https://codereview.appspot.com/569390043/diff/571710044/config.make.in
File config.make.in (right):

https://codereview.appspot.com/569390043/diff/571710044/config.make.in#newcode28
config.make.in:28: GUILE_LIBS = @GUILE_LIBS@ @GUILE_CFLAGS@
Can be $(GUILE_CFLAGS), already substituted above

https://codereview.appspot.com/569390043/diff/571710044/config.make.in#newcode129
config.make.in:129: GUILE_CONFIG = @GUILE_CONFIG@
Please remove these GUILE_CFLAGS (there's another declaration above) and
GUILE_CONFIG which should be unused now.

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

jonas.hahnfeld
In reply to this post by David Kastrup

https://codereview.appspot.com/569390043/diff/571710044/aclocal.m4
File aclocal.m4 (right):

https://codereview.appspot.com/569390043/diff/571710044/aclocal.m4#newcode697
aclocal.m4:697: if [[ -z "$GUILE_FLAVOR" ]] ; then
You can actually make this a little nicer by putting the next check in
the false branch.

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by David Kastrup
On 2020/02/21 17:31:40, hahnjo wrote:
> On 2020/02/21 17:08:34, hanwenn wrote:
> > On 2020/02/21 16:39:20, hahnjo wrote:
> > > $ pkg-config --cflags guile-1.8
> > > -D_FORTIFY_SOURCE=2 -pthread
> >
> > Done. (It's ugly to put CFLAGS into the LIBS variable, but it works)
>
> Uh, we don't add CFLAGS when linking? That sounds ... dangerous.

see stepmake/executable-rules.make

https://codereview.appspot.com/569390043/

Reply | Threaded
Open this post in threaded view
|

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by David Kastrup

https://codereview.appspot.com/569390043/diff/571710044/aclocal.m4
File aclocal.m4 (right):

https://codereview.appspot.com/569390043/diff/571710044/aclocal.m4#newcode693
aclocal.m4:693: if [[ -n "$GUILE_FLAVOR" ]] ; then
On 2020/02/21 17:42:00, hahnjo wrote:
> I first thought this was bash syntax and not portable, but m4
substitution comes
> into play here and removes one set of [ ]. Could we write this as
> test -n "$GUILE_FLAVOR"
> instead?

oh, of course

"[[" is recommended in the google bash style guide, for reasons that
have to do with (IIRC) wildcard expansion.

https://codereview.appspot.com/569390043/diff/571710044/aclocal.m4#newcode694
aclocal.m4:694: PKG_CHECK_MODULES(GUILE, $GUILE_FLAVOR, true, true)
On 2020/02/21 17:42:00, hahnjo wrote:
> Please escape all arguments with [ ... ], here and in other locations.
>
> Additionally we should probably reset GUILE_FLAVOR in the false case
to trigger
> the error below.

done. Note that we don't use [ ] in most pkg_check_modules() calls
elsewhere.

https://codereview.appspot.com/569390043/diff/571710044/aclocal.m4#newcode697
aclocal.m4:697: if [[ -z "$GUILE_FLAVOR" ]] ; then
On 2020/02/21 17:48:54, hahnjo wrote:
> You can actually make this a little nicer by putting the next check in
the false
> branch.

Done.

https://codereview.appspot.com/569390043/diff/571710044/aclocal.m4#newcode697
aclocal.m4:697: if [[ -z "$GUILE_FLAVOR" ]] ; then
On 2020/02/21 17:48:54, hahnjo wrote:
> You can actually make this a little nicer by putting the next check in
the false
> branch.

Done.

https://codereview.appspot.com/569390043/diff/571710044/config.make.in
File config.make.in (right):

https://codereview.appspot.com/569390043/diff/571710044/config.make.in#newcode28
config.make.in:28: GUILE_LIBS = @GUILE_LIBS@ @GUILE_CFLAGS@
On 2020/02/21 17:42:00, hahnjo wrote:
> Can be $(GUILE_CFLAGS), already substituted above

Done.

https://codereview.appspot.com/569390043/