Code review comment for lp:~peter-pearse/ubuntu/natty/python2.6/prop001

Hi Peter,

Still looking for an explanation for the modifications to debian/patches/unicode.diff and debian/patches/multibytecodec.diff here; I think this was a workaround for other brokenness at the time you were first patching python, can you please check whether the package cross-builds for you without these changes?

+ buildir = os.environ.get('CURDIR')

This may be a GNU-make-specific variable; ok for me, not sure if it's ok for upstream.

+ os.environ["LDFLAGS"] = '-L /usr/' + exec_prefix + '/lib -L /usr/' + exec_prefix + '/usr/lib'
+ os.environ["CFLAGS"] = '-I/usr/' + exec_prefix + '/include -I/usr/' + exec_prefix + '/usr/include'

/usr/$arch/usr/? What creates such a directory? I don't think that should be in here.

@@ -342,7 +402,7 @@
 ifneq (,$(filter $(DEB_HOST_ARCH), hppa))
   TEST_EXCLUDES += test_fork1 test_multiprocessing test_socketserver test_wait3 test_wait4 test_gdb
 endif
-ifneq (,$(filter $(DEB_HOST_ARCH), arm avr32))
+ifneq (,$(filter $(DEB_HOST_ARCH), avr32))
   TEST_EXCLUDES += test_ctypes
 endif
 ifneq (,$(filter $(DEB_HOST_ARCH), m68k avr32))

This seems to be an unrelated change; we should never match on 'arm' here, that's the obsolete OABI architecture (never existed for Ubuntu, no longer available for Debian). Please drop this change.

@@ -556,6 +623,14 @@
        sed -i 's/\(Py_InitModule4[^@]*\)@/\1_64@/' \
                debian/lib$(PVER).symbols debian/$(PVER)-dbg.symbols
 endif
+ifneq ($(DEB_BUILD_GNU_TYPE),$(DEB_HOST_GNU_TYPE))
+ @echo "Drop any arch dependent symbols"
+ @symfiles=`find debian -type f -name \*.symbols` ; \
+ for f in $$symfiles ; do \
+ nawk '$$0 ~ /arch=/ {next;}; {print $$0}' $$f > tmpfile ; \
+ mv tmpfile $$f ; \
+ done ;
+endif

 install: $(build_target) stamps/stamp-install
 stamps/stamp-install: stamps/stamp-build control-file stamps/stamp-control

This seems to be a workaround for a dpkg-shlibdeps bug, isn't it? We shouldn't need to drop architecture-dependent symbols by hand, the commands that postprocess these should know which architecture is targeted. If this is a workaround, let's not change this here - let's get it fixed where it should be fixed instead.

Finally, there are an awful lot of changes to Makefile.pre.in to replace 'BUILDPYTHON' with 'HOSTPYTHON'; but the changes exactly reverse the meaning wrt GNU cross-compiling convention. I can't remember if this was already the case in the earlier merge request, but given how many places this is getting changed to the wrong way around here, I think we should be calling this 'BUILDPYTHON' as is originally the case upstream. Indeed, I think doing it that way around requires fixing up a much smaller number of make target dependencies, changing them from $(BUILDPYTHON) to $(PYTHON), instead of fixing every invocation of $(BUILDPYTHON).

Most of the target dependencies should probably be changed to $(PYTHON) $(BUILDPYTHON), and the $(BUILDPYTHON) target itself should be changed to $(PYTHON), IMHO.