Re: [Patch] Make Ruby and Perl shared libraries .dll again

On Fri, May 29, 2009 at 3:56 AM, Stefan Sperling <stsp_at_elego.de> wrote:
> On Thu, May 28, 2009 at 10:23:54PM -0700, Joe Swatosh wrote:
>> This is not part of the Ruby bindings so it is not for me to commit
>> without approval by a full committer. Is the patch acceptable with the
>> log message amended below?
>>
>> --
>> Joe
>>
>> [[[
>>
>> Partially revert r37331 "On Windows, shared libs for Python bindings
>> should be *.pyd not *.dll."  In addition to changing the shared libs for
>> the Python bindings, r37331 inadvertently changed the extensions of the
>> shared libraries produced for the Perl and Ruby bindings from .dll to
>> .pyd.  Change the extensions back to .dll for the Ruby and Perl
>> bindings.
>>
>> * build/generator/gen_base.py (TargetSWIG.add_dependencies): Mark Python
>>   wrapper shared libs as type "pyd" not "lib".
>>
>> ]]]
>>
>> ===================================================================
>> --- build/generator/gen_base.py (revision 37787)
>> +++ build/generator/gen_base.py (working copy)
>> @@ -550,12 +550,13 @@
>>     # Extract SWIG module name from .i file name
>>     module_name = iname[:4] != 'svn_' and iname[:-2] or iname[4:-2]
>>
>> -    lib_extension = self.gen_obj._extension_map['pyd', 'target']
>> +    lib_extension = self.gen_obj._extension_map['lib', 'target']
>>     if self.lang == "ruby":
>>       lib_filename = module_name + lib_extension
>>     elif self.lang == "perl":
>>       lib_filename = '_' + module_name.capitalize() + lib_extension
>>     else:
>> +      lib_extension = self.gen_obj._extension_map['pyd', 'target']
>
> Why put this in an else clause? This code assumes that we'll never
> ever add more scripting languages to the bindings. I'd say it would
> be safer to explicitly check for python before changing the extension
> to ".pyd". We don't want the same problem to bite us again some time
> later. We should rather make sure that this problem will never hit
> us again. So I'd suggest doing something like this instead:
>
>     if self.lang == "ruby":
>       lib_filename = module_name + lib_extension
>     elif self.lang == "perl":
>       lib_filename = '_' + module_name.capitalize() + lib_extension
>     elif self.lang == "python":
>       lib_extension = self.gen_obj._extension_map['pyd', 'target']
>     else:
>       # throw an error, unknown language binding, please add proper
>       # library filename extension here
>
> Stefan
>
Hi,

Its not that I think this is a bad idea, but I'm trying to follow the patch
submission guidelines: "A patch submission should contain one logical change;
please don't mix N unrelated changes in one submission — send N separate emails
instead." I am only trying to restore my ability to build the Ruby bindings
without local modifications. Since I haven't been able to build the Perl
bindings for several years, I was actually quite hesitant to include
that in the
patch. The last time I suggested patching something to do with the Perl I was
told I shouldn't since I wasn't a Perl expert.

With all that in mind, I was trying to make a very small change that would
accomplish my goal. I would prefer that my patch or similar be approved and
then you make whatever changes you think best to the formatting and logic.

--
Joe
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2359276