Re: coreutils-8.12.193-d8dc8: 2 tests skipped: failed to create ext2 file system

[Jim Meyering]

Bernhard Voelker wrote:

> On 09/06/2011 10:58 PM, Jim Meyering wrote:
>> Bernhard Voelker wrote:
>>> These 2 root checks fail because my USER doesn't have /sbin nor
>>> /usr/sbin in PATH.
>>
>> I'd say they were skipped, not failed :-)
>
> +1 for you ;-)
>
>>> SKIP: cp/cp-mv-enotsup-xattr (exit: 77)
>>> =======================================
>>>
>>> + dd if=/dev/zero of=noxattr.bin bs=8192 count=200
>>> + mkdir noxattr
>>> + mkfs -t ext2 -F noxattr.bin
>>> ./cp/cp-mv-enotsup-xattr: line 41: mkfs: command not found
>>> + skip_ 'failed to create ext2 file system'
>>> + warn_ 'cp-mv-enotsup-xattr: skipped test: failed to create ext2 file 
>>> system'
>>>
>>> SKIP: rm/read-only (exit: 77)
>>> =============================
>>> ...
>>> + dd if=/dev/zero of=blob bs=8192 count=200
>>> + mkdir mnt
>>> + mkfs -t ext2 -F blob
>>> ./rm/read-only: line 31: mkfs: command not found
>>> + skip_ 'failed to create ext2 file system'
>>> + warn_ 'read-only: skipped test: failed to create ext2 file system'
>>>
>>>
>>> If started with
>>>    sudo env PATH="/sbin:$PATH" NON_ROOT_USERNAME=$USER make -k check-root
>>> then they pass.
>>>
>>> Should we
>>> * add /sbin to PATH for the whole check-root test suite, or
>>> * add it only for these 2 tests, or
>>> * enhance the README file for this issue?
>>
>> IMHO, having a usable PATH is a prerequisite.
>> If root's default search path lacks /sbin, you may want to
>> take that up with the folks who manage your distribution.
>> It's something that affects many packages, not just coreutils.
>
> It's not root's PATH which lacks /sbin, but the PATH of a non-root user.
> Running `sudo env PATH="$PATH" NON_ROOT_USERNAME=$USER make -k check-root`
> started as such non-root user will therefore also miss /sbin.

It's the PATH you get for a sudo-to-root shell.
I've adjusted your comment in init.cfg and will also
adjust the one in the commit log.

>> Saying something like that in README would be sort of like saying you must
>> have a user name.  While it is possible to do a lot of things with just
>> a numeric user ID, many tools require that you also have a corresponding
>> user name.  It's so basic that no one should have to bother to check or
>> warn about the possibility that the assumption does not hold.
>>
>> Skipping a few tests is not a problem.
>>
>> In spite of all that, I think that enough people would skip these tests
>> that it's worth addressing.  In case you're interested, here's the sort
>> of patch I'd like:
>>
>> Add a function in tests/init.cfg named something like
>> require_mkfs_PATH_ that does this:
>>
>>    if there is no mkfs in PATH
>>      if PATH does not contain /sbin
>>        if /sbin/mkfs is executable
>>          PATH=$PATH:/sbin
>>
>> Then, invoke that new function from each of the two tests above
>> to make it less likely that they'll be skipped.
>
>
> I had to add the new function to a few more tests which are skipped
> on my system due to other reasons (e.g. missing selinux support).

Thanks.
Here are some minor changes I expect to merge into your commit.
I'll post a complete patch for your ACK before I push it.

(btw, your patch did not apply via "git am FILE"
due to fuzz in the init.cfg patch, but I managed
by applying with patch, pasting your log into ChangeLog
and running vc-dwim --commit.  However that failed
due to a typo fixed via s/writeable/writable.)

diff --git a/tests/init.cfg b/tests/init.cfg
index bbd576c..4565907 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -180,18 +180,17 @@ uid_is_privileged_()
   esac
 }

-# Some distributions do not add /sbin to PATH for non-root users.
+# Some distributions do not include /sbin in the sudo-to-root PATH.
 # Test if mkfs is in PATH, otherwise try to adapt PATH.
 require_mkfs_PATH_()
 {
-  which mkfs >/dev/null 2>&1 \
-    && return
+  type mkfs && return

   case ":$PATH:" in
     *:/sbin:*) skip_ "no usable mkfs found" ;;
   esac

-  [ -x /sbin/mkfs ] \
+  test -x /sbin/mkfs \
     || skip_ "no usable mkfs found"

   PATH="$PATH:/sbin"

> Subject: [PATCH] tests: add mkfs to PATH for some check-root tests.
>
> tests/init.cfg (require_mkfs_PATH_): New function to test whether mkfs
> is in PATH, otherwise adding /sbin to PATH. Needed for distributions
> (OpenSuSE, Solaris) which do not add /sbin to PATH for non-root users.
> tests/cp/cp-a-selinux: Use require_mkfs_PATH_.
> tests/cp/cp-mv-enotsup-xattr: Likewise.
> tests/cp/sparse-fiemap: Likewise.
> tests/mkdir/writeable-under-readonly: Likewise.
> tests/rm/read-only: Likewise.