This is the mail archive of the
cygwin-apps@cygwin.com
mailing list for the Cygwin project.
[Review - Not Yet Good to Go] rxp 1.3.0-1: Simple Validating XML parser (long)
- From: "Rafael Kitover" <caelum at debian dot org>
- To: <cygwin-apps at cygwin dot com>
- Date: Wed, 4 Feb 2004 14:08:06 -0800
- Subject: [Review - Not Yet Good to Go] rxp 1.3.0-1: Simple Validating XML parser (long)
- Reply-to: <caelum at debian dot org>
*** Binary package looks good, and it seems to work. (PASSED)
I tried the following on a file from docbook-xsl:
rxp -V -N /usr/share/docbook-xsl/common/l10n.xml 1>/dev/null
And got:
Warning: Undeclared attribute lang for element l:gentext
in entity "af" at line 72 char 57 of
file:///usr/share/docbook-xsl/common/af.xml
in unnamed entity at line 47 char 5 of
file:///usr/share/docbook-xsl/common/l10n.xml
Warning: Undeclared attribute lang for element l:dingbat
in entity "af" at line 177 char 57 of
file:///usr/share/docbook-xsl/common/af.xml
in unnamed entity at line 47 char 5 of
file:///usr/share/docbook-xsl/common/l10n.xml
Warning: Undeclared attribute lang for element l:template
in entity "af" at line 213 char 51 of
file:///usr/share/docbook-xsl/common/af.xml
in unnamed entity at line 47 char 5 of
file:///usr/share/docbook-xsl/common/l10n.xml
zsh: exit 2 rxp -V -N /usr/share/docbook-xsl/common/l10n.xml > /dev/null
Looks like it's doing the right thing to me :) Seriously, a validating xml
parser this fast would be an extremely useful thing to have in Cygwin,
especially once the docbook stuff gets sorted out and more people get hacking
on Cygwin documentation, thank you!!!
*** Source package is broken (CRITICAL):
./rxp-1.3.0-1.sh all
-- cygbuild.sh 2004-01-27 1.238 http://cygbuild.sourceforge.net/
-- Wait, examining environment and preparing variables
./rxp-1.3.0-1.sh: line 612: [: -eq: unary operator expected
./rxp-1.3.0-1.sh: line 612: [: -eq: unary operator expected
-- Extracting /tmp/cyg/rxp-1.3.0.tar.gz
./rxp-1.3.0-1.sh: line 2445: -zxf: command not found
./rxp-1.3.0-1.sh.CygbuildCommandMain: [FATAL] status is 127.
zsh: exit 127 ./rxp-1.3.0-1.sh all
If TAR was set in the script, it otherwise builds the binary package fine, and
would be good to go. See patch for script below.
Fixing a few things, there's still:
/bin/install "-m 644" -d /tmp/cyg/rxp-1.3.0/.inst/usr/share/man/man1
/bin/install "-m 644" *.1 /tmp/cyg/rxp-1.3.0/.inst/usr/share/man/man1
find: /tmp/cyg/rxp-1.3.0/.inst/usr/share/man: Permission denied
A directory must be executable for find to chdir into it.
*** This is a problem with your patch (CRITICAL):
+
+install-man:
+ $(INSTALL) $(INSTALL_DATA) -d $(MANDIR)
It should be something like
$(INSTALL) $(INSTALL_DIR) -d $(MANDIR)
With INSTALL_DIR = "-m 755"
This prevents the build directory from being deleted:
rm -rf rxp-1.3.0
rm: cannot change to directory `rxp-1.3.0/.inst/usr/share/man': Permission
denied
rm: cannot remove directory `rxp-1.3.0/.inst/usr/share': Directory not empty
rm: cannot remove directory `rxp-1.3.0/.inst/usr': Directory not empty
rm: cannot remove directory `rxp-1.3.0/.inst': Directory not empty
rm: cannot remove directory `rxp-1.3.0': Directory not empty
zsh: exit 1 rm -rf rxp-1.3.0
*** Build directory not removed (CRITICAL):
Also, please make sure that "./rxp-1.3.0-1.sh all" completely removes the
directory "rxp-1.3.0/" when it finishes running. I wouldn't consider it
good-to-go otherwise.
*** Other build script bugs (NON-CRITICAL):
./rxp-1.3.0-1.sh: line 2: -print: command not found
This is a missing \ line continuation. Line continuations are evil and should
be avoided anyway :)
*** Here's a patch to get the build script to at least work properly:
-----------------------------------------------------------------------
--- rxp-1.3.0-1.sh.orig 2004-01-29 04:59:12.000000000 -0800
+++ rxp-1.3.0-1.sh 2004-02-04 13:50:31.640625000 -0800
@@ -352,6 +352,7 @@
EGREP=$(BinPath egrep)
WHICH=$(BinPath which)
MAKE=$(BinPath make)
+TAR=$(BinPath tar)
#######################################################################
#
@@ -609,7 +610,7 @@
local count=${arr[*]}
- if [ $count -eq 3 ]; then
+ if [ "x$count" = "x3" ]; then
try=${arr[2]}
fi
@@ -3208,7 +3209,7 @@
for file in $(find $instdir \
-name "*.dll" \
-o -name "*.exe" \
- -o \( -type f -a -perm +111 \) # Executables
+ -o \( -type f -a -perm +111 \) \
-print )
do
if [ -z "$done" ]; then
@@ -3993,4 +3994,4 @@
CygbuildMain $*
-# End of file
\ No newline at end of file
+# End of file
-----------------------------------------------------------------------
I'll give it a good to go with this patch, but would of course prefer that some
of the other issues be also looked at, even in a later release.
*** Trace messages in build script (NON-CRITICAL):
-- Making package [source] /tmp/cyg/rxp-1.3.0-1-src.tar.bz2
./rxp-1.3.0-1.sh: line 1: type: cygbuild.pl: not found
NOTE: To build a source package, install Cygbuild from
http://cygbuild.sourceforge.net/
This is confusing, since it does build a source package successfully.
Also stuff like:
make: *** No rule to make target `distclean'. Stop.
-- [FIX] Hm, running rm *.o instead
<rant>
I personally believe that extraneous garbage should not pop up in a package
build script, whatever script is used as a base. Distribution packages are by
definition higher quality than a wild CVS snapshot. This __WILL NOT__ hold up
release, just my opinion. In the future Cygwin users may be building packages
from sources more frequently, or even using an automated tool like Debian's
"apt-get -b source <pkg>" or the FreeBSD "portupgrade <pkg>" and this will only
become more important.
</rant>
*** Warnings in compile (NON-CRITICAL):
infoset-print.c: In function `pcdata':
infoset-print.c:459: warning: comparison is always true due to limited range of
data type
gcc -Wall -ansi -pedantic -g -O -DCHAR_SIZE=16 -c -o xmlparser.o xmlparser.c
xmlparser.c: In function `check_qualname_syntax':
xmlparser.c:1239: warning: comparison is always true due to limited range of
data type
xmlparser.c: In function `process_xml_space':
xmlparser.c:1879: warning: comparison is always true due to limited range of
data type
xmlparser.c:1883: warning: comparison is always true due to limited range of
data type
xmlparser.c:1889: warning: comparison is always true due to limited range of
data type
xmlparser.c: In function `parse_pcdata':
xmlparser.c:1967: warning: comparison is always true due to limited range of
data type
xmlparser.c:2081: warning: comparison is always true due to limited range of
data type
xmlparser.c: In function `process_xml_decl':
xmlparser.c:2840: warning: comparison is always true due to limited range of
data type
xmlparser.c: In function `check_attribute_syntax':
xmlparser.c:5051: warning: comparison is always true due to limited range of
data type
xmlparser.c:5075: warning: comparison is always true due to limited range of
data type
xmlparser.c: At top level:
xmlparser.c:7: warning: `vcid' defined but not used
and
input.c: In function `translate_latin':
input.c:458: warning: label `more_bytes' defined but not used
input.c: In function `translate_latin1':
input.c:486: warning: label `more_bytes' defined but not used
input.c: In function `external_reader':
input.c:672: warning: `trans' might be used uninitialized in this function
Fix these and send a patch upstream if/when you get a chance, it is just a
matter of correct casting.
Cheers,
--
Rafael