Fix struct dirent d_type macro test #348

sjshuck · 2025-07-03T22:36:08Z

Fixes #347. Credit to Andrew Gunnerson (@chenxiaolong).

Test program:

module Main (main) where

import Control.Exception                (bracket)
import Data.Function                    (fix)
import Foreign.C.String                 (peekCString)
import System.Posix.Directory
import System.Posix.Directory.Internals
import Text.Printf                      (printf)

printDirEnt :: DirEnt -> IO ()
printDirEnt dirEnt = do
    dName <- dirEntName dirEnt >>= peekCString
    dType <- dirEntType dirEnt
    printf "%-20s%s\n" dName (show dType)

main :: IO ()
main = bracket (openDirStream "/home/steve/foo") closeDirStream $ \dirStream ->
    fix $ \go -> readDirStreamWith printDirEnt dirStream >>= mapM_ (\_ -> go)

Output for unix-2.8.7.0:

.                   DirType 0
..                  DirType 0
a regular file.txt  DirType 0
a directory.d       DirType 0
a symlink           DirType 0

Output with this change:

.                   DirType 4
..                  DirType 4
a regular file.txt  DirType 8
a directory.d       DirType 4
a symlink           DirType 10

@chenxiaolong

Credit to Andrew Gunnerson (@chenxiaolong)

Bodigrim · 2025-07-03T22:47:57Z

@sjshuck could you add a test please?

sjshuck · 2025-07-03T23:24:16Z

I'm a little stuck on how to skip the test if d_type isn't defined or if the filesystem doesn't set non-zero values for it. Open to suggestions.

Do all test containers have a cc installed on them?

Bodigrim · 2025-07-03T23:43:25Z

Do all test containers have a cc installed on them?

I think so, these are generic CI machines, not bare minimum containers.

Is #ifdef HAVE_STRUCT_DIRENT_D_TYPE inaccessible in the test suite?

sjshuck · 2025-07-04T01:45:46Z

Is #ifdef HAVE_STRUCT_DIRENT_D_TYPE inaccessible in the test suite?

I'm not sure TBH. I don't know how standard of a macro that is, how much it has to do with GNU, Linux, ... I did the test another way, compiling optimistically with d_type, and if the compilation fails, I'm assuming it's because that isn't defined, and the Haskell test is skipped. I could scrape stderr and find mention of "d_type" in it to be extra sure, I suppose.

hasufell · 2025-07-04T04:00:56Z

tests/DirEnt.c

+    }
+}
+
+int main() {


I don't understand the purpose of this. We check for headers and library functions in configure.ac

unix/configure.ac

Lines 30 to 66 in eced23b

AC_STRUCT_DIRENT_D_TYPE

FP_CHECK_CONST([DT_UNKNOWN], [

#if HAVE_STRUCT_DIRENT_D_TYPE

#include <dirent.h>

#endif], [-1])

FP_CHECK_CONST([DT_FIFO], [

#if HAVE_STRUCT_DIRENT_D_TYPE

#include <dirent.h>

#endif], [-2])

FP_CHECK_CONST([DT_CHR], [

#if HAVE_STRUCT_DIRENT_D_TYPE

#include <dirent.h>

#endif], [-3])

FP_CHECK_CONST([DT_DIR], [

#if HAVE_STRUCT_DIRENT_D_TYPE

#include <dirent.h>

#endif], [-4])

FP_CHECK_CONST([DT_BLK], [

#if HAVE_STRUCT_DIRENT_D_TYPE

#include <dirent.h>

#endif], [-5])

FP_CHECK_CONST([DT_REG], [

#if HAVE_STRUCT_DIRENT_D_TYPE

#include <dirent.h>

#endif], [-6])

FP_CHECK_CONST([DT_LNK], [

#if HAVE_STRUCT_DIRENT_D_TYPE

#include <dirent.h>

#endif], [-7])

FP_CHECK_CONST([DT_SOCK], [

#if HAVE_STRUCT_DIRENT_D_TYPE

#include <dirent.h>

#endif], [-8])

FP_CHECK_CONST([DT_WHT], [

#if HAVE_STRUCT_DIRENT_D_TYPE

#include <dirent.h>

#endif], [-9])

Explaining in a comment in the code.

hasufell · 2025-07-04T06:40:26Z

tests/DirEnt.c

+ * The purpose of this C program is to do one of 4 things.  Which one it does,
+ * indicates to the DirEnt test suite something it ought to do.
+ *
+ * fail to compile


This can be done in configure.ac via AC_LINK_IFELSE and AC_LANG_PROGRAM, no?

I'm not sure; this is beyond the limit of my C build chain knowledge.

hasufell · 2025-07-04T06:52:06Z

tests/DirEnt.c

+ * exit with status 0
+ *   This means the "." entry in the DIR stream opened at ".", guaranteed to be
+ *   a directory, has a d_type of DT_DIR.  We should proceed with a test for it
+ *   in Haskell.
+ * exit with status 1
+ *   This means something unexpected went wrong.  Fail the Haskell test also.
+ * exit with stauts 2
+ *   This means the "." entry has a d_type of DT_UNKNOWN.  This is valid; no
+ *   filesystem or operating system is required to yield a useful d_type.
+ *   We should skip testing for non-DT_UNKNOWN values in Haskell.


Why can't we do this check in Haskell?

hasufell · 2025-07-04T06:54:15Z

tests/DirEnt.hs

+
+prepareTest :: IO ()
+prepareTest = do
+    system_x "cc --version" `onFailure` exitWith


I think this is highly non-portable. I'd prefer to remove the c-tests altogether, do "static" checks in configure.ac and do additional runtime checks (whether the kernel just returns DT_UNKNOWN) in the Haskell code itself.

If we really want to run C code, we can compile it properly through cabal and use the C FFI to run the appropriate function. I do not understand why we compile manually in the test suite and produce a binary here.

The thing being tested is: When d_type is defined and the filesystem supports non-zero values for it, test that dirEntType works and doesn't return 0 because someone defined it wrong in cbits/HsUnix.c. Because that's what happened. I can't think of any other way to test for that than what I did.

I don't know how autotools works, but intuitively, doing checks in configure.ac is testing a macro with a macro, which doesn't buy us anything. We can't check if the kernel returns DT_UNKNOWN because kernels are always allowed to return two values for any node, DT_THE_ACTUAL_TYPE and DT_UNKNOWN—you can't fix that by configuring the CI image. We need to know what the kernel thinks the d_type is, and check if dirEntType agrees with that, hence the non-Haskell (C) program. We can't compile it via Cabal because it should be allowed to fail to compile, unless there's an "allow-fail" option I'm not aware of. We can guard against compilation failures by doing the appropriate macro test in C, but again, that doesn't buy us anything.

My opinion: #347 is below the threshold of testability. In practice that happens sometimes. However, if we're going to change the thing being tested from what I wrote above to something else, and implement that test, I'm all for it. I just want to know what we're testing. cc @Bodigrim

When d_type is defined and the filesystem supports non-zero values for it,

Yes, these absolutely can be tested for in configure.ac. I think one of my comments got swallowed by GitHub.

https://autotools.info/autoconf/custom-tests.html

Okay, you had alluded to it above. I need to take a break from this for a while.

Meanwhile, something that occurred to me that could actually be the sweet spot for trade-off between complexity and correctness in testing: Assume that for Linux, GitHub CI and Hackage CI always make . have d_type DT_DIR, and test exactly that with dirEntType, and only run it under Linux. If it returns UnknownType or some other value, fail the test noisily, running mount(8), and deal with that when that happens. I bet you it never will. Let me know what you think

Assume that for Linux, GitHub CI and Hackage CI always make . have d_type DT_DIR, and test exactly that with dirEntType, and only run it under Linux. If it returns UnknownType or some other value, fail the test noisily, running mount(8), and deal with that when that happens.

I think it's good enough and strikes a good balance. Although I do not get what mount(8) reference is about.

Cool. When I get a chance I'll make that change. I was talking about dumping mount points to the console to analyze if the CI container had changed to some exotic fs that didn't support non-zero d_types. Although I think I'll use /proc/self's dirent instead of .'s, since that's as guaranteed to fully support d_type as anything.

It's technically a non-portable test and so I think it should be guarded behind a cabal flag. Maybe non-portable-tests?

@hasufell I disabled it for non-Linux in unix.cabal but maybe that's not what you're talking about. I set "Allow edits by maintainers" so please feel free to add any Cabal flags.

…Type /proc/self is guaranteed to exist as a symlink on Linux and procfs has emitted its d_type for decades.

sjshuck · 2025-07-05T13:46:32Z

Temporarily changed back to the incorrect #ifdef HAVE_DIRENT_D_TYPE and the test on my Linux machine fails with

DirEnt of /proc/self has DirType 0; expected DirType 10!

Bodigrim

Awesome!

Fix struct dirent d_type macro test

1f571bc

Credit to Andrew Gunnerson (@chenxiaolong)

sjshuck force-pushed the fix-dirent-d-type branch from a459fc8 to 9eb439b Compare July 4, 2025 02:38

Add test for dirEntType

c18caa0

sjshuck force-pushed the fix-dirent-d-type branch from 9eb439b to c18caa0 Compare July 4, 2025 03:49

hasufell reviewed Jul 4, 2025

View reviewed changes

Explain what tests/DirEnt.c is for

ea507b6

hasufell requested changes Jul 4, 2025

View reviewed changes

Make dirEntType test Linux-only and check /proc/self for SymbolicLink…

0935635

…Type /proc/self is guaranteed to exist as a symlink on Linux and procfs has emitted its d_type for decades.

sjshuck requested review from hasufell and Bodigrim July 5, 2025 13:46

Bodigrim approved these changes Jul 5, 2025

View reviewed changes

sjshuck mentioned this pull request Jul 6, 2025

Make use of dirEntType in sourceDirectoryDeep snoyberg/conduit#527

Open

	AC_STRUCT_DIRENT_D_TYPE
	FP_CHECK_CONST([DT_UNKNOWN], [
	#if HAVE_STRUCT_DIRENT_D_TYPE
	#include <dirent.h>
	#endif], [-1])
	FP_CHECK_CONST([DT_FIFO], [
	#if HAVE_STRUCT_DIRENT_D_TYPE
	#include <dirent.h>
	#endif], [-2])
	FP_CHECK_CONST([DT_CHR], [
	#if HAVE_STRUCT_DIRENT_D_TYPE
	#include <dirent.h>
	#endif], [-3])
	FP_CHECK_CONST([DT_DIR], [
	#if HAVE_STRUCT_DIRENT_D_TYPE
	#include <dirent.h>
	#endif], [-4])
	FP_CHECK_CONST([DT_BLK], [
	#if HAVE_STRUCT_DIRENT_D_TYPE
	#include <dirent.h>
	#endif], [-5])
	FP_CHECK_CONST([DT_REG], [
	#if HAVE_STRUCT_DIRENT_D_TYPE
	#include <dirent.h>
	#endif], [-6])
	FP_CHECK_CONST([DT_LNK], [
	#if HAVE_STRUCT_DIRENT_D_TYPE
	#include <dirent.h>
	#endif], [-7])
	FP_CHECK_CONST([DT_SOCK], [
	#if HAVE_STRUCT_DIRENT_D_TYPE
	#include <dirent.h>
	#endif], [-8])
	FP_CHECK_CONST([DT_WHT], [
	#if HAVE_STRUCT_DIRENT_D_TYPE
	#include <dirent.h>
	#endif], [-9])

Fix struct dirent d_type macro test #348

Are you sure you want to change the base?

Fix struct dirent d_type macro test #348

Conversation

sjshuck commented Jul 3, 2025

Uh oh!

Bodigrim commented Jul 3, 2025

Uh oh!

sjshuck commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bodigrim commented Jul 3, 2025

Uh oh!

sjshuck commented Jul 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjshuck Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjshuck commented Jul 5, 2025

Uh oh!

Bodigrim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sjshuck commented Jul 3, 2025 •

edited

Loading

sjshuck Jul 4, 2025 •

edited

Loading