Code review comment for lp:~peter-pearse/ubuntu/natty/tcl8.5/prop001

thanks, this looks pretty good overall. A few minor adjustments I'd like to see before applying:

+ifeq ($(CROSS_BUILDING), yes)
+ BUILD_TCLSH = $(UNIX_DIR)/build_host_bin/tclsh
[...]
+tclsh: ${TCLSH_OBJS} ${TCL_LIB_FILE} ./build_host_bin/tclsh

inconsistent use of paths; please either use $(UNIX_DIR) everywhere or './' everywhere.

+ifeq ($(CROSS_BUILDING), yes)
+tclsh: ${TCLSH_OBJS} ${TCL_LIB_FILE} ./build_host_bin/tclsh
+else
 tclsh: ${TCLSH_OBJS} ${TCL_LIB_FILE}
+endif

I think this can be more simply written as:

ifeq ($(CROSS_BUILDING), yes)
tclsh: ./build_host_bin/tclsh
endif
tclsh: ${TCLSH_OBJS} ${TCL_LIB_FILE}

If not, then at least this would work:

ifeq ($(CROSS_BUILDING), yes)
CROSS_TARGET=./build_host_bin/tclsh
endif
tclsh: ${TCLSH_OBJS} ${TCL_LIB_FILE} ${CROSS_TARGET}

+ifeq ($(CROSS_BUILDING), yes)
+
+./build_host_bin/tclsh:
+ @echo "Need a pre_built tclsh for cross builds"
+ @echo "Install, or build the package, on the host."
+ @echo "If building, pass CROSS_BUILDING=prepass to make."
+ @echo "Copy down resulting tclsh into unix/build_host_bin/tclsh"
+
+endif

Rather than spitting out a message, it would be nice if we could handle this entirely in the upstream build rules - i.e., somehow setting a CC_FOR_BUILD variable and building all our build-arch object files in a separate path, so we can do the ./configure once and have tcl do the right thing. This would also improve the odds of such a patch being accepted cleanly upstream, where it would benefit everyone and not just those using this Debian packaging.

How much additional effort do you think it would be to reorganize the patch this way? If it's too hard I'm ok to take this part as-is for now, though we eventually need to clean it up.

Regardless, please address the first two points mentioned above.