Mailing-List: contact dev-help@lucy.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@lucy.apache.org
Received-SPF: neutral (nike.apache.org: local policy)
MIME-Version: 1.0
In-Reply-To: <4F7D8C9F.2030406@aevum.de>
References: 
 <CAAS6=7hDi=unksg3dAtkOe-tiZgS7T_oDH3g+qdhmojm1DXFEg@mail.gmail.com>
	<4F7757EF.4030705@aevum.de>
	<CAAS6=7hpSS-vDwFVY3sj5mKz9PwP5zwkjtR5yir+KgiXSAzKxA@mail.gmail.com>
	<4F7C9F07.8090500@aevum.de>
	<CAAS6=7i59sez5HMETEOnEM8qDt5o+F4Yof_SCqs1_E_ri-9YfA@mail.gmail.com>
	<4F7D8C9F.2030406@aevum.de>
Date: Thu, 5 Apr 2012 09:35:47 -0700
Message-ID: 
 <CAAS6=7gJ9f2mBPOP5bwSDNaT0r1C40eXzQd2=p=OhZfZVJ5mUA@mail.gmail.com>
From: Marvin Humphrey <marvin@rectangular.com>
To: dev@lucy.apache.org
Content-Type: text/plain; charset=ISO-8859-1
Subject: Re: [lucy-dev] Build support classes for C extensions

On Thu, Apr 5, 2012 at 5:14 AM, Nick Wellnhofer <wellnhofer@aevum.de> wrote:
> I will write some POD once the implementation is stable. Currently, the
> C::C::P::B API looks like this:
>
> C::C::P::B is a subclass of Module::Build.
>
> There's a new constructor argument 'clownfish_params' that takes a hashref
> containing the following entries:
>
> * cf_include_dirs: arrayref of Clownfish include dirs containing .cfh files.
> Empty by default. Extensions will typically set this to
> cf_system_include_dirs (see below).
>
> * extra_c_sources: arrayref of extra C source dirs or files. Empty by
> default.
>
> * autogen_header: header for autogenerated files. Defaults to a "do not
> edit" text without license.
>
> * core_source_dir: directory containing the core Clownfish sources (.cfh and
> .c files). Defaults to 'core' or '../core'. Doesn't have to be provided
> normally.

Could "core_source_dir" and "extra_c_sources" be consolidated into a single
"source" param?

And then can we leverage the standard Module::Build "c_source" param to hold
directories which contain C code exclusively (i.e. no .cfh files)?

I think we can just dump all of Lucy's C source directories into
$clownfish_params{source} to work around the limitations of c_source in
versions of M::B prior to 0.3604.  The only annoying bit is that we might want
to separate out any .h files lest they be vacuumed up and installed into the
clownfish system include dir.

On a related note, I would suggest that we rename
$clownfish_params{cf_include_dirs} to $clownfish_params{include}.

  * The need for the "cf_" prefix is not strong because the param is nested
    within "clownfish_params".  (Things would be different if we were adding
    top-level M::B params).
  * IMO, it would be nice to parallel the "include" and "source" attributes of
    CFCHierarchy by using the same names.

> There's also an accessor for 'clownfish_params'.
>
> Then there are two class methods:
>
> my @dirs = C::C::P::B->cf_system_include_dirs;
>
> Returns the list of system-wide Clownfish include dirs in
> $Config{installsitearch} and $Config{installvendorarch}.
>
> my $path = C::C::P::B->cf_system_library_file($module_name);
>
> Returns the installed library file (.so or .dll) for a module. This is
> needed by extensions which have to link against the library of the module
> they're extending.

+1 to both, sounds good!

> I can try to mirror the API we would get with
> Module::Build#add_property using an inside-out class attribute.

I think we might consider just using an ordinary hash param inside the
Module::Build object.  I seem to recall some weirdness with Module::Build
object serializing and deserializing itself in all the process-spawning chaos,
and I'm not sure that an inside-out attribute will survive.

> Doesn't that mean that Lucy can't be loaded which sounds like the worst case
> to me?

I wasn't clear.  Here are some things that are worse than installation
failure:

  * Index corruption.
  * Security vulnerability.
  * Publication of a bad API which confuses users, constrains future
    development, etc.
  * A version number screwup affecting the CPAN toolchain rather than just
    Lucy module loading.
  * A bug which causes silent failure, e.g. discarding a supplied parameter.
  * A crashing bug which the test suite fails to catch.

I don't mean that having XSLoader crash out wouldn't be a serious problem, but
it is really easy to recover from with a subsequent release.  The version
number in question only ends up in the shared object.

So, I didn't want to stand in your way, because even if you were to disregard
the concern I raised, CPAN testers would ultimately force us to fix things and
no irreparable damage would be done.

> Is it that only that some versions of XSLoader have problems?

I recall experiencing problems with XSLoader not liking version numbers that
were not exactly the same -- I think it was 0.003 vs "0.003000" or something
like that.  It could have been a floating-point precision problem[1], but in
any case, the conservative strategy is to make sure that XSLoader::load() gets
fed exactly the same version number format that the shared object was built
with.

> It would be easy to switch to the old way of parsing the version number from
> Lucy.pm.

I don't think that's true any more because that code block has moved into
Clownfish::CFC::Build::Perl, right?  Invoking dist_version() is really the
right design choice.

Maybe we can adapt Lucy.pm instead, passing e.g. a literal version string
instead of $Lucy::VERSION.

Marvin Humphrey

[1] Note to self: Don't ever design a system to use floating point version
    numbers because of problematic comparison semantics.