Re: Windows build - Bug fixes for "out of cygwin build".

David Genest wrote:
> Hi all,
> 
> Attached is a patch that fixes some bugs for the windows build. (Using
> native python for "python setup.py build" command).

Thanks a lot ! Let me review it to make sure I understand the purpose:


> Index: autogen.sh
> ===================================================================
> --- autogen.sh	(revision 1825)
> +++ autogen.sh	(working copy)
> @@ -1,27 +1,40 @@
>  #!/bin/sh
>  #
>  
> +CLEAN=0
> +
> +for opt in "$@" 
> +do 
> +	case $opt in 
> +	  	-clean) CLEAN=1 ;;
> +		*) ;;
> +	esac	
> +done 

[...]

OK, you add an option to remove all configure scripts. Why do you want that ?
You still need to run autogen.sh each time one of the configure.ac scripts has
changed.

> +	   if [ "$2" == "--with-header" ]; then 
> +		   autoheader
> +	   fi

[...]

> -conf_with_header Synopsis/Parsers/IDL
> +conf Synopsis/Parsers/IDL --with-header

Shouldn't autoheader only do something if the corresponding configure.ac file
contains a call to AC_CONFIG_HEADERS. In other words, is the above addition
really changing anything ? (I'm just trying to keep things simple...)


> Index: src/configure.ac
> ===================================================================
> --- src/configure.ac	(revision 1825)
> +++ src/configure.ac	(working copy)
> @@ -80,7 +80,7 @@
>  
>  AC_SUBST_FILE(GC_BRIDGE)
>  
> -PLATFORM=`$PYTHON -c "import os; print os.name"`
> +PLATFORM=`$PYTHON -c "import os; print os.name" | tr -d "\r"`

Excellent !

>  
>  case `uname -s` in
>  CYGWIN*)
> @@ -91,7 +91,7 @@
>        if test "$CXX" == "g++"; then
>          CPPFLAGS="$CPPFLAGS -D PARSE_MSVC"
>          CFLAGS="-mno-cygwin $CFLAGS"
> -        CXXFLAGS="-mno-cygwin $CXXFLAGS"
> +        CXXFLAGS="-mno-cygwin -D_WIN32 $CXXFLAGS"

This looks wrong. The compiler should predefine a platform macro. If it
isn't _WIN32, we need to check for something else. To see what macros
are predefined, you can use 'g++ -v -dD -E ...' (just what we do when
invoking gcc from the Emulator module).


> Index: Synopsis/dist/command/build_doc.py
> ===================================================================

> @@ -43,7 +51,7 @@
>     def finalize_options (self):
>  
>        # If no option was given, do all media.
> -      if not (self.html or self.printable or self.sxr):
> +      if not (self.html or self.printable or self.sxr or self.tutorial):
>           self.html = self.printable = True

I think the logic here was to set all media options to True if none
was given. 'tutorial' isn't a medium. 'setup.py build_doc --tutorial'
still doesn't tell whether to generate html or pdf.

>        build.build.finalize_options(self)
>  
> @@ -56,7 +64,7 @@
>        self.build_lib = '.'
>  
>        if self.man_page: self.build_man_page()
> -      if self.ref_manual or self.sxr: self.build_ref_manual()
> +      if self.ref_manual or self.sxr or self.html: self.build_ref_manual()

Similarly here: 'setup.py build_doc --tutorial --html' should build the tutorial,
but not the reference manual.


>        if self.tutorial: self.build_tutorial()
>      
>     def build_man_page(self):
> @@ -100,17 +108,18 @@
>        self.announce("building API reference manual")
>  
>        tmp_man_dir = os.path.abspath(os.path.join(self.build_temp,
> -                                                 'doc/Manual'))
> +                                                 'Release/doc/Manual'))

I'm not sure about that one. Is the original path wrong ? (I remember
having seen 'Release/' as part of some path when building on windows, but
probably only in the compilation of the code, not the doc generation. So is
this for consistency only ? Also, you seem to make this change irrespectively
of the platform. This should be 'nt' only, no ?)

>        make = os.environ.get('MAKE', 'make')
>  
>        build_clib = self.distribution.get_command_obj('build_clib')
>        build_clib.ensure_finalized()
>  
> -      tmpdir = os.path.join(build_clib.build_ctemp, 'src')
> +      tmpdir = os.path.abspath(os.path.join(build_clib.build_ctemp, 'src'))
>  
> -      # build the 'doc' target
> -      spawn([make, '-C', tmpdir, 'doc'])
> +	  # build the 'doc' target
> +      makecommand = '%s -C %s doc'% (make, makePlatformPath(tmpdir))
> +      spawn(['sh', '-c', makecommand])
>  
>        for d in ['cxx.syn', 'cxx-sxr.syn']:
>           src, dest = os.path.join(tmpdir, d), os.path.join(tmp_man_dir, d)
> @@ -126,19 +135,20 @@
>              print 'not copying', src
>           
>        # now run make inside doc/Manual to do the rest
> -
> -      srcdir = os.path.abspath('doc/Manual/')
> -
>        cwd = os.getcwd()
>        mkpath(tmp_man_dir, 0777, self.verbose, self.dry_run)
>  
>        if self.html:
> -         spawn([make, '-C', tmp_man_dir, 'html'])
> +         target = 'html'
>        if self.printable:
> -         spawn([make, '-C', tmp_man_dir, 'pdf'])
> +         target = 'pdf'
>        if self.sxr:
> -         spawn([make, '-C', tmp_man_dir, 'sxr', 'sxr=%s'%self.sxr])
> +         target = 'sxr sxr=%s'%self.sxr
>  
> +      if not target == None:
> +         command = 'make -C %s %s' % (makePlatformPath(tmp_man_dir), target)
> +         spawn(['sh', '-c', command], self.verbose, self.dry_run)
> +

Careful here. '--html' and '--printable' aren't exclusive, i.e.
'setup.py build_doc --html --printable' is an entirely reasonable command.
With your change this wouldn't work any more.


> Index: Synopsis/dist/command/build_ext.py
> ===================================================================
> --- Synopsis/dist/command/build_ext.py	(revision 1825)
> +++ Synopsis/dist/command/build_ext.py	(working copy)
> @@ -83,7 +83,7 @@
>          
>          make = os.environ.get('MAKE', 'make')
>  
> -        command = '%s -C "%s" %s'%(make, path, ext[1])
> +        command = '%s -C \"%s\" %s'%(make, path, ext[1])

Why is that needed ? (I'm confused: a single '\' will escape the following
character, in python. That shouldn't be needed. Are you trying to escape
one level down, i.e. in the command that gets run later ? You'd need '\\'
for that, no ?)

>          spawn(['sh', '-c', command], self.verbose, self.dry_run)
>  
>          #The extension may not be compiled. For now just skip it.
> Index: Synopsis/dist/command/config.py
> ===================================================================
> --- Synopsis/dist/command/config.py	(revision 1825)
> +++ Synopsis/dist/command/config.py	(working copy)
> @@ -77,8 +77,7 @@
>              self.config('src/Synopsis/gc', self.build_ctemp, self.build_clib)
>  
>          if os.name == 'nt':
> -            syn_cxx = '`cygpath -a %s/src`'%os.path.abspath(self.build_ctemp)
> -            syn_cxx = syn_cxx.replace('\\', '\\\\\\\\\\\\\\\\')
> +            syn_cxx = '`cygpath -a \"%s/src\"`'%os.path.abspath(self.build_ctemp)

Same here. I'm surprized this works (though I would love to be able to throw away
all this multi-escaping. It's so ugly.)

>          else:
>              syn_cxx = '%s/src'%os.path.abspath(self.build_ctemp)    
>  

I think I'll understand the rest of the patch once I see the answers to the above
questions. Many thanks !

		Stefan

-- 

      ...ich hab' noch einen Koffer in Berlin...