5 Jun 2007 03:44
Re: Windows build - Bug fixes for "out of cygwin build".
David Genest <david.genest <at> gmail.com>
2007-06-05 01:44:46 GMT
2007-06-05 01:44:46 GMT
Hi,
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)
> <at> <at> -1,27 +1,40 <at> <at>
> #!/bin/sh
> #
>
> +CLEAN=0
> +
> +for opt in "$ <at> "
> +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...)
Because of my attempt to build on multiple environments in-cygwin, out-of-cygwin, I was under the impression that the configure scripts of the last run in one environment was interfering on the other. I wanted to make sure that it was not the case, thus adding a clean option. While refactoring this, I felt that the new structure would help expand in the future. The addition of the option was made to remove code duplication.
>
> case `uname -s` in
> CYGWIN*)
> <at> <at> -91,7 +91,7 <at> <at>
> 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).
Ok, if this is wrong, where should I put the define ? The Path.cc file needs to be aware of the correct define in order to select the path-win32.cc or path-posix.cc . Anyway, as someone is already porting to boost::filesystem, the problem will go away. But there might be other places where we need this information. I'm not sure I understand the way you suggest of finding that out.
> Index: Synopsis/dist/command/build_doc.py
> ===================================================================
> <at> <at> -43,7 +51,7 <at> <at>
> 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.
You are right, I had changed this in order to debug, and forgot to remove it.
> build.build.finalize_options(self)
>
> <at> <at> -56,7 +64,7 <at> <at>
> 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.
same....
> if self.tutorial : self.build_tutorial()
>
> def build_man_page(self):
> <at> <at> -100,17 +108,18 <at> <at>
> 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 ?)
When I tried debugging, I saw that the all the outputs were not made at the same place. There was a Release prefix in my build. I thought it was needed, but you are right, I was careless about the platform. We should: remove the Release/ prefix, or add a special case. I prefer the former. I do not know where the Release/ prefix comes from.
You will probably need the major part of the fix for the multiplatform support of the doc build.
> 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)
> <at> <at> -126,19 +135,20 <at> <at>
> 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.
Right again. My refactoring introduced a subtle error. Nice catch... Would the concatenation operation work here (so we can have "make -C html pdf")?
> Index: Synopsis/dist/command/build_ext.py
> ===================================================================
> --- Synopsis/dist/command/build_ext.py (revision 1825)
> +++ Synopsis/dist/command/build_ext.py (working copy)
> <at> <at> -83,7 +83,7 <at> <at>
>
> 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)
> <at> <at> -77,8 +77,7 <at> <at>
> 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.)
It turns out that the \ is not needed (because escaping " with \" does nothing: ie: print 'this is \"a test\"' yields the same as: print 'this is "a test"'). But the double quote is.
for example, without the double quotes, when I encounter this (taken from the python setup.py setup output):
C:\bin\cygwin\bin\sh.exe -c "../../../src/configure --prefix=\"`cygpath -a c:\bin\Python25`\" --with-python=\"`cygpath -a c:\bin\Python25`/python.exe\"
later, the script dies saying that "../../../src/configure: line 4294: /cygdrive/c/binPython25/python.exe: No such file or directory". Someone is clearly using the backslashes as escapes. If you use my change (without escaping), this error goes away, and you don't need the ugly \\\\\\\\\\\\\ patch.
I added locally the quotes fix on the latest revision (1827) of setup.py and everything builds. The "| tr" fix does the job, and no other change is needed in the .ac files.
D.
_______________________________________________ Synopsis-devel mailing list Synopsis-devel <at> lists.fresco.org http://lists.fresco.org/cgi-bin/listinfo/synopsis-devel
RSS Feed