lucy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nick Wellnhofer <wellnho...@aevum.de>
Subject Re: [lucy-dev] Build support classes for C extensions
Date Thu, 05 Apr 2012 18:26:36 GMT
On 05/04/2012 18:35, Marvin Humphrey wrote:
> 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.

If we start to use the c_source parameter, M::B will try to compile and 
link our .xs and .c files. This should work even with M::B prior to 
0.3604 if we copy all .c file into a single directory, but it doesn't 
solve the problem of how to actually pass the list of C sources with 
older M::B's. I think we'll need an extra parameter for that.

At the moment, we have our own code in ACTION_compile_custom_xs which 
doesn't care about c_source. I'd prefer to keep it until we decide it's 
OK to require M::B 0.3604 at some point in the future.

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

For now, I decided to only copy selected .h files manually. See patch 
0015 in LUCY-215.

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

My first choice was 'include_dirs'. I only added the 'cf_' prefix to 
avoid confusion with the 'include_dirs' parameter of M::B. Using only 
'include' is OK with me.

>    * IMO, it would be nice to parallel the "include" and "source" attributes of
>      CFCHierarchy by using the same names.

Yes, I thought about that. We should ultimately support multiple source 
directories in the same way as include directories. It just needs a bit 
of work in various parts of the build process.

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

I had a look at the source code of M::B 0.2808_01 (which we already 
require), and it should support add_property just fine. Only the 
'default' parameter is passed in a different way, and the 'check' 
parameter isn't supported, but we could work around that with a version 
check.

I'm also OK with a hash param.

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

Ah, it's only a compile/install time problem.

>> 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?

We could derive the path to the main .pm file from module_name like we 
do for the rest of the build files, and try to parse the version from there.

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

I'm not sure what's the best way to deal with that. Maybe keep 
dist_version and switch back if it causes problems?

Nick


Mime
View raw message