Re: request review: branch "wingo"

[Neil Jerram <neil@ossau.uklinux.net>]

Andy Wingo <wingo@pobox.com> writes:

> Hey all,
>
> On Fri 27 Mar 2009 16:29, Andy Wingo <wingo@pobox.com> writes:
>
>> On the "wingo" branch in the main repository, you will find the
>> following patches:
>
> So, I really intended to wait for review, but it's irritating having
> `master' broken, so I went ahead and merged this in.

Here are my comments on the merged commits.

- allow building against uninstalled guile; move some things to meta/

Looks like a good idea.  Could be some gotchas lurking, but I don't
immediately see any.

- add getrlimit and setrlimit wrappers

Should scm_getrlimit code be surrounded by #ifdef HAVE_GETRLIMIT ?

What's the idea of the numerical args?  Should RLIMIT_XXX be defined
somewhere as Scheme-level integers?

Does it make sense to want to set the soft limit only?  I thought some
hard limits were only settable by root, so I would suspect yes.  If
yes, does the setrlimit implementation support this?

- getrlimit-based stack limits
- rely on getrlimit to DTRT, don't make stack calibration file

My inclination is that we should revert these; but I'm not sure, so
could be persuaded that I'm wrong.

My feeling is that what we had before is a stronger statement (than
using getrlimit) about the expected behaviour of typical Scheme
programs (and in particular those used during the build), and is
therefore worth keeping.  The getrlimit implementation just says "if a
program is running away, we'll generate a Scheme exception a little
bit before you'd otherwise get a core".

But maybe this isn't important enough to justify the hassle around
stack-limit calibration.  What do others think?

- fix check for guile-tools running uninstalled
- fix distcheck hopefully, by cleaning the vm-i-*.i files

Fine.

- fix "linking" of guile-config

I don't understand the problem here.  In what way was @bindir@ not
fully expanded?

- bugfix: don't dynamic link if we found a registered extension

Would be great to have a test for this, if feasible.

> I think the stack calibration stuff is correct, but perhaps more jarring
> in this commit is a move from ./pre-inst-guile to ./meta/guile, and
> ./pre-inst-guile-env to ./meta/uninstalled-env. I describe the rationale
> in 0b6d8fdc28ed8af56e93157179c305fef037e0a0. But then again, given that
> Neil invested so much time into the stack calibration stuff, that might
> be jarring too.

As above - the meta/ stuff looks fine to me, but I'm not sure about
the stack limit change.

Regards,
        Neil