This is the mail archive of the cygwin-apps@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Review - Not Yet Good to Go] rxp 1.3.0-1: Simple Validating XML parser (long)


*** 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]