lucy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marvin Humphrey <mar...@rectangular.com>
Subject Re: [lucy-dev] Build support classes for C extensions
Date Sat, 07 Apr 2012 02:15:26 GMT
On Thu, Apr 5, 2012 at 11:26 AM, Nick Wellnhofer <wellnhofer@aevum.de> wrote:
> 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.

OK, that makes sense.

Can we still consolidate "core_source_dir" and "extra_c_sources" into
a single "source" param?

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

So your approach is to do what it takes to make those .h files available
because otherwise the autogenerated files won't compile -- but the
installation of those .h files is an implementation detail and they are not
part of the interface exposed to extension writers.

Sounds ideal.  :)

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

We might benefit from taking a step back and considering how various compiler
interfaces allow the user to communicate what files should be compiled.

    * cc, gcc, clang etc.
    * Various IDEs, e.g XCode.
    * javac
    * perl/python/ruby interpreters
    * etc.

Right now, CFC is closer to an IDE, javac, or an interpreter than to
command-line cc, in that CFC wants to compile the whole project in one session
rather than a compile a single source file to a single object file.

CFC is also closer to an IDE than any of the others in that it starts from a
multi-file specification.

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

IMO, it's desirable to avoid burdening extension writers with having to write
their $VERSION assignment code in a format we can parse.

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

+1 to keep dist_version().  Both you and David Wheeler have suggested it
independently.  I'm persuaded.

In addition, we should apply the patch below my sig, which changes
XSLoader::load to use the same X.Y.Z version format that we supply as the
dist_version argument to the Lucy::Build constructor.

For the record, I'd misremembered the call to XSLoader::load().  I had thought
it looked like this:

    XSLoader::load( 'Lucy', $Lucy::VERSION );

Turns out we were already passing a literal:

    XSLoader::load( 'Lucy', '0.003000' );

The patch below changes it to be a different literal:

    XSLoader::load( 'Lucy', '0.3.0' );

That variant tested successfully with both stock perl 5.10.0 and stock perl
5.15.4.

Marvin Humphrey

diff --git a/devel/bin/update_version b/devel/bin/update_version
index 72e5ad4..1e27126 100755
--- a/devel/bin/update_version
+++ b/devel/bin/update_version
@@ -50,7 +50,7 @@ my $buf;
 $buf = read_file('perl/lib/Lucy.pm');
 $buf =~ s/(our \$VERSION\ +=\ +)'.+?'/$1'$x_yyyzzz_version'/
     or die "no match";
-$buf =~ s/XSLoader::load\( 'Lucy', '(.+?)'/XSLoader::load\( 'Lucy',
'$x_yyyzzz_version'/
+$buf =~ s/XSLoader::load\( 'Lucy', '(.+?)'/XSLoader::load\( 'Lucy',
'$x_y_z_version'/
     or die "no match";
 write_file( 'perl/lib/Lucy.pm', $buf );

diff --git a/perl/lib/Lucy.pm b/perl/lib/Lucy.pm
index df942ce..97c3c47 100644
--- a/perl/lib/Lucy.pm
+++ b/perl/lib/Lucy.pm
@@ -27,7 +27,7 @@ $VERSION = eval $VERSION;
 use XSLoader;
 # This loads a large number of disparate subs.
 BEGIN {
-    XSLoader::load( 'Lucy', '0.003000' );
+    XSLoader::load( 'Lucy', '0.3.0' );
     _init_autobindings();
 }

Mime
View raw message