Re: Windows build - Bug fixes for "out of cygwin build".
"David Genest" <[email protected]> Mon, 4 Jun 2007 21:44:46 -0400
| Newsgroups | gmane.comp.documentation.synopsis |
|---|---|
| Message-ID | <[email protected]> |
--===============0036886916==
Content-Type: multipart/alternative;
boundary="----=_Part_15100_16860507.1181007886938"
------=_Part_15100_16860507.1181007886938
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
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)
> > @@ -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.acfile
> 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*)
> > @@ -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).
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
> > ===================================================================
>
> > @@ -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.
You are right, I had changed this in order to debug, and forgot to remove
it.
> 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.
same....
> 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 ?)
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)
> > @@ -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.
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)
> > @@ -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.)
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.pyand everything builds. The "| tr" fix does the job, and no
other change is
needed in the .ac files.
D.
------=_Part_15100_16860507.1181007886938
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Hi, <br><br><div><span class="gmail_quote"></span><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Thanks a lot ! Let me review it to make sure I understand the purpose:
<br><br><br>> Index: autogen.sh<br>> ===================================================================<br>> --- autogen.sh (revision 1825)<br>> +++ autogen.sh (working copy)<br>> @@ -1,27 +1,40 @@
<br>> #!/bin/sh<br>> #<br>><br>> +CLEAN=0<br>> +<br>> +for opt in "$@"<br>> +do<br>> + case $opt in<br>> + -clean) CLEAN=1 ;;<br>> + *) ;;<br>> + esac
<br>> +done<br><br>[...]<br><br>OK, you add an option to remove all configure scripts. Why do you want that ?<br>You still need to run autogen.sh each time one of the <a href="http://configure.ac" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)">
configure.ac</a> scripts has
<br>changed.<br><br>> + if [ "$2" == "--with-header" ]; then<br>> + autoheader<br>> + fi<br><br>[...]<br><br>> -conf_with_header Synopsis/Parsers/IDL<br>> +conf Synopsis/Parsers/IDL --with-header
<br><br>Shouldn't autoheader only do something if the corresponding <a href="http://configure.ac" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)">configure.ac</a> file<br>contains a call to AC_CONFIG_HEADERS. In other words, is the above addition
<br>really changing anything ? (I'm just trying to keep things simple...)
</blockquote><div><br>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.
<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">><br>> case `uname -s` in<br>> CYGWIN*)<br>> @@ -91,7 +91,7 @@<br>
> if test "$CXX" == "g++"; then
<br>> CPPFLAGS="$CPPFLAGS -D PARSE_MSVC"<br>> CFLAGS="-mno-cygwin $CFLAGS"<br>> - CXXFLAGS="-mno-cygwin $CXXFLAGS"<br>> + CXXFLAGS="-mno-cygwin -D_WIN32 $CXXFLAGS"
<br><br>This looks wrong. The compiler should predefine a platform macro. If it<br>isn't _WIN32, we need to check for something else. To see what macros<br>are predefined, you can use 'g++ -v -dD -E ...' (just what we do when
<br>invoking gcc from the Emulator module).</blockquote><div><br>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.
<br> </div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">> Index: Synopsis/dist/command/build_doc.py<br>> ===================================================================
<br><br>> @@ -43,7 +51,7 @@<br>> def finalize_options (self):<br>><br>> # If no option was given, do all media.<br>> - if not (self.html or self.printable or self.sxr):<br>> + if not (
self.html or self.printable or self.sxr or self.tutorial):<br>> self.html = self.printable = True<br><br>I think the logic here was to set all media options to True if none<br>was given. 'tutorial' isn't a medium. '
setup.py build_doc --tutorial'<br>still doesn't tell whether to generate html or pdf.</blockquote><div><br>You are right, I had changed this in order to debug, and forgot to remove it.<br> </div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
> build.build.finalize_options(self)<br>><br>> @@ -56,7 +64,7 @@<br>> self.build_lib = '.'<br>><br>> if self.man_page: self.build_man_page()<br>> - if self.ref_manual
or self.sxr: self.build_ref_manual()<br>> + if self.ref_manual or self.sxr or self.html: self.build_ref_manual()<br><br>Similarly here: 'setup.py build_doc --tutorial --html' should build the tutorial,<br>
but not the reference manual.</blockquote><div><br> same....</div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">> if self.tutorial
: self.build_tutorial()<br>><br>> def build_man_page(self):<br>> @@ -100,17 +108,18 @@<br>> self.announce("building API reference manual")<br>><br>> tmp_man_dir = os.path.abspath
(os.path.join(self.build_temp,<br>> - 'doc/Manual'))<br>> + 'Release/doc/Manual'))<br><br>I'm not sure about that one. Is the original path wrong ? (I remember
<br>having seen 'Release/' as part of some path when building on windows, but<br>probably only in the compilation of the code, not the doc generation. So is<br>this for consistency only ? Also, you seem to make this change irrespectively
<br>of the platform. This should be 'nt' only, no ?)</blockquote><div><br>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.
<br><br>You will probably need the major part of the fix for the multiplatform support of the doc build.<br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
> make = os.environ.get('MAKE', 'make')<br>><br>
> build_clib =
self.distribution.get_command_obj('build_clib')<br>> build_clib.ensure_finalized()<br>><br>> - tmpdir = os.path.join(build_clib.build_ctemp, 'src')<br>> + tmpdir = os.path.abspath
(os.path.join(build_clib.build_ctemp, 'src'))<br>><br>> - # build the 'doc' target<br>> - spawn([make, '-C', tmpdir, 'doc'])<br>> + # build the 'doc' target
<br>> + makecommand = '%s -C %s doc'% (make, makePlatformPath(tmpdir))<br>> + spawn(['sh', '-c', makecommand])<br>><br>> for d in ['cxx.syn', 'cxx-sxr.syn']:
<br>> src, dest = os.path.join(tmpdir, d), os.path.join(tmp_man_dir, d)<br>> @@ -126,19 +135,20 @@<br>> print 'not copying', src<br>><br>> # now run make inside doc/Manual to do the rest
<br>> -<br>> - srcdir = os.path.abspath('doc/Manual/')<br>> -<br>> cwd = os.getcwd()<br>> mkpath(tmp_man_dir, 0777, self.verbose, self.dry_run)<br>><br>> if self.html
:<br>> - spawn([make, '-C', tmp_man_dir, 'html'])<br>> + target = 'html'<br>> if self.printable:<br>> - spawn([make, '-C', tmp_man_dir, 'pdf'])
<br>> + target = 'pdf'<br>> if self.sxr:<br>> - spawn([make, '-C', tmp_man_dir, 'sxr', 'sxr=%s'%self.sxr])<br>> + target = 'sxr sxr=%s'%self.sxr
<br>><br>> + if not target == None:<br>> + command = 'make -C %s %s' % (makePlatformPath(tmp_man_dir), target)<br>> + spawn(['sh', '-c', command], self.verbose, self.dry_run
)<br>> +<br><br>Careful here. '--html' and '--printable' aren't exclusive, i.e.<br>'setup.py build_doc --html --printable' is an entirely reasonable command.<br>With your change this wouldn't work any more.
</blockquote><div><br>Right again. My refactoring introduced a subtle error. Nice catch... Would the concatenation operation work here (so we can have "make -C html pdf")?<br> </div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
> Index: Synopsis/dist/command/build_ext.py<br>> ===================================================================<br>> --- Synopsis/dist/command/build_ext.py (revision 1825)<br>> +++ Synopsis/dist/command/build_ext.py (working copy)
<br>> @@ -83,7 +83,7 @@<br>><br>> make = os.environ.get('MAKE', 'make')<br>><br>> - command = '%s -C "%s" %s'%(make, path, ext[1])<br>> + command = '%s -C \"%s\" %s'%(make, path, ext[1])
<br><br>Why is that needed ? (I'm confused: a single '\' will escape the following<br>character, in python. That shouldn't be needed. Are you trying to escape<br>one level down, i.e. in the command that gets run later ? You'd need '\\'
<br>for that, no ?)<br><br>> spawn(['sh', '-c', command], self.verbose, self.dry_run)<br>><br>> #The extension may not be compiled. For now just skip it.<br>> Index: Synopsis/dist/command/config.py
<br>> ===================================================================<br>> --- Synopsis/dist/command/config.py (revision 1825)<br>> +++ Synopsis/dist/command/config.py (working copy)<br>> @@ -77,8 +77,7 @@
<br>> self.config('src/Synopsis/gc', self.build_ctemp, self.build_clib)<br>><br>> if <a href="http://os.name" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)">
os.name</a> == 'nt':<br>> - syn_cxx = '`cygpath -a %s/src`'%os.path.abspath(
self.build_ctemp)<br>> - syn_cxx = syn_cxx.replace('\\', '\\\\\\\\\\\\\\\\')<br>> + syn_cxx = '`cygpath -a \"%s/src\"`'%os.path.abspath(self.build_ctemp)<br><br>
Same here. I'm surprized this works (though I would love to be able to throw away<br>all this multi-escaping. It's so ugly.)</blockquote><div><br>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.
<br>for example, without the double quotes, when I encounter this (taken from the python setup.py setup output): <br>C:\bin\cygwin\bin\sh.exe -c "../../../src/configure --prefix=\"`cygpath -a c:\bin\Python25`\" --with-python=\"`cygpath -a c:\bin\Python25`/python.exe\"
<br>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.
<br><br> 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.<br><br>D.<br></div></div><br>
------=_Part_15100_16860507.1181007886938--
--===============0036886916==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
_______________________________________________
Synopsis-devel mailing list
[email protected]
http://lists.fresco.org/cgi-bin/listinfo/synopsis-devel
--===============0036886916==--