haskell / unix

POSIX functionality
https://hackage.haskell.org/package/unix
Other
107 stars 92 forks source link

Work around missing TABX defines #263

Closed blackgnezdo closed 1 year ago

Bodigrim commented 1 year ago

For any system, which was able to compile unix-2.8 before, this change is a non-breaking no-op. Systems like OpenBSD were not able to compile unix-2.8 at all, so the change cannot break anything (because everything has been already broken). Thus I think this does not require any further compatibility layers.

Even if some future versions of OpenBSD become POSIX compliant, it does not hurt to be compatible with not-so-bleeding-edge releases.

blackgnezdo commented 1 year ago

I suspect termios.h provides these. Which likely means that lack of HAVE_TERMIOS_H means no TABX either.

Bodigrim commented 1 year ago

@hs-viktor how does it look?

Bodigrim commented 1 year ago

@vdukhovni ping.

vdukhovni commented 1 year ago

If we're changing this, I'd very much want to see this move from an ADT to a set of Pattern Synonyms:

-- | Output Mode bit fields
newtype TerminalMode = TerminalMode CUInt
pattern TAB0 :: TerminalMode
#ifdef TAB0
pattern TAB0 = TerminalMode (#const TAB0)
#else
pattern TAB0 = 0
#endif
pattern GeneralMode :: CUInt -> TerminalMode
pattern GeneralMode n = TerminalMode n
...
instance Semigroup TerminalMode where
  (TerminalMode a) <> (TerminalMode b) = TerminalMode (a .|. b)
instance Monoid TerminalMode where
  mempty = TerminalMode 0

hasModes :: TerminalMode -> TerminalMode -> Bool
hasModes (TerminalMode x) (TerminalMode m) = x /= 0 && (x .&. m) == x
...

This is extensible without incompatible appearance of new constructors that causes incomplete pattern matches, deals with systems that return bits not known at compile time, and these values are more simply converted to the underlying integral values and used in various functions. The only thing that's a bit harder is Show and Read instances, but this is quite manageable.

So I'd much rather see this migrate to PatternSynonyms than dig the hole deeper.

Bodigrim commented 1 year ago

Pattern synonyms are even better, because the set of constructors will remain unconditional independently of a platform. @blackgnezdo do you have time to take a stab? Or maybe @vdukhovni would like to give it a go?

blackgnezdo commented 1 year ago

If we're changing this, I'd very much want to see this move from an ADT to a set of Pattern Synonyms:

-- | Output Mode bit fields
newtype TerminalMode = TerminalMode CUInt
pattern TAB0 :: TerminalMode
#ifdef TAB0
pattern TAB0 = TerminalMode (#const TAB0)
#else
pattern TAB0 = 0
#endif
pattern GeneralMode :: CUInt -> TerminalMode
pattern GeneralMode n = TerminalMode n
...
instance Semigroup TerminalMode where
  (TerminalMode a) <> (TerminalMode b) = TerminalMode (a .|. b)
instance Monoid TerminalMode where
  mempty = TerminalMode 0

hasModes :: TerminalMode -> TerminalMode -> Bool
hasModes (TerminalMode x) (TerminalMode m) = x /= 0 && (x .&. m) == x
...

This is extensible without incompatible appearance of new constructors that causes incomplete pattern matches, deals with systems that return bits not known at compile time, and these values are more simply converted to the underlying integral values and used in various functions. The only thing that's a bit harder is Show and Read instances, but this is quite manageable.

So I'd much rather see this migrate to PatternSynonyms than dig the hole deeper.

This does look appealing but not without complications. The withMode/withoutMode API hides the fact that these bits are spread across multiple underlying termios fields. At the very least TerminalMode representation should express that the underlying struct contains c_[lcio]flag. I could concoct something, but maybe people have opinions about the matter?

There's also this little matter that NoFlushOnInterrupt is inverted compared to the rest and will require special magic of some sort.

blackgnezdo commented 1 year ago

Another consideration is OpenBSD 7.3 will have TAB[03] defines. So we will be able to add the OpenBSD CI within 6 months by itself. At least the tests pass on the current snapshot of the OS.

Bodigrim commented 1 year ago

I'm very keen to enable OpenBSD CI job asap, without waiting for future OpenBSD 7.3 to be available from Cirrus, so that we do not miss other potential regressions. It does not look like anyone has energy to pursue a solution via pattern synonyms, so I suggest we merge as is.

@vdukhovni ?

Bodigrim commented 1 year ago

@hs-viktor @vdukhovni just another ping.

vdukhovni commented 1 year ago

@hs-viktor @vdukhovni just another ping.

Noted, thanks. I'll make some time by Sunday at the latest.

vdukhovni commented 1 year ago

Given how small this PR is, and that the migration to PatternSynonyms is non-trivial, given the unfortunate representation we have now, I'm approving this PR as-is at present. The PatternSynonym work will have to be separate.