HEASARC / cfitsio

C and Fortran library for reading and writing FITS files
26 stars 10 forks source link

Curl is hard coded to off for all Windows #29

Closed kmilos closed 1 month ago

kmilos commented 2 months ago

From the readme: -DUSE_CURL=OFF (default is ON, but unavailable in MS Visual Studio)

https://github.com/HEASARC/cfitsio/blob/3b6fc6feea25c6e8926645e06570b16f6e15cfeb/CMakeLists.txt#L75

However, there should be no problem using it w/ MinGW, which is also covered by the CMake WIN32 define.

Did you perhaps mean if(NOT MSVC) here instead?

Same goes for the USE_BZIP2 gating as well - there's no real reason it wouldn't work w/ MinGW...

kmilos commented 2 months ago

Ah, this fails for another reason, it's not just MSVC only... Looks like there is symbol clash also w/ MinGW:

FAILED: CMakeFiles/cfitsio.dir/cfileio.c.obj 
  D:\M\msys64\clang64\bin\clang.exe -DCFITSIO_HAVE_CURL -DHAVE_FTRUNCATE -DHAVE_UNISTD_H -D_REENTRANT -IC:/_/B/src/cfitsio-4.5.0 -IC:/_/B/src/static-CLANG64 -march=nocona -msahf -mtune=generic -O2 -pipe -Wp,-D_FORTIFY_SOURCE=2 -fstack-protector-strong -Wp,-D__USE_MINGW_ANSI_STDIO=1 -D_LARGEFILE_SOURCE=ON -D_FILE_OFFSET_BITS=64 -O3 -DNDEBUG -msse2 -MD -MT CMakeFiles/cfitsio.dir/cfileio.c.obj -MF CMakeFiles\cfitsio.dir\cfileio.c.obj.d -o CMakeFiles/cfitsio.dir/cfileio.c.obj -c C:/_/B/src/cfitsio-4.5.0/cfileio.c
  In file included from C:/_/B/src/cfitsio-4.5.0/cfileio.c:16:
  In file included from D:/M/msys64/clang64/include/curl/curl.h:81:
  In file included from D:/M/msys64/clang64/include/winsock2.h:23:
  In file included from D:/M/msys64/clang64/include/windows.h:69:
  In file included from D:/M/msys64/clang64/include/windef.h:9:
  In file included from D:/M/msys64/clang64/include/minwindef.h:163:
  D:/M/msys64/clang64/include/winnt.h:384:25: error: expected identifier or '('

    384 |   typedef unsigned char TBYTE, *PTBYTE;

        |                         ^

  C:/_/B/src/cfitsio-4.5.0/fitsio.h:223:22: note: expanded from macro 'TBYTE'

    223 | #define TBYTE        11

        |                      ^

  1 error generated.

May I suggest simply renaming your TBYTE to e.g. T_BYTE everywhere?

bonimy commented 1 month ago

May I suggest simply renaming your TBYTE to e.g. T_BYTE everywhere?

That would probably break a ton of dependencies, but you could add a define for what the typedef should be, and change it for windows/mingw systems that want curl. e.g.

#if defined(CFITSIO_HAVE_CURL) && (defined(_WIN32) || defined(__MINGW__))
#define __FITS_UCHAR_TYPENAME__ T_BYTE
#define T_BYTE 11
#else
#define __FITS_UCHAR_TYPENAME__ TBYTE
#define TBYTE 11
#endif

However, you can also fix it on the windows side:

// Located in winnt.h, where the issue is occurring
#ifndef _TCHAR_DEFINED
typedef char TCHAR, *PTCHAR;
typedef unsigned char TBYTE , *PTBYTE ;
#define _TCHAR_DEFINED
#endif /* !_TCHAR_DEFINED */

just add the compiler define _TCHAR_DEFINED and the collision should go away.

kmilos commented 1 month ago

just add the compiler define _TCHAR_DEFINED and the collision should go away

This could indeed work, thanks.

However, still requires the project to remove the hard coded IF(NOT WIN32) gate for USE_CURL (and USE_BZIP2; if either doesn't work, one can still easily switched them off at configure time).

bryanirby commented 1 month ago

We've been testing a change to cfileio.c that moves the CURL ifdef ahead of the fitsio2.h include, and adds an #undef TBYTE:

ifdef CFITSIO_HAVE_CURL

include <curl/curl.h>

endif

undef TBYTE

include "fitsio2.h"

This works well for me so far on Windows with MinGW and Visual Studio (after taking out the WIN32 conditional in CMakeLists.txt). Still looking into the bzip2 question.

bryanirby commented 1 month ago

I've just pushed a change to the develop branch that removes the restrictions on curl & bzip2 for WIN32. It's not clear to me why bzip2 was previously disallowed.