Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#9945 closed bug (fixed)

export list for System.Posix.Internals breaking the build on Windows

Reported by: MartinF Owned by:
Priority: normal Milestone: 8.0.1
Component: Build System Version: 7.11
Keywords: Cc: dfeuer
Operating System: Windows Architecture: Unknown/Multiple
Type of failure: Building GHC failed Test Case:
Blocked By: Blocking:
Related Tickets: #9852 Differential Rev(s):
Wiki Page:

Description

The export list in System.Posix.Internals includes some names that, thanks to CPP, aren't defined when building on Windows. It was compiling fine a few days ago, so I think D551 was the breaking change.

I attach the make output from the failing step.

Attachments (1)

System.Posix.Internals.hs-error.txt (3.7 KB) - added by MartinF 3 years ago.
the relevant make output showing the errors

Download all attachments as: .zip

Change History (24)

Changed 3 years ago by MartinF

the relevant make output showing the errors

comment:1 Changed 3 years ago by MartinF

I'm thinking the fix should just be a matter of wrapping the relevant names in the export list in CPP conditionals, to match the CPP in the module body - so names are included in the export list IFF they're actually defined. That would restore the old behaviour from before the export list was added, anyway.

I'm working on a patch to that end now. Shouldn't take long.

Does that sound like the right course of action?

comment:2 Changed 3 years ago by MartinF

Owner: set to MartinF

comment:3 Changed 3 years ago by hvr

Cc: dfeuer added

comment:4 Changed 3 years ago by dfeuer

Fun times. I really thought I'd checked the CPP reasonably well, but this is the second failure. The real question is whether these conditionally-defined names are used outside the module, or whether they're intended only for internal use. If the latter, they just shouldn't be exported at all.

comment:5 Changed 3 years ago by MartinF

Good call, I'll check while I'm at it.

I've already discovered is_console was missing from the export list entirely, but CPP-conditionally used in GHC.IO.FD.isTerminal:

isTerminal :: FD -> IO Bool
isTerminal fd =
#if defined(mingw32_HOST_OS)
    if fdIsSocket fd then return False
                     else is_console (fdFD fd) >>= return.toBool
#else
    c_isatty (fdFD fd) >>= return.toBool
#endif

I note it's used there #if defined(mingw32_HOST_OS), but its definition in System.Posix.Internals is #if !defined(HTYPE_TCFLAG_T) - that doesn't seem right, but I don't know enough about what those constants mean to say what should be done about it (if anything).

comment:6 Changed 3 years ago by simonpj

I tripped over it too. The patch below "fixes" it, but it is obviously Not Right.

Various functions are defined under THREE different sets of conditions

  • #if !defined(HTYPE_TCFLAG_T)
  • #if !defined(mingw32_HOST_OS)
  • #if !defined(mingw32_HOST_OS) && !defined(__MINGW32__)

They should presumably be just one set of conditions?

Anyway it would be great if someone could fix it properly.

Simon

diff --git a/libraries/base/System/Posix/Internals.hs b/libraries/base/System/Posix/Internals.hs
index e2e32c3..344c09c 100644
--- a/libraries/base/System/Posix/Internals.hs
+++ b/libraries/base/System/Posix/Internals.hs
@@ -25,24 +25,43 @@ module System.Posix.Internals
      CFLock, CFilePath, CGroup, CLconv, CPasswd, CSigaction, CSigset, CStat,
      CTermios, CTm, CTms, CUtimbuf, CUtsname, FD,
 
-     c_access, c_chmod, c_close, c_creat, c_dup, c_dup2, c_fcntl_lock,
-     c_fcntl_read, c_fcntl_write, c_fork, c_fstat, c_ftruncate, c_getpid,
-     c_isatty, c_lflag, c_link, c_lseek, c_mkfifo, c_open, c_pipe, c_read,
-     c_s_isblk, c_s_ischr, c_s_isdir, c_s_isfifo, c_s_isreg, c_s_issock,
-     c_safe_open, c_safe_read, c_safe_write, c_sigaddset, c_sigemptyset,
-     c_sigprocmask, c_stat, c_tcgetattr, c_tcsetattr, c_umask, c_unlink,
-     c_utime, c_waitpid, c_write, const_echo, const_f_getfl, const_f_setfd,
+     c_access, c_chmod, c_close, c_creat, c_dup, c_dup2, 
+
+#if !defined(mingw32_HOST_OS) && !defined(__MINGW32__)
+     c_fcntl_lock, c_fcntl_read, c_fcntl_write, c_fork, c_link, c_mkfifo, c_pipe, 
+     c_sigemptyset, c_sigaddset, c_sigprocmask, c_tcgetattr, c_tcsetattr, 
+     c_utime, c_waitpid, c_s_issock, 
+     setCloseOnExec, 
+#endif
+#if !defined(mingw32_HOST_OS)
+     peekFilePathLen, 
+#endif
+
+#if defined(HTYPE_TCFLAG_T)
+     tcSetAttr, 
+     sizeof_termios, sizeof_sigset_t, c_lflag, poke_c_lflag, ptr_c_cc,
+     get_saved_termios, set_saved_termios,
+#else
+     is_console,
+#endif
+     
+     c_fstat, c_ftruncate, c_getpid,
+     c_isatty, c_lseek, c_open, c_read,
+     c_s_isblk, c_s_ischr, c_s_isdir, c_s_isfifo, c_s_isreg, 
+     c_safe_open, c_safe_read, c_safe_write,
+     c_stat, c_umask, c_unlink,
+     c_write, const_echo, const_f_getfl, const_f_setfd,
      const_f_setfl, const_fd_cloexec, const_icanon, const_sig_block,
      const_sig_setmask, const_sigttou, const_tcsanow, const_vmin, const_vtime,
      dEFAULT_BUFFER_SIZE, fdFileSize, fdGetMode, fdStat, fdType, fileType,
-     getEcho, get_saved_termios, ioe_unknownfiletype, lstat, newFilePath,
+     getEcho, ioe_unknownfiletype, lstat, newFilePath,
      o_APPEND, o_BINARY, o_CREAT, o_EXCL, o_NOCTTY, o_NONBLOCK, o_RDONLY,
-     o_RDWR, o_TRUNC, o_WRONLY, peekFilePath, peekFilePathLen, poke_c_lflag,
-     ptr_c_cc, puts, sEEK_CUR, sEEK_END, sEEK_SET, s_isblk, s_ischr, s_isdir,
-     s_isfifo, s_isreg, s_issock, setCloseOnExec, setCooked, setEcho,
-     setNonBlockingFD, set_saved_termios, sizeof_sigset_t, sizeof_stat,
-     sizeof_termios, st_dev, st_ino, st_mode, st_mtime, st_size, statGetType,
-     tcSetAttr, withFilePath
+     o_RDWR, o_TRUNC, o_WRONLY, peekFilePath, 
+     puts, sEEK_CUR, sEEK_END, sEEK_SET, s_isblk, s_ischr, s_isdir,
+     s_isfifo, s_isreg, s_issock, setCooked, setEcho,
+     setNonBlockingFD, sizeof_stat,
+     st_dev, st_ino, st_mode, st_mtime, st_size, statGetType,
+     withFilePath
      ) where
 
 #include "HsBaseConfig.h"

comment:7 Changed 3 years ago by MartinF

Ah, you've beaten me to it. I've been working along effectively the same lines:

diff --git a/libraries/base/System/Posix/Internals.hs b/libraries/base/System/Posix/Internals.hs
index e2e32c3..d316826 100644
--- a/libraries/base/System/Posix/Internals.hs
+++ b/libraries/base/System/Posix/Internals.hs
@@ -25,24 +25,33 @@ module System.Posix.Internals
      CFLock, CFilePath, CGroup, CLconv, CPasswd, CSigaction, CSigset, CStat,
      CTermios, CTm, CTms, CUtimbuf, CUtsname, FD,

-     c_access, c_chmod, c_close, c_creat, c_dup, c_dup2, c_fcntl_lock,
-     c_fcntl_read, c_fcntl_write, c_fork, c_fstat, c_ftruncate, c_getpid,
-     c_isatty, c_lflag, c_link, c_lseek, c_mkfifo, c_open, c_pipe, c_read,
-     c_s_isblk, c_s_ischr, c_s_isdir, c_s_isfifo, c_s_isreg, c_s_issock,
-     c_safe_open, c_safe_read, c_safe_write, c_sigaddset, c_sigemptyset,
-     c_sigprocmask, c_stat, c_tcgetattr, c_tcsetattr, c_umask, c_unlink,
-     c_utime, c_waitpid, c_write, const_echo, const_f_getfl, const_f_setfd,
-     const_f_setfl, const_fd_cloexec, const_icanon, const_sig_block,
-     const_sig_setmask, const_sigttou, const_tcsanow, const_vmin, const_vtime,
-     dEFAULT_BUFFER_SIZE, fdFileSize, fdGetMode, fdStat, fdType, fileType,
-     getEcho, get_saved_termios, ioe_unknownfiletype, lstat, newFilePath,
-     o_APPEND, o_BINARY, o_CREAT, o_EXCL, o_NOCTTY, o_NONBLOCK, o_RDONLY,
-     o_RDWR, o_TRUNC, o_WRONLY, peekFilePath, peekFilePathLen, poke_c_lflag,
-     ptr_c_cc, puts, sEEK_CUR, sEEK_END, sEEK_SET, s_isblk, s_ischr, s_isdir,
-     s_isfifo, s_isreg, s_issock, setCloseOnExec, setCooked, setEcho,
-     setNonBlockingFD, set_saved_termios, sizeof_sigset_t, sizeof_stat,
-     sizeof_termios, st_dev, st_ino, st_mode, st_mtime, st_size, statGetType,
-     tcSetAttr, withFilePath
+     c_access, c_chmod, c_close, c_creat, c_dup, c_dup2, c_fstat, c_ftruncate,
+     c_getpid, c_isatty, c_lseek, c_open, c_read, c_s_isblk, c_s_ischr,
+     c_s_isdir, c_s_isfifo, c_s_isreg, c_safe_open, c_safe_read, c_safe_write,
+     c_stat, c_umask, c_unlink, c_write, const_echo, const_f_getfl,
+     const_f_setfd, const_f_setfl, const_fd_cloexec, const_icanon,
+     const_sig_block, const_sig_setmask, const_sigttou, const_tcsanow,
+     const_vmin, const_vtime, dEFAULT_BUFFER_SIZE, fdFileSize, fdGetMode,
+     fdStat, fdType, fileType, getEcho, ioe_unknownfiletype, lstat,
+     newFilePath, o_APPEND, o_BINARY, o_CREAT, o_EXCL, o_NOCTTY, o_NONBLOCK,
+     o_RDONLY, o_RDWR, o_TRUNC, o_WRONLY, peekFilePath, puts, sEEK_CUR,
+     sEEK_END, sEEK_SET, s_isblk, s_ischr, s_isdir, s_isfifo, s_isreg,
+     s_issock, setCooked, setEcho, setNonBlockingFD, sizeof_stat, st_dev,
+     st_ino, st_mode, st_mtime, st_size, statGetType, withFilePath,
+#if !defined(mingw32_HOST_OS) && !defined(__MINGW32__)
+     c_fcntl_lock, c_fcntl_read, c_fcntl_write, c_fork, c_link, c_mkfifo,
+     c_pipe, c_s_issock, c_sigaddset, c_sigemptyset, c_sigprocmask,
+     c_tcgetattr, c_tcsetattr, c_utime, c_waitpid, setCloseOnExec,
+#endif
+#if !defined(mingw32_HOST_OS)
+     peekFilePathLen,
+#endif
+#if defined(HTYPE_TCFLAG_T)
+     c_lflag, get_saved_termios, poke_c_lflag, ptr_c_cc, set_saved_termios,
+     sizeof_sigset_t, sizeof_termios, tcSetAttr,
+#else
+     is_console,
+#endif
      ) where

 #include "HsBaseConfig.h"

What do HTYPE_TCFLAG_T / mingw32_HOST_OS / __MINGW32__ mean exactly? Are their precise semantics written down somewhere?

comment:8 Changed 3 years ago by dfeuer

Erik de Castro Lopo believes that conditional exports are generally wrong, and I think he's generally right. Can users of these conditional definitions be moved to Internals? Or can these definitions be broken out into a platform-specific module?

comment:9 Changed 3 years ago by MartinF

Owner: MartinF deleted

Yikes. I claimed the job intending to make the naïve fix, but that sort of more substantial change is probably beyond me at the moment - I'm very much still learning the ropes, having first downloaded the GHC source only a few days ago. So I think I'd better disown this & let those more familiar with the code take over.

comment:10 Changed 3 years ago by MartinF

So apparently I just can't tear myself away - this morning I figured I might as well take another look at this.

It turns out (after much git grep -w) that almost all of those conditional definitions are module-internal anyway. After dropping those from the export list, this is my diff (to be composed with the one I pasted above):

diff --git a/libraries/base/System/Posix/Internals.hs b/libraries/base/System/Posix/Internals.hs
index d316826..ea24ec3 100644
--- a/libraries/base/System/Posix/Internals.hs
+++ b/libraries/base/System/Posix/Internals.hs
@@ -39,17 +39,12 @@ module System.Posix.Internals
      s_issock, setCooked, setEcho, setNonBlockingFD, sizeof_stat, st_dev,
      st_ino, st_mode, st_mtime, st_size, statGetType, withFilePath,
 #if !defined(mingw32_HOST_OS) && !defined(__MINGW32__)
-     c_fcntl_lock, c_fcntl_read, c_fcntl_write, c_fork, c_link, c_mkfifo,
-     c_pipe, c_s_issock, c_sigaddset, c_sigemptyset, c_sigprocmask,
-     c_tcgetattr, c_tcsetattr, c_utime, c_waitpid, setCloseOnExec,
+     c_pipe, setCloseOnExec,
 #endif
 #if !defined(mingw32_HOST_OS)
      peekFilePathLen,
 #endif
-#if defined(HTYPE_TCFLAG_T)
-     c_lflag, get_saved_termios, poke_c_lflag, ptr_c_cc, set_saved_termios,
-     sizeof_sigset_t, sizeof_termios, tcSetAttr,
-#else
+#if !defined(HTYPE_TCFLAG_T)
      is_console,
 #endif
      ) where

That compiles for me. As for the remaining conditional exports...

c_pipe
Only used in GHC.Event.Control.newControl. I node that module imports it unconditionally - but it seems all of GHC.Event is only used in non-Windows environments (if I'm reading libraries/base/base.cabal correctly), so that's fine.
setCloseOnExec
Also used in GHC.Event.Control.newControl, but additionally in GHC.Event.EPoll.epollCreate.
peekFilePathLen
Only used in System.Environment.ExecutablePath.readSymbolicLink, and only #if defined(linux_HOST_OS).
is_console
As above, only used in GHC.IO.FD.isTerminal, and only #if defined(mingw32_HOST_OS).

So am I right in thinking, for the ones that are only used in one place, the solution would be to just move the foreign import to the point-of-use?

That'd just leave setCloseOnExec (used in two places) - would GHC.Event.Internal be a good place for that one?

comment:11 Changed 3 years ago by hvr

btw this reminds me of a trick I sometimes use to find out which symbols are used where within base by compiling base w/ -ddump-minimal-imports and use the result as a first approximation; another trick (if you are only interested in a few symbol's use sites) is to attach WARNING pragmas to symbols and see where the warnings get triggered

comment:12 Changed 3 years ago by MartinF

I've noticed something strange while looking into peekFilePathLen - this is where it's defined:

#ifdef mingw32_HOST_OS
withFilePath :: FilePath -> (CWString -> IO a) -> IO a
withFilePath = withCWString

newFilePath :: FilePath -> IO CWString
newFilePath = newCWString

peekFilePath :: CWString -> IO FilePath
peekFilePath = peekCWString
#else

withFilePath :: FilePath -> (CString -> IO a) -> IO a
newFilePath :: FilePath -> IO CString
peekFilePath :: CString -> IO FilePath
peekFilePathLen :: CStringLen -> IO FilePath

withFilePath fp f = getFileSystemEncoding >>= \enc -> GHC.withCString enc fp f
newFilePath fp = getFileSystemEncoding >>= \enc -> GHC.newCString enc fp
peekFilePath fp = getFileSystemEncoding >>= \enc -> GHC.peekCString enc fp
peekFilePathLen fp = getFileSystemEncoding >>= \enc -> GHC.peekCStringLen enc fp

#endif

So, #ifdef mingw32_HOST_OS, those three related functions all have types involving CWString - but if not, their types have CString instead.

If CPP-conditional exports are generally wrong, would the same be true for exports with CPP-conditional types?

comment:13 Changed 3 years ago by dfeuer

Yuck! It doesn't get much nastier than that, I don't think.

comment:14 Changed 3 years ago by dfeuer

I believe the correct way to do this is probably to change CFilePath from a type synonym to a newtype, and hide the details from users.

comment:15 Changed 3 years ago by simonpj

Simon M says: we don't need __MINGW32__ any more, ever since we abandoned support for Cygwin. Replace with mingw32_HOST_OS.

Maybe it is only used in this one file!

Simon

comment:16 Changed 3 years ago by thoughtpolice

Component: CompilerBuild System
Milestone: 7.12.1
Owner: set to thoughtpolice

comment:17 Changed 3 years ago by Austin Seipp <austin@…>

In e1a458101a5233068f4d50fdfcfa473b5c8fdd11/ghc:

Revert "Fix undefined GHC.Real export with integer-simple"

This reverts commit 228902aa4a3350a9c99e421c0c989c7de794b7b6.

This commit is dependent on d6e7f5dc9db7e382ce34d649f85505176a451a04,
which broke the build on Windows (issue #9945).

comment:18 Changed 3 years ago by Austin Seipp <austin@…>

In f006ed7965a0fa918d720cc387b33cb8e7083854/ghc:

Revert "Add export lists to some modules."

This reverts commit d6e7f5dc9db7e382ce34d649f85505176a451a04.

This commit broke the build on Windows due to CPP weirdness (#9945).

comment:19 Changed 3 years ago by thoughtpolice

Resolution: fixed
Status: newclosed

I've reverted the offending commits in the mean time. We'll need a more principled solution to this later on anyway if we want to reintroduce them.

comment:20 Changed 3 years ago by thoughtpolice

Owner: thoughtpolice deleted
Resolution: fixed
Status: closednew

Whoops, closed in error - there's still a problem here obviously.

comment:21 Changed 3 years ago by simonpj

So the underlying issue is that of these overlapping #defines. See comment:6.

It'd be great if someone could actually fix the underlying problem.

comment:22 Changed 2 years ago by thomie

Resolution: fixed
Status: newclosed

We are figuring out what all the #ifdefs actually mean, and cleaning them up in #10485.

Building on Windows works currently, so I'm closing this ticket.

David: please open a new ticket for adding those export lists (reapplying your patches).

comment:23 Changed 2 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

Note: See TracTickets for help on using tickets.