Re: testsuite portability nit
Jim Meyering [EMAIL PROTECTED] writes:
I wouldn't have objected to the original patch,
OK, then I installed the following, which I hope implements all the
spirit of the original patch (though it differs in some unimportant
details.
there is precedent for using $BUILD_SRC_DIR.
It shouldn't be needed here, though, right? The code is already using
$pwd/../../src for that purpose.
2005-04-20 Paul Eggert [EMAIL PROTECTED]
Port test cases to Microsoft-Windows-related environments,
following suggestions from Eric Blake.
* tests/install/Makefile.am (TESTS_ENVIRONMENT): Add EXEEXT.
* tests/install/basic-1: Undo previous change.
(dd, dd2): New vars, which use $EXEEXT. All uses of dd and dd2 changed.
* tests/install/trap: Undo previous change.
(sig): New var. Use it insted of trap '' CHLD.
Append $EXEEXT to executable name.
Index: tests/install/Makefile.am
===
RCS file: /fetish/cu/tests/install/Makefile.am,v
retrieving revision 1.7
diff -p -u -r1.7 Makefile.am
--- tests/install/Makefile.am 10 May 2004 15:13:45 - 1.7
+++ tests/install/Makefile.am 20 Apr 2005 23:44:58 -
@@ -4,4 +4,5 @@ AUTOMAKE_OPTIONS = 1.3 gnits
TESTS = trap basic-1 create-leading
EXTRA_DIST = $(TESTS)
TESTS_ENVIRONMENT = \
+ EXEEXT='$(EXEEXT)' \
PATH=`pwd`/../../src$(PATH_SEPARATOR)$$PATH
Index: tests/install/basic-1
===
RCS file: /fetish/cu/tests/install/basic-1,v
retrieving revision 1.14
diff -p -u -r1.14 basic-1
--- tests/install/basic-1 18 Apr 2005 18:48:22 - 1.14
+++ tests/install/basic-1 20 Apr 2005 23:44:58 -
@@ -8,13 +8,6 @@ fi
dir=dir
file=file
-# Skip this test if we're on a non-POSIX platform where executables' names end
-# in .exe, or we have some other problem like that.
-cat ../../src/dd /dev/null || {
- echo 2 $0: ../../src/dd is not readable, so can't run this test
- exit 77
-}
-
pwd=`pwd`
tmp=inst-basic.$$
trap 'status=$?; cd $pwd; rm -rf $tmp exit $status' 0
@@ -41,10 +34,12 @@ test -f $file || fail=1
test -f $dir/$file || fail=1
# Make sure strip works.
-cp ../../../src/dd .
-cp dd dd2
+dd=dd$EXEEXT
+dd2=dd2$EXEEXT
+cp ../../../src/$dd . || fail=1
+cp $dd $dd2 || fail=1
-strip dd2 || \
+strip $dd2 || \
{
cat 12 EOF
$0: WARNING!!!
@@ -56,12 +51,12 @@ EOF
# This test would fail with 3.16s when using versions of strip that
# don't work on read-only files (the one from binutils works fine).
-ginstall -s -c -m 555 dd $dir || fail=1
+ginstall -s -c -m 555 $dd $dir || fail=1
# Make sure the source file is still around.
-test -f dd || fail=1
+test -f $dd || fail=1
# Make sure that the destination file has the requested permissions.
-set X `ls -l $dir/dd`
+set X `ls -l $dir/$dd`
shift
test $1 = -r-xr-xr-x || fail=1
Index: tests/install/trap
===
RCS file: /fetish/cu/tests/install/trap,v
retrieving revision 1.2
diff -p -u -r1.2 trap
--- tests/install/trap 18 Apr 2005 06:35:22 - 1.2
+++ tests/install/trap 20 Apr 2005 23:44:58 -
@@ -7,12 +7,6 @@ if test $VERBOSE = yes; then
ginstall --version
fi
-(trap '' CHLD) || {
- echo 2 $0: the shell command \trap '' CHLD\ does not work, \
-so can't run this test
- exit 77
-}
-
pwd=`pwd`
t0=`echo $0|sed 's,.*/,,'`.tmp; tmp=$t0/$$
trap 'status=$?; cd $pwd; chmod -R u+rwx $t0; rm -rf $t0 exit $status' 0
@@ -29,8 +23,10 @@ fi
fail=0
+# ash doesn't support trap '' CHLD; it knows only signal numbers.
+sig=`$pwd/../../src/kill -l CHLD 2/dev/null` trap '' $sig
+
# Before 2004-04-21, install would infloop, in the `while (wait...' loop:
-trap '' CHLD
-ginstall -s $pwd/../../src/ginstall .
+ginstall -s $pwd/../../src/ginstall$EXEEXT .
(exit $fail); exit $fail
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: testsuite portability nit
Paul Eggert [EMAIL PROTECTED] wrote: Eric Blake [EMAIL PROTECTED] writes: What was wrong with my proposed patch? I had some qualms with it, because it added coupling between the test cases and the rest of the code, by propagating BUILD_SRC_DIR and EXEEXT from the latter to the former. Coupling like that places extra ... Jim may have other opinions, though. Perhaps he'd prefer the approach of the original patch, in which case we should install that instead. I wouldn't have objected to the original patch, but do see that it would introduce the first use of $EXEEXT in a test script. However, there is precedent for using $BUILD_SRC_DIR. And since there are so few tests for install, I think it's a little friendlier to the other-OS crowd not to skip those tests. Who knows... maybe this test will someday help someone developing on a non-Unix-like system to submit a better patch to install.c. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: testsuite portability nit
I installed the following patch, in the hopes that it'd be relatively
simple and easy to maintain. It skip the tests on the platforms with
the contrary-to-POSIX glitches.
2005-04-17 Paul Eggert [EMAIL PROTECTED]
Work around a couple of make check failures reported for Cygwin
and ash by Eric Blake.
* tests/install/basic-1: Skip this test if ../../src/dd isn't readable.
* tests/install/trap: Skip this test if trap '' CHLD doesn't work.
--- basic-1 11 Aug 2004 23:41:59 - 1.11
+++ basic-1 18 Apr 2005 06:35:06 - 1.12
@@ -8,6 +8,11 @@ fi
dir=dir
file=file
+test -r ../../src/dd || {
+ echo 2 $0: ../../src/dd is not readable, so can't run this test
+ exit 77
+}
+
pwd=`pwd`
tmp=inst-basic.$$
trap 'status=$?; cd $pwd; rm -rf $tmp exit $status' 0
--- trap10 May 2004 15:13:29 - 1.1
+++ trap18 Apr 2005 06:35:22 - 1.2
@@ -7,6 +7,12 @@ if test $VERBOSE = yes; then
ginstall --version
fi
+(trap '' CHLD) || {
+ echo 2 $0: the shell command \trap '' CHLD\ does not work, \
+so can't run this test
+ exit 77
+}
+
pwd=`pwd`
t0=`echo $0|sed 's,.*/,,'`.tmp; tmp=$t0/$$
trap 'status=$?; cd $pwd; chmod -R u+rwx $t0; rm -rf $t0 exit $status' 0
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: testsuite portability nit
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Paul Eggert on 4/18/2005 12:39 AM: I installed the following patch, in the hopes that it'd be relatively simple and easy to maintain. It skip the tests on the platforms with the contrary-to-POSIX glitches. 2005-04-17 Paul Eggert [EMAIL PROTECTED] Work around a couple of make check failures reported for Cygwin and ash by Eric Blake. * tests/install/basic-1: Skip this test if ../../src/dd isn't readable. * tests/install/trap: Skip this test if trap '' CHLD doesn't work. What was wrong with my proposed patch? It made both of these tests PASS instead of SKIP on cygwin, always a good goal in testsuite coverage. And my patch to tests/install/trap was shorter than your patch to skip it, so I argue that it was simple and easy enough to maintain. Furthermore, your patch to tests/install/basic-1 does not solve the problem: `test -r ../../src/dd' succeeds on cygwin because of .exe magic built in to cygwin stat(2); the failure comes later when open(2) fails after the stat(2) succeeded because the filename is missing the extension (in cp(1) if I have not applied my cygwin .exe patches to cp yet, otherwise in strip(1) called by ginstall). Propagating $EXEEXT to the testsuite completely works around half-baked .exe magic in cygwin by explicitly specifying it up front instead of relying on cygwin magic to determine when it is needed. Under your patch: $ make -C tests/install check TESTS=basic-1 VERBOSE=yes make: Entering directory `/home/eblake/coreutils/tests/install' make check-TESTS make[1]: Entering directory `/home/eblake/coreutils/tests/install' + ginstall --version install (GNU coreutils) 5.3.1 Written by David MacKenzie. Copyright (C) 2005 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + dir=dir + file=file + test -r ../../src/dd + pwd + pwd=/home/eblake/coreutils/tests/install + tmp=inst-basic.3840 + trap status=$?; cd $pwd; rm -rf $tmp exit $status 0 + trap exit $? 1 2 13 15 + framework_failure=0 + mkdir inst-basic.3840 + cd inst-basic.3840 + rm -rf dir file + mkdir -p dir + echo foo + test 0 = 1 + fail=0 + ginstall file dir + test -f file + test -f dir/file + cp ../../../src/dd . cp: cannot open `../../../src/dd' for reading: No such file or directory + cp dd dd2 cp: cannot stat `dd': No such file or directory + strip dd2 strip: 'dd2': No such file + ginstall -s -c -m 555 dd dir ginstall: cannot stat `dd': No such file or directory + fail=1 + test -f dd + fail=1 + ls -l dir/dd ls: dir/dd: No such file or directory + set X + shift + test = -r-xr-xr-x + fail=1 + ginstall -d . + ginstall -d newdir + ginstall -d newdir1 newdir2 newdir3 + exit 1 + exit 1 + status=1 + cd /home/eblake/coreutils/tests/install + rm -rf inst-basic.3840 + exit 1 FAIL: basic-1 == 1 of 1 tests failed Please report to bug-coreutils@gnu.org == make[1]: *** [check-TESTS] Error 1 make[1]: Leaving directory `/home/eblake/coreutils/tests/install' make: *** [check-am] Error 2 make: Leaving directory `/home/eblake/coreutils/tests/install' Under my patch: $ make -C tests/install check TESTS=basic-1 VERBOSE=yes make: Entering directory `/home/eblake/coreutils/tests/install' make check-TESTS make[1]: Entering directory `/home/eblake/coreutils/tests/install' + ginstall --version install (GNU coreutils) 5.3.1 Written by David MacKenzie. Copyright (C) 2005 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + dir=dir + file=file + test -r ../../src/dd + pwd + pwd=/home/eblake/coreutils/tests/install + tmp=inst-basic.2796 + trap status=$?; cd $pwd; rm -rf $tmp exit $status 0 + trap exit $? 1 2 13 15 + framework_failure=0 + mkdir inst-basic.2796 + cd inst-basic.2796 + rm -rf dir file + mkdir -p dir + echo foo + test 0 = 1 + fail=0 + ginstall file dir + test -f file + test -f dir/file + cp ../../../src/dd.exe . + cp dd.exe dd2.exe + strip dd2.exe + ginstall -s -c -m 555 dd.exe dir + test -f dd.exe + ls -l dir/dd.exe + set X -r-xr-xr-x 1 eblake None 39424 Apr 18 06:55 dir/dd.exe + shift + test -r-xr-xr-x = -r-xr-xr-x + ginstall -d . + ginstall -d newdir + ginstall -d newdir1 newdir2 newdir3 + exit 0 + exit 0 + status=0 + cd /home/eblake/coreutils/tests/install + rm -rf inst-basic.2796 + exit 0 PASS: basic-1 == All 1 tests passed == make[1]: Leaving directory `/home/eblake/coreutils/tests/install' make: Leaving directory `/home/eblake/coreutils/tests/install' - -- Life is short - so eat dessert first! Eric Blake [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.0 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using
Re: testsuite portability nit
Eric Blake [EMAIL PROTECTED] writes:
What was wrong with my proposed patch?
I had some qualms with it, because it added coupling between the test
cases and the rest of the code, by propagating BUILD_SRC_DIR and
EXEEXT from the latter to the former. Coupling like that places extra
constraints on future mainstream development, and makes it harder for
others to figure out what's going on in the test cases. The extra
cost is relatively small here, but the benefit is small as well, and
in my judgment the tradeoff wasn't quite worth it.
Looking forward, I'd rather have the Microsoft-Windows-specific stuff
be relatively isolated -- ideally, in a separate subdirectory where
the mainstream developers don't have to worry about it. We're not at
this point with coreutils, but I'd rather not make it harder to head
in that direction.
Jim may have other opinions, though. Perhaps he'd prefer the approach
of the original patch, in which case we should install that instead.
`test -r ../../src/dd' succeeds on cygwin because of .exe magic
built in to cygwin stat(2); the failure comes later when open(2)
fails after the stat(2) succeeded
Sorry, I wasn't aware of the complexity of this particular problem.
I installed the following patch to work around this glitch.
2005-04-18 Paul Eggert [EMAIL PROTECTED]
* tests/install/basic-1: Use cat, not test, to test for
../../src/dd. Problem reported by Eric Blake.
--- basic-1 18 Apr 2005 06:35:06 - 1.12
+++ basic-1 18 Apr 2005 18:48:22 - 1.14
@@ -8,7 +8,9 @@ fi
dir=dir
file=file
-test -r ../../src/dd || {
+# Skip this test if we're on a non-POSIX platform where executables' names end
+# in .exe, or we have some other problem like that.
+cat ../../src/dd /dev/null || {
echo 2 $0: ../../src/dd is not readable, so can't run this test
exit 77
}
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
testsuite portability nit
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On cygwin, where /bin/sh is ash, trap '' CHLD doesn't work (ash only
accepts signal numbers, not names):
$ trap '' CHLD
trap: bad signal CHLD
But CHLD is not one of the portable signal numbers (it is 20 on cygwin,
but other numbers on other platforms). Not all versions of kill(1) can
convert names to numbers, but this is coreutils.
Furthermore, install needs to use $EXEEXT on arguments.
2005-04-16 Eric Blake [EMAIL PROTECTED]
* tests/install/Makefile.am (TESTS_ENVIRONMENT): Propagate
BUILD_SRC_DIR and EXEEXT to tests.
* tests/install/basic-1: Use EXEEXT.
* tests/install/trap: trap '' CHLD is not portable on
Cygwin. Also, use EXEEXT.
- --
Life is short - so eat dessert first!
Eric Blake [EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.0 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD8DBQFCYZM784KuGfSFAYARAvu1AJ4u2qNQIlvTiVMz3OYnS/QeomZhYACfUoWe
DyToYY44Z2r+NZeKCoXNUr4=
=/O1E
-END PGP SIGNATURE-
Index: tests/install/Makefile.am
===
RCS file: /cvsroot/coreutils/coreutils/tests/install/Makefile.am,v
retrieving revision 1.7
diff -u -p -r1.7 Makefile.am
--- tests/install/Makefile.am 10 May 2004 15:13:45 - 1.7
+++ tests/install/Makefile.am 16 Apr 2005 22:33:13 -
@@ -4,4 +4,6 @@ AUTOMAKE_OPTIONS = 1.3 gnits
TESTS = trap basic-1 create-leading
EXTRA_DIST = $(TESTS)
TESTS_ENVIRONMENT = \
- PATH=`pwd`/../../src$(PATH_SEPARATOR)$$PATH
+ PATH=`pwd`/../../src$(PATH_SEPARATOR)$$PATH \
+ EXEEXT=$(EXEEXT) \
+ BUILD_SRC_DIR=`pwd`/../../src
Index: tests/install/Makefile.in
===
RCS file: /cvsroot/coreutils/coreutils/tests/install/Makefile.in,v
retrieving revision 1.139
diff -u -p -r1.139 Makefile.in
Index: tests/install/basic-1
===
RCS file: /cvsroot/coreutils/coreutils/tests/install/basic-1,v
retrieving revision 1.11
diff -u -p -r1.11 basic-1
--- tests/install/basic-1 11 Aug 2004 23:41:59 - 1.11
+++ tests/install/basic-1 16 Apr 2005 22:33:13 -
@@ -34,10 +34,10 @@ test -f $file || fail=1
test -f $dir/$file || fail=1
# Make sure strip works.
-cp ../../../src/dd .
-cp dd dd2
+cp ../../../src/dd${EXEEXT} .
+cp dd${EXEEXT} dd2${EXEEXT}
-strip dd2 || \
+strip dd2${EXEEXT} || \
{
cat 12 EOF
$0: WARNING!!!
@@ -49,12 +49,12 @@ EOF
# This test would fail with 3.16s when using versions of strip that
# don't work on read-only files (the one from binutils works fine).
-ginstall -s -c -m 555 dd $dir || fail=1
+ginstall -s -c -m 555 dd${EXEEXT} $dir || fail=1
# Make sure the source file is still around.
-test -f dd || fail=1
+test -f dd${EXEEXT} || fail=1
# Make sure that the destination file has the requested permissions.
-set X `ls -l $dir/dd`
+set X `ls -l $dir/dd${EXEEXT}`
shift
test $1 = -r-xr-xr-x || fail=1
Index: tests/install/trap
===
RCS file: /cvsroot/coreutils/coreutils/tests/install/trap,v
retrieving revision 1.1
diff -u -p -r1.1 trap
--- tests/install/trap 10 May 2004 15:13:29 - 1.1
+++ tests/install/trap 16 Apr 2005 22:33:13 -
@@ -24,7 +24,8 @@ fi
fail=0
# Before 2004-04-21, install would infloop, in the `while (wait...' loop:
-trap '' CHLD
-ginstall -s $pwd/../../src/ginstall .
+# neither trap '' CHLD nor trap '' `kill -l CHLD` is portable
+trap '' `${BUILD_SRC_DIR?}/kill -l CHLD`
+ginstall -s $pwd/../../src/ginstall${EXEEXT} .
(exit $fail); exit $fail
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: testsuite portability nit
On Sat, Apr 16, 2005 at 04:35:39PM -0600, Eric Blake wrote: On cygwin, where /bin/sh is ash, trap '' CHLD doesn't work (ash only accepts signal numbers, not names): $ trap '' CHLD trap: bad signal CHLD But CHLD is not one of the portable signal numbers (it is 20 on cygwin, but other numbers on other platforms). Not all versions of kill(1) can convert names to numbers, but this is coreutils. The names should be used instead of numbers. That is, the existing usage seems to be the right thing to do; it's what POSIX mangates nowadays. See http://www.opengroup.org/onlinepubs/009695399/utilities/trap.html. Regards, James. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: testsuite portability nit
On cygwin, where /bin/sh is ash, trap '' CHLD doesn't work (ash only accepts signal numbers, not names): $ trap '' CHLD trap: bad signal CHLD But CHLD is not one of the portable signal numbers (it is 20 on cygwin, but other numbers on other platforms). Not all versions of kill(1) can convert names to numbers, but this is coreutils. The names should be used instead of numbers. That is, the existing usage seems to be the right thing to do; it's what POSIX mangates nowadays. See http://www.opengroup.org/onlinepubs/009695399/utilities/trap.html. I'm in total agreement that POSIX requires support for names, but my argument was that cygwin's /bin/sh (and any other platform that uses ash or other non-POSIX shells for /bin/sh) does not support names. Look at the screenshot you quoted from my original mail. Hence, my proposed patch is the only 'portable' way I could come up with to supply the number to trap; and even that is not portable outside of the coreutils testsuite, because it relies on coreutils kill(1) to do the translation from name to number. See my query to the autoconf list a while back: http://lists.gnu.org/archive/html/autoconf/2005-01/msg00136.html -- Eric Blake ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
