jtdaugherty / vty

A high-level ncurses alternative written in Haskell
BSD 3-Clause "New" or "Revised" License
319 stars 57 forks source link

`test:verify-terminal` fails to build with `unix-2.8.*` #257

Closed RyanGlScott closed 1 year ago

RyanGlScott commented 1 year ago

test:verify-terminal fails to build with unix-2.8.1.0 (bundled with GHC 9.6.1):

$ cabal test verify-terminal -w ghc-9.6
Build profile: -w ghc-9.6.1 -O1
In order, the following will be built (use -v for more details):
 - vty-tests-5.37 (test:verify-terminal) (first run)
Preprocessing test suite 'verify-terminal' for vty-tests-5.37..
Building test suite 'verify-terminal' for vty-tests-5.37..
[1 of 1] Compiling VerifyOutput     ( programs/VerifyOutput.hs, /home/rscott/Documents/Hacking/Haskell/vty/dist-newstyle/build/x86_64-linux/ghc-9.6.1/vty-tests-5.37/t/verify-terminal/build/verify-terminal/VerifyOutput.o, /home/rscott/Documents/Hacking/Haskell/vty/dist-newstyle/build/x86_64-linux/ghc-9.6.1/vty-tests-5.37/t/verify-terminal/build/verify-terminal/VerifyOutput.dyn_o )

programs/VerifyOutput.hs:37:16: error: [GHC-83865]
    • Couldn't match expected type: OpenFileFlags
                                    -> IO System.Posix.Types.Fd
                  with actual type: IO System.Posix.Types.Fd
    • The function ‘openFd’ is applied to four value arguments,
        but its type ‘FilePath
                      -> OpenMode -> OpenFileFlags -> IO System.Posix.Types.Fd’
        has only three
      In a stmt of a 'do' block:
        nullOut <- openFd "/dev/null" WriteOnly Nothing defaultFileFlags
      In the expression:
        do nullOut <- openFd "/dev/null" WriteOnly Nothing defaultFileFlags
           t <- outputForConfig
                  $ defaultConfig
                      {outputFd = Just nullOut, termName = Just termName,
                       colorMode = Just NoColor}
           reserveDisplay t
           dc <- displayContext t (100, 100)
           ....
   |
37 |     nullOut <- openFd "/dev/null" WriteOnly Nothing defaultFileFlags
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

programs/VerifyOutput.hs:37:45: error: [GHC-83865]
    • Couldn't match expected type ‘OpenFileFlags’
                  with actual type ‘Maybe a0’
    • In the third argument of ‘openFd’, namely ‘Nothing’
      In a stmt of a 'do' block:
        nullOut <- openFd "/dev/null" WriteOnly Nothing defaultFileFlags
      In the expression:
        do nullOut <- openFd "/dev/null" WriteOnly Nothing defaultFileFlags
           t <- outputForConfig
                  $ defaultConfig
                      {outputFd = Just nullOut, termName = Just termName,
                       colorMode = Just NoColor}
           reserveDisplay t
           dc <- displayContext t (100, 100)
           ....
   |
37 |     nullOut <- openFd "/dev/null" WriteOnly Nothing defaultFileFlags
   |                                             ^^^^^^^

This is because the type of unix's openFd function changed in unix-2.8.0.0 to remove the Maye FileMode argument, instead making it a part of the OpenFileFlags argument. (See the changelog.)

I can see two different ways to fix this issue:

  1. Guard the use of Nothing with !MIN_VERSION_unix(2,8,0).
  2. Replace the use of unix's openFd with base' openFile. As far as I can tell, nothing about this code requires anything that openFile couldn't give you.

Which would you prefer?

jtdaugherty commented 1 year ago

If I had to pick, I'd pick (2). By your question it sounds like you might be offering to make the change. Is that the case? (If so, thank you!)

jtdaugherty commented 1 year ago

(Also, incidentally, anything we can do to move further away from using unix is a good move in my book since support for Windows is getting some attention and any unix-isms in the codebase are going to make that harder, so this is on my mind.)

RyanGlScott commented 1 year ago

Sounds good—I will submit a patch implementing (2) shortly. Note that this doesn't really get us away from Unix conventions because we will still be using /dev/null, which doesn't exist on Windows. That being said, it's fairly simple to paper over it should you want to support Windows later, as demonstrated here.

RyanGlScott commented 1 year ago

Ah, never mind, option (2) isn't as straightforward as I thought it would be. This is because vty's API directly uses unix's Fd, as in here:

https://github.com/jtdaugherty/vty/blob/7475b75cfc9a6be7c1964b57a381bcb237c9c9d0/src/Graphics/Vty/Config.hs#L176-L179

We could try to remove those uses of Fd, but that would turn this into a much larger patch than what I originally had in mind. As such, I'll go with (1) instead.

jtdaugherty commented 1 year ago

Okay, thanks. (Yeah, the Fd usage is a problem for Windows as I'm discussing with others elsewhere.)