[Distutils] Re: CygwinCCompiler, msvc hack, BCPPCompiler

[Greg Ward]

On 27 July 2000, Rene Liebscher said:
> * msvc.patch: changes msvccompiler.py and build_ext.py
>       (You need its changes in build_ext.py to get the 
>        other two compilers work properly after applying their patches.)

OK, I've checked that one in.  One problem: I don't like the fact that
Extension.ext_symbols can now be None -- you took away the "or []" in
the constructor, which is inconsistent with every other attribute of
Extension.  Since this class is just a pile of data without much
behaviour, I value convenience -- don't have to check for None, just
assume everything is a list -- over completeness.  And I really dislike
the inconsistency you've introduced.

I can see why None-ness is needed, though: in
build_ext.get_export_symbols(), you can tell if 'export_symbols' was
supplied at all by looking for None.  But if an empty list was supplied, 
isn't that wrong?  If export_symbols is an empty list, then we'll go
ahead and build the .pyd with nothing in the export list, not even
"init" + module_name.  In the general-purpose library-building code
(msvccompiler.py), this would be right: but this code is geared towards
building Python extensions, and if a Python extension doesn't export its 
init function, what's the point?

I suggest changing build_ext.get_export_symbols() from this:

    if ext.export_symbols is None:
        return []
    else:
        return ext.export_symbols

to this:

    initfunc_name = "init" + string.split(ext.name,'.')[-1]
    if initfunc_name not in ext.export_symbols:
        ext.export_symbols.append(initfunc_name)

which of course assumes that ext.export_symbols is a list, which will be
valid as long as we put back the "or []" in Extension's constructor.
Sound reasonable?

> * bcpp.patch: changes bcppcompiler.py

OK, I'm going over this one now.

>     # either exchange python15.lib in the python libs directory against
>     # a Borland-like one, or create one with name bcpp_python15.lib 
>     # there and remove the pragmas from config.h  
>     #libraries.append ('mypylib')            
>     libraries.append ('import32')
>     libraries.append ('cw32mt')

I don't understand.  You commented out the use of 'mypylib' and added
the comment.  Can you explain what's going on here, please?  If we need
to make changes in Python (1.6 or 2.0!) to support Borland C++ better,
now's the time to do it.

>     ld_args.extend([',',output_filename])
>     # no map file
>     ld_args.extend([','])
>     # start libraries 
>     ld_args.extend([','])

This code seems fishy to me.  First of all, why are you using
'extend([x])' when 'append(x)' would do the same thing?  And second, are 
those commas really supposed to be whole words in the arg list?  I don't 
know how things work in Windows, but in Unix doing that would mean that
the compiler's argv list would look like

    [..., ',', 'foo.pyd', ',', ',', ...]

which is really strange.  I suspect that really should be coded as

    ld_args.append(",%s,," % output_filename)

which would make an argv of

    [..., ',foo.pyd,,', ...]

which seems a) more sensible, and b) looks like the Borland C++ command
lines I remember from 7-8 years ago.

So: which arguments really have to be separate words (ie. separate
elements of the argv[] array that BCPP will ultimately have to process)?
Does this question even make sense on a DOS/Windows environment, ie. is
the argument delineation implicit in the 'spawn()' call preserved in the 
child process?

> -    def find_library_file (self, dirs, lib):
> -
> +    def find_library_file (self, dirs, lib, debug=0):
> +        # find library file

Umm, the 'debug' flag is not part of the official CCompiler interface to 
'find_library_file()'.  Is this really necessary?  Should MSVCCompiler
do it as well?  If so, then the interface should change.

Looks like 'find_library_file()' is used in BCPPCompiler to give
preference to bcpp_xxx.lib; is this really important?  Where does it
arise?  Is this a common convention, or a Python-only convention?  (Or a 
Liebscher-only convention?  ;-)

I also note some lines of code > 80 columns in this file.  Please keep
all code under 75 columns!


> * cygwin.patch: the latest version of cygwinccompiler.py, 
>       some changes in spawn.py (I moved a port of spawn_nt()
>       in a separate function find_executable(), I use this
>       in my compiler class) and it removes from sysconfig.py
>        g['CC'] = "cc" and this other stuff which UnixCCompiler
>       needed in its old version.

In future, please supply a checkin comment for cygwinccompiler.py --
I'll figure out this time myself, but as long as the code is your
responsibility, so too are the checkin messages.

Other comments...

  * why are {gcc,ld,dllwrap}_version class attributes?  They are
    derived from the particular compiler used by a particular
    instance of the class, and when you set them in the constructor
    they become instance attributes.  So why even define them as
    class attrs?

  * 'check_config_h()' really should return meaningful strings instead
    of magic numbers.  Comment or no, any occurence of a bare
    integer in non-arithmetic circumstances is a magic number in
    my book, and I don't like magic numbers.

  * again, there are lines > 80 columns!  (I've fixed most of these.)

  * you shouldn't write to stdout or stderr in a compiler class -- use
    the 'announce()' method.  I should really add a 'debug_print()'
    method; that would be handy.

  * I don't like using 'list(foo)' to make a copy of a list 'foo' --
    either 'foo[:]' or 'copy(foo)' (from the standard copy module) is
    acceptable.

  * when assigning to a tuple, please use parens:
       (a, b) = ...
    not
       a, b = ...

OK, I've checked it in with a few tweaks (fixed long lines and tuple
assignments, mostly).  Please submit a new patch fixing the above
problems.

> And some cosmetic changes:
> Almost all messages start with a verb, but this one in install_lib.py
> starts with a noun.
[...]
> -                    skip_msg = "byte-compilation of %s skipped" % f
> +                    skip_msg = "skipping byte-compilation of %s" % f

Thanks!  At the cost of one extra character, I'll take it.  That has
been a very teeny tiny irritant for a while now.

        Greg
-- 
Greg Ward - Linux geek                                  gward@python.net
http://starship.python.net/~gward/
I don't understand the HUMOUR of the THREE STOOGES!!