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