search

sezero / mikmod

Mikmod Sound System (mirror of git repo at https://sf.net/projects/mikmod/)
http://mikmod.sourceforge.net/
69 stars 21 forks source link

Detect MSYS2 in configure.ac, include windows.h for dsound.h. #31

Closed AliceLR closed 3 years ago

AliceLR commented 3 years ago

Fixes configury issues discussed in #29 by adding checks for MSYS2 to configure.ac. My environment also refuses to compile anything including dsound.h unless windows.h is included first, so I patched that as well (if it hurts anything this can be reverted).

sezero commented 3 years ago

My environment also refuses to compile anything including dsound.h unless windows.h is included first, so I patched that as well (if it hurts anything this can be reverted).

For drv_ds.c side of the patch: It includes mikmod_internals.h which includes windows.h but of course only if _WIN32 is defined. Does MSYS compilers not define _WIN32?

AliceLR commented 3 years ago

Nice catch. From this test program:

#ifdef _WIN32
#warning has _WIN32
#else
#warning no _WIN32
#endif

#ifdef WIN32
#warning has WIN32
#else
#warning no WIN32
#endif

#ifdef __GNUC__
#warning has __GNUC__
#else
#warning no __GNUC__
#endif

#ifdef __MINGW32__
#warning has __MINGW32__
#else
#warning no __MINGW32__
#endif

#ifdef __CYGWIN__
#warning has __CYGWIN__
#else
#warning no __CYGWIN__
#endif

I get the output:

$ gcc tmp.c -otmp
tmp.c:4:2: warning: #warning no _WIN32 [-Wcpp]
    4 | #warning no _WIN32
      |  ^~~~~~~
tmp.c:10:2: warning: #warning no WIN32 [-Wcpp]
   10 | #warning no WIN32
      |  ^~~~~~~
tmp.c:14:2: warning: #warning has __GNUC__ [-Wcpp]
   14 | #warning has __GNUC__
      |  ^~~~~~~
tmp.c:22:2: warning: #warning no __MINGW32__ [-Wcpp]
   22 | #warning no __MINGW32__
      |  ^~~~~~~
tmp.c:26:2: warning: #warning has __CYGWIN__ [-Wcpp]
   26 | #warning has __CYGWIN__
      |  ^~~~~~~

This is using the GCC packaged for the MSYS2 "MSYS" environment. The compilers packaged for the MSYS2 MINGW64/MINGW32 environments both output:

$ gcc tmp.c -otmp
tmp.c:2:2: warning: #warning has _WIN32 [-Wcpp]
    2 | #warning has _WIN32
      |  ^~~~~~~
tmp.c:8:2: warning: #warning has WIN32 [-Wcpp]
    8 | #warning has WIN32
      |  ^~~~~~~
tmp.c:14:2: warning: #warning has __GNUC__ [-Wcpp]
   14 | #warning has __GNUC__
      |  ^~~~~~~
tmp.c:20:2: warning: #warning has __MINGW32__ [-Wcpp]
   20 | #warning has __MINGW32__
      |  ^~~~~~~
tmp.c:28:2: warning: #warning no __CYGWIN__ [-Wcpp]
   28 | #warning no __CYGWIN__
      |  ^~~~~~~

So yes, it looks like the problem is just that this specific compiler is missing these defines. Adding __CYGWIN__ to the check in mikmod_internal.h is probably preferable to the change to drv_ds.c.

AliceLR commented 3 years ago

Update: when I add a check for __CYGWIN__ there, windows.h is included correctly (and very obviously because compilation time more than doubles), but this error also occurs on link:

/usr/lib/gcc/x86_64-pc-msys/10.2.0/../../../../x86_64-pc-msys/bin/ld: drivers/.libs/drv_ds.o:drv_ds.c:(.rdata$.refptr.IID_IDirectSoundNotify[.refptr.IID_IDirectSoundNotify]+0x0): undefined reference to `IID_IDirectSoundNotify'

This didn't occur before with this patch probably because the windows.h include was placed after the CINTERFACE define. So unless CINTERFACE is something that should go in mikmod.h or mikmod_internals.h I think that include in drv_ds.c is needed regardless.

Is the only reason windows.h is being included globally the Windows types? Now seems like a good time to mention that adding __CYGWIN__ to those checks to include windows.h globally doubles my build times... 🙂

edit:

make clean
time make -j8

with windows.h includes:

real    0m35.660s
user    2m4.994s
sys     1m14.717s

without:

real    0m19.501s
user    0m48.235s
sys     0m37.683s
sezero commented 3 years ago

Update: when I add a check for __CYGWIN__ there, windows.h is included correctly (and very obviously because compilation time more than doubles), but this error also occurs on link:

/usr/lib/gcc/x86_64-pc-msys/10.2.0/../../../../x86_64-pc-msys/bin/ld: drivers/.libs/drv_ds.o:drv_ds.c:(.rdata$.refptr.IID_IDirectSoundNotify[.refptr.IID_IDirectSoundNotify]+0x0): undefined reference to `IID_IDirectSoundNotify'

This didn't occur before with this patch probably because the windows.h include was placed after the CINTERFACE define. So unless CINTERFACE is something that should go in mikmod.h or mikmod_internals.h I think that include in drv_ds.c is needed regardless.

The problem is not CINTERFACE, it is INITGUID: Whatever version of windows platform sdk headers msys is using, they seem to be somewhat broken? Can you give me a link to msys headers? And here is something from memory:

diff --git a/libmikmod/drivers/drv_ds.c b/libmikmod/drivers/drv_ds.c
index 69e4540..8f86de9 100644
--- a/libmikmod/drivers/drv_ds.c
+++ b/libmikmod/drivers/drv_ds.c
@@ -44,6 +44,12 @@
 #include <string.h>

 #define INITGUID
+#ifdef __LCC__
+#include <guiddef.h>
+#else
+#include <basetyps.h> /* guiddef.h not in all SDKs, e.g. mingw.org */
+#endif
+
 #if !defined(__cplusplus) && !defined(CINTERFACE)
 #define CINTERFACE
 #endif

Is the only reason windows.h is being included globally the Windows types? Now seems like a good time to mention that adding __CYGWIN__ to those checks to include windows.h globally doubles my build times...

Should be (mostly) fixed by defining WIN32_LEAN_AND_MEAN. E.g.:

diff --git a/libmikmod/include/mikmod_internals.h b/libmikmod/include/mikmod_internals.h
index c0c20ae..cc75a6b 100644
--- a/libmikmod/include/mikmod_internals.h
+++ b/libmikmod/include/mikmod_internals.h
@@ -107,6 +107,9 @@ extern MikMod_handler_t _mm_errorhandler;
             DosReleaseMutexSem(_mm_mutex_##name)

 #elif defined(_WIN32)
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
 #include <windows.h>
 #define DECLARE_MUTEX(name) \
         extern HANDLE _mm_mutex_##name
AliceLR commented 3 years ago

The problem is not CINTERFACE, it is INITGUID

I added that patch and it does fix that linking issue.

Whatever version of windows platform sdk headers msys is using, they seem to be somewhat broken? Can you give me a link to msys headers? And here is something from memory:

Not sure if there's a browsable version but an archive of the Win32 headers package is available here: https://packages.msys2.org/package/msys2-w32api-headers?repo=msys&variant=x86_64

WIN32_LEAN_AND_MEAN

I did try this initially in drv_ds.c (but not in the global headers), and this happens (even with the guiddef.h/basetyps.h code added):

libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I./include -DMIKMOD_BUILD=1 -g -O2 -ffast-math -Wall -DSYM_VISIBILITY -fvisibility=hidden -MT drivers/drv_ds.lo -MD -MP -MF drivers/.deps/drv_ds.Tpo -c drivers/drv_ds.c  -DDLL_EXPORT -DPIC -o drivers/.libs/drv_ds.o
In file included from /usr/include/w32api/wtypes.h:23,
                 from /usr/include/w32api/winscard.h:10,
                 from /usr/include/w32api/windows.h:97,
                 from /usr/include/w32api/rpc.h:16,
                 from /usr/include/w32api/objbase.h:7,
                 from /usr/include/w32api/dsound.h:29,
                 from drivers/drv_ds.c:57:
/usr/include/w32api/wtypesbase.h:319:5: error: unknown type name ‘byte’
  319 |     byte abData[1];
      |     ^~~~
/usr/include/w32api/wtypesbase.h:341:5: error: unknown type name ‘byte’
  341 |     byte abData[1];
      |     ^~~~
/usr/include/w32api/wtypesbase.h:356:5: error: unknown type name ‘byte’
  356 |     byte *pData;
      |     ^~~~
/usr/include/w32api/wtypesbase.h:371:5: error: unknown type name ‘hyper’
  371 |     hyper *pData;
      |     ^~~~~
/usr/include/w32api/wtypesbase.h:377:9: error: unknown type name ‘boolean’
  377 | typedef boolean BOOLEAN;
      |         ^~~~~~~
/usr/include/w32api/wtypesbase.h:377:17: error: conflicting types for ‘BOOLEAN’
  377 | typedef boolean BOOLEAN;
      |                 ^~~~~~~
In file included from /usr/include/w32api/minwindef.h:163,
                 from /usr/include/w32api/windef.h:8,
                 from /usr/include/w32api/windows.h:69,
                 from /usr/include/w32api/rpc.h:16,
                 from /usr/include/w32api/objbase.h:7,
                 from /usr/include/w32api/dsound.h:29,
                 from drivers/drv_ds.c:57:
/usr/include/w32api/winnt.h:605:16: note: previous declaration of ‘BOOLEAN’ was here
  605 |   typedef BYTE BOOLEAN;
      |                ^~~~~~~
In file included from /usr/include/w32api/winscard.h:10,
                 from /usr/include/w32api/windows.h:97,
                 from /usr/include/w32api/rpc.h:16,
                 from /usr/include/w32api/objbase.h:7,
                 from /usr/include/w32api/dsound.h:29,
                 from drivers/drv_ds.c:57:
/usr/include/w32api/wtypes.h:46:5: error: unknown type name ‘byte’
   46 |     byte data[1];
      |     ^~~~
/usr/include/w32api/wtypes.h:54:5: error: unknown type name ‘byte’
   54 |     byte data[1];
      |     ^~~~
/usr/include/w32api/wtypes.h:59:5: error: unknown type name ‘byte’
   59 |     byte data[1];
      |     ^~~~
/usr/include/w32api/wtypes.h:63:5: error: unknown type name ‘byte’
   63 |     byte data[1];
      |     ^~~~
/usr/include/w32api/wtypes.h:68:5: error: unknown type name ‘byte’
   68 |     byte data[1];
      |     ^~~~
/usr/include/w32api/wtypes.h:73:5: error: unknown type name ‘byte’
   73 |     byte data[1];
      |     ^~~~
/usr/include/w32api/wtypes.h:299:5: error: unknown type name ‘byte’
  299 |     byte pBuffer[1];
      |     ^~~~

But even when building drv_ds.h with an explicit windows.h include, and with __CYGWIN__ checks and WIN32_LEAN_AND_MEAN in both mikmod.h and mikmod_internals.h, the build time is

real    0m35.792s
user    2m6.319s
sys     1m14.992s

Since Windows types are used everywhere these really should be included for my setup, but still, wow.

AliceLR commented 3 years ago

I've updated this to add the checks for __CYGWIN__ and also your guiddef.h/basetyps.h patch.

sezero commented 3 years ago

Did minor revising in drv_ds.c part. Also made this into a three-commits patchset, inlined below. Does it work OK?

Part - 1: configure.ac

configure.ac: detect MSYS2 as a windows target.

diff --git a/libmikmod/configure.ac b/libmikmod/configure.ac
index c75af9d..f516202 100644
--- a/libmikmod/configure.ac
+++ b/libmikmod/configure.ac
@@ -52,6 +52,7 @@ libmikmod_hpux=no

 case $host_os in
    mingw*) libmikmod_mingw=yes ;;
+   msys*) libmikmod_mingw=yes ;;
    cygwin*) libmikmod_cygwin=yes ;;
    linux*) libmikmod_linux=yes ;;
    darwin*) libmikmod_darwin=yes ;;
@@ -792,13 +793,13 @@ AC_SUBST(SDL_LIBS)
 # Windows audio
 # ---------------------
 case $host_os in
- mingw*|cygwin*)
+ mingw*|msys*|cygwin*)
    # do a windows.h check, just in case..
    AC_CHECK_HEADERS([windows.h],,[AC_MSG_ERROR([Targeting windows, but windows.h not available])])
    if test $libmikmod_driver_ds = yes
    then
        libmikmod_driver_ds=no
-       AC_CHECK_HEADERS([dsound.h],[libmikmod_driver_ds=yes])
+       AC_CHECK_HEADERS([dsound.h],[libmikmod_driver_ds=yes],[],[#include <windows.h>])
    fi
    if test $libmikmod_driver_xaudio2 = yes
    then

Part - 2: mikmod[_internals].h

mikmod[_internals].h, __CYGWIN__: include windows.h and classify as a Windows target

diff --git a/libmikmod/include/mikmod.h b/libmikmod/include/mikmod.h
index 8f33a32..84c6a7f 100644
--- a/libmikmod/include/mikmod.h
+++ b/libmikmod/include/mikmod.h
@@ -87,7 +87,7 @@ MIKMODAPI extern long MikMod_GetVersion(void);
  *  ========== Dependency platform headers
  */

-#ifdef _WIN32
+#if defined(_WIN32)||defined(__CYGWIN__)
 #ifndef WIN32_LEAN_AND_MEAN
 #define WIN32_LEAN_AND_MEAN
 #endif
diff --git a/libmikmod/include/mikmod_internals.h b/libmikmod/include/mikmod_internals.h
index c0c20ae..ec315cf 100644
--- a/libmikmod/include/mikmod_internals.h
+++ b/libmikmod/include/mikmod_internals.h
@@ -106,7 +106,7 @@ extern MikMod_handler_t _mm_errorhandler;
         if(_mm_mutex_##name)\
             DosReleaseMutexSem(_mm_mutex_##name)

-#elif defined(_WIN32)
+#elif defined(_WIN32)||defined(__CYGWIN__)
 #include <windows.h>
 #define DECLARE_MUTEX(name) \
         extern HANDLE _mm_mutex_##name

Part - 3: drv_ds.c

drv_ds.c: include guiddef.h after defining INITGUID for IID_IDirectSoundNotify to be a local symbol

diff --git a/libmikmod/drivers/drv_ds.c b/libmikmod/drivers/drv_ds.c
index 69e4540..5445e6d 100644
--- a/libmikmod/drivers/drv_ds.c
+++ b/libmikmod/drivers/drv_ds.c
@@ -20,8 +20,6 @@

 /*==============================================================================

-  $Id$
-
   Driver for output on win32 platforms using DirectSound

 ==============================================================================*/
@@ -36,7 +34,7 @@
 #include "config.h"
 #endif

-#include "mikmod_internals.h"
+#include "mikmod_internals.h" /* includes windows.h */

 #ifdef DRV_DS

@@ -44,6 +42,12 @@
 #include <string.h>

 #define INITGUID
+#if defined(__LCC__)||defined(__WATCOMC__)
+#include <guiddef.h>
+#else
+#include <basetyps.h> /* guiddef.h not in all SDKs, e.g. mingw.org */
+#endif
+
 #if !defined(__cplusplus) && !defined(CINTERFACE)
 #define CINTERFACE
 #endif
@@ -58,7 +62,6 @@
  * https://github.com/open-watcom/open-watcom-v2/commit/961ef1ff756f3ec5a7248cefcae00a6ecaa97ff4
  * Therefore, we define and use a local copy of IID_IDirectSoundNotify here.
  */
-#include <guiddef.h>
 DEFINE_GUID(IID_IDirectSoundNotify,0xB0210783,0x89cd,0x11d0,0xAF,0x08,0x00,0xA0,0xC9,0x25,0xCD,0x16);
 #endif
AliceLR commented 3 years ago

Since the patch 1 and 2 changes are identical to the current patch they should be fine. Patch 3 compiles but I think exchanging the explicit windows.h include in drv_ds.c for an implicit include, even if it's commented, is a bad idea.

1) It seems to rely specifically on the full windows.h include, where the mikmod_internals.h usage of windows.h worked just fine when I added WIN32_LEAN_AND_MEAN to it. Because mikmod_internals.h (correctly) doesn't mention drv_ds.c's dependency (or any other dependencies) on the full include, it seems reasonable to assume that it could be changed in the future and accidentally cause breakage (e.g. if mikmod_internals.h gets split up, since having a single internal header for the entire library adds a lot of extra compile time). 2) The mikmod_internals.h include of windows.h is conditional and should not be relied upon by anything aside from the small portion of the header that requires it. For example, if I were to enable pthread (MSYS2 supports it), windows.h would not be included, because mikmod_internals.h only includes it for mutex symbols. Since this ifdef chain is specifically for threading symbols it seems reasonable that the chain could also be extended and cause accidental breakage. 3) Including windows.h directly before dsound.h (preferably with a comment) more clearly communicates the intent behind including windows.h (the intent being "so this specific compilation unit doesn't break").

Would you prefer the split commits as separate pull requests or force-pushed to this branch?

sezero commented 3 years ago

Patch 3 compiles but I think exchanging the explicit windows.h include in drv_ds.c for an implicit include, even if it's commented, is a bad idea.

OK

Would you prefer the split commits as separate pull requests or force-pushed to this branch?

Split-patches force-pushed to this PR would be good.

AliceLR commented 3 years ago

OK, I've split this into separate commits as recommended. The third commit should be the same as your patch aside from changing the way the windows.h include is handled/commented.

sezero commented 3 years ago

Thanks.