fltk-1.4 build error in Fl_WinAPI_System_Driver.cxx with 32-bit mingw

[imacarthur]

All,

Was just checking the logs for my builds on WIn32/mingw and my 32-bit builds of fltk-1.4 are both (where "both" here means cmake and autoconf builds) have failed in WinAPI/Fl_WinAPI_System_Driver.cxx.

The cmake builds gives:

[ 34%] Building CXX object src/CMakeFiles/fltk.dir/drivers/WinAPI/Fl_WinAPI_System_Driver.cxx.obj
d:/Support/fltk-1.4/src/drivers/WinAPI/Fl_WinAPI_System_Driver.cxx: In member function 'virtual char* Fl_WinAPI_System_Driver::preference_rootnode(Fl_Preferences*, Fl_Preferences::Root, const char*, const c                                              har*)':
d:/Support/fltk-1.4/src/drivers/WinAPI/Fl_WinAPI_System_Driver.cxx:863:26: error: 'SHGFP_TYPE_CURRENT' was not declared in this scope
                          SHGFP_TYPE_CURRENT, // dwFlags: use current, potentially redirected path
                          ^~~~~~~~~~~~~~~~~~
make[2]: *** [src/CMakeFiles/fltk.dir/drivers/WinAPI/Fl_WinAPI_System_Driver.cxx.obj] Error 1
make[1]: *** [src/CMakeFiles/fltk.dir/all] Error 2
make: *** [all] Error 2

And the "configure" build gives:

Compiling drivers/WinAPI/Fl_WinAPI_System_Driver.cxx...
drivers/WinAPI/Fl_WinAPI_System_Driver.cxx: In member function 'virtual char* Fl_WinAPI_System_Driver::preference_rootnode(Fl_Preferences*, Fl_Preferences::Root, const char*, const char*)':
drivers/WinAPI/Fl_WinAPI_System_Driver.cxx:863:26: error: 'SHGFP_TYPE_CURRENT' was not declared in this scope
                          SHGFP_TYPE_CURRENT, // dwFlags: use current, potentially redirected path
                          ^~~~~~~~~~~~~~~~~~
make[1]: *** [drivers/WinAPI/Fl_WinAPI_System_Driver.o] Error 1

Note that my mingw 64-bit builds all seem to be fine.

The crux seems to be the that the SHGFP_TYPE_CURRENT enum in the shlobj.h header file  is protected by a compile time guard of:

#if (_WIN32_IE >= 0x0500)

....

But the 32-bit mingw toolchain seems not to be setting many (any?) of the many and varied MS version macros, and certainly does not appear to be setting _WIN32_IE to anything useful, so the code fails to compile...

The wingw64 tools do seem to be setting the version macros to useful values, as do the MSVC tools, so they do not exhibit this failure.

We probably need to do something like (untested):

#ifdef _WIN32
#  if !(defined _WIN32_IE) || (_WIN32_IE < 0x0500)
#      undef _WIN32_IE
#      define _WIN32_IE  0x0500
#  endif /* _WIN32_WINNT checks */
#endif /* _WIN32 */

At the top of Fl_WinAPI_System_Driver.cxx as a workaround for this?

[imacarthur]

On Thursday, 20 April 2023 at 13:25:48 UTC+1 imacarthur wrote:

We probably need to do something like (untested):

#ifdef _WIN32
#  if !(defined _WIN32_IE) || (_WIN32_IE < 0x0500)
#      undef _WIN32_IE
#      define _WIN32_IE  0x0500
#  endif /* _WIN32_WINNT checks */
#endif /* _WIN32 */

At the top of Fl_WinAPI_System_Driver.cxx as a workaround for this?

FWIW, I tried this tweak, and it seems to work.

That said, I do wonder if the issue is more to do with the mingw32 headers than with fltk per se.

I'm not convinced that enum definition should be guarded by the _WIN32_IE macro check at all... (And I do not have the "real" MS headers here to compare against, either.)

[Albrecht Schlosser]

On 4/20/23 14:37 imacarthur wrote:

On Thursday, 20 April 2023 at 13:25:48 UTC+1 imacarthur wrote:

We probably need to do something like (untested):

#ifdef _WIN32
#  if !(defined _WIN32_IE) || (_WIN32_IE < 0x0500)
#      undef _WIN32_IE
#      define _WIN32_IE  0x0500
#  endif /* _WIN32_WINNT checks */
#endif /* _WIN32 */

At the top of Fl_WinAPI_System_Driver.cxx as a workaround for this?

FWIW, I tried this tweak, and it seems to work.

OK, great that you found this. I agree that we could do this and have two notes:

(1) We probably don't need '#ifdef _WIN32' because this file is only compiled under Windows anyway (we also include <windows.h> and more).

(2) I checked that 'SHGFP_TYPE_CURRENT == 0', hence we could replace the constant (name) with 0 and add a comment which would have the same effect.

Ian, can you please try option (2) just to be sure?

That said, I do wonder if the issue is more to do with the mingw32 headers than with fltk per se.

I'm not convinced that enum definition should be guarded by the _WIN32_IE macro check at all...

It's presumably done because these constants/functions didn't exist in versions older than '0x0500' which is IIRC Win XP or something like that.

(And I do not have the "real" MS headers here to compare against, either.)

Neither do I on this machine. However, I checked this with MinGW cross-compilation on Linux (Debian 12 / Bookworm aka 'testing').

How old are your MinGW header files? Maybe they are too old to "know" about these features?

Anyway, we should try to fix this in one or the other way. The cleaner solution would probably be to define the constant as you suggested in your workaround.

[imacarthur]

On Thursday, 20 April 2023 at 15:15:49 UTC+1 Albrecht Schlosser wrote:

OK, great that you found this. I agree that we could do this and have two notes:

(1) We probably don't need '#ifdef _WIN32' because this file is only compiled under Windows anyway (we also include <windows.h> and more).

True - I typed that out of force of habit, but this file *must* be WIN32 anyway so it's redundant here.

 

(2) I checked that 'SHGFP_TYPE_CURRENT == 0', hence we could replace the constant (name) with 0 and add a comment which would have the same effect.

Ian, can you please try option (2) just to be sure?

Yes, this seems fine too.

I checked, and (at least under mingw32) the enum explicitly sets the value to zero:

typedef enum {
SHGFP_TYPE_CURRENT = 0,
SHGFP_TYPE_DEFAULT = 1,
} SHGFP_TYPE;

So hard-coding it as zero would work here - though we'd probably need to add a comment that said the zero was meant to be SHGFP_TYPE_CURRENT I suppose.

 

That said, I do wonder if the issue is more to do with the mingw32 headers than with fltk per se.

I'm not convinced that enum definition should be guarded by the _WIN32_IE macro check at all...

It's presumably done because these constants/functions didn't exist in versions older than '0x0500' which is IIRC Win XP or something like that.

Yes, something like that, though this is the  _WIN32_IE macro (IIRC, the installation of IE brought with it a load of dll changes to add new functionality that only later found their way into the OS core...)

The XP / Vista /Win7 versioning is mainly covered by the  _WIN32_WINNT macro instead, and in that case I think 0x0501 means XP...

I have No Idea what 0x0500 means in terms of the IE versions though!

I note that shlobj.h has a lot of conditionally defined sections, using various MS macros (_WIN32_WINNT,  _WIN32_IE,  UNICODE,  COBJMACROS, possibly others) and I'm not sure the mingw32 toolchain actually sets *any* of these by default!

How old are your MinGW header files? Maybe they are too old to "know" about these features?

So I looked - I have a couple of mingw32 versions on this machine, one has the mingw32 files versioned as 5.0.2, the newer one has 5.3.0.

Both do define the necessary enum, but both have the conditional guard around the definition.

FWIW, I note that my mingw64 headers are versioned as 4.0.0 (they number it differently from the mingw32 folks though) but does not have the conditional guards around the enum declaration, so it "just works" as a result.

[Albrecht Schlosser]

On 4/20/23 16:52 imacarthur wrote:

On Thursday, 20 April 2023 at 15:15:49 UTC+1 Albrecht Schlosser wrote:

How old are your MinGW header files? Maybe they are too old to "know" about these features?

So I looked - I have a couple of mingw32 versions on this machine, one has the mingw32 files versioned as 5.0.2, the newer one has 5.3.0.

Both do define the necessary enum, but both have the conditional guard around the definition.

I installed current MinGW today and my header files are version 5.4.2 and include the guard '#if (_WIN32_IE >= 0x0500)' and the build works well out of the box.

This is only an observation on my new installation, it doesn't mean that we should not "fix" it though...

[imm]

On Thu, 20 Apr 2023, 20:48 Albrecht Schlosser wrote:

I installed current MinGW today and my header files are version 5.4.2 and include the guard '#if (_WIN32_IE >= 0x0500)' and the build works well out of the box.

This is only an observation on my new installation, it doesn't mean that we should not "fix" it though...

That's interesting - though I don't think it's the guards that are really the problem, in fact I think they're probably correct.

Rather I think the problem is that the toolchain doesn't seem to actually to be setting them at all so...

FWIW, I think that mingw64 toolchain does seem to be setting them, though it's headers don't seem to have them anyway.

The msvc tools do set the macros, of course...

I think setting them (as in my example) might still be a valid solution...?

--
Ian
From my Fairphone FP3

[Albrecht Schlosser]

On 4/20/23 22:46 imm wrote:

On Thu, 20 Apr 2023, 20:48 Albrecht Schlosser wrote:

I installed current MinGW today and my header files are version 5.4.2 and include the guard '#if (_WIN32_IE >= 0x0500)' and the build works well out of the box.

This is only an observation on my new installation, it doesn't mean that we should not "fix" it though...

That's interesting - though I don't think it's the guards that are really the problem, in fact I think they're probably correct.

Rather I think the problem is that the toolchain doesn't seem to actually to be setting them at all so...

I made some tests, and I found that the bare toolchain (i.e. preprocessor + compiler) doesn't set _WIN32_IE by default. It's not defined if I compile an otherwise "empty" source file w/o other includes (using MinGW-w64 cross compiler on Linux). However, if I '#include <windows.h>' it's "magically" defined.

There are also other header files that set it to a "minimal" value if it's not defined.

FWIW, I think that mingw64 toolchain does seem to be setting them, though it's headers don't seem to have them anyway.

The msvc tools do set the macros, of course...

I think setting them (as in my example) might still be a valid solution...?

The question is where to set them best. In my version of <windows.h> they are set depending on other defines like _WIN32_WINNT etc. or just to a minimal value.

I propose to define _WIN32_IE (as in your example) right before '#include <shlobj.h>' to avoid other potential side effects. Please test attached diff if it works for you.

There's one issue with this patch though: it could happen that it would #undef _WIN32_IE if it's not defined. Is this acceptable or would it perhaps issue compiler warnings? I tested such a case with 'gcc  -Wall -W -Wpedantic -Wextra' and didn't get warnings, but I don't know if the standard "allows" it and didn't test with MS compilers. We could make sure that this doesn't happen with another #if defined(_WIN32_IE) but if it's not necessary we could just use my patch as-is.

I'd appreciate comments and/or better solutions.

Ian, if you test the patch and it works, would it be OK if I committed it?

[imacarthur]

On Friday, 21 April 2023 at 15:05:03 UTC+1 Albrecht Schlosser wrote:

 

I propose to define _WIN32_IE (as in your example) right before '#include <shlobj.h>' to avoid other potential side effects. Please test attached diff if it works for you.

Tested in various configs; seems to be working fine, at least for me!

 

There's one issue with this patch though: it could happen that it would #undef _WIN32_IE if it's not defined. Is this acceptable or would it perhaps issue compiler warnings? I tested such a case with 'gcc  -Wall -W -Wpedantic -Wextra' and didn't get warnings, but I don't know if the standard "allows" it and didn't test with MS compilers. We could make sure that this doesn't happen with another #if defined(_WIN32_IE) but if it's not necessary we could just use my patch as-is.

I do not know for sure - I have never (as far as I can recall, anyway!) ever seen a warning from #undef'ing a macro that was not actually defined, but TBH I have no idea what the standards say, or whether there may be some compiler that would complain.

 

Ian, if you test the patch and it works, would it be OK if I committed it?

I'm happy with the patch, so go ahead with the commit.

If you are interested in a trivial "improvement" it might be to adjust the comment:

 // notably header versions 5.3.0 and earlier, whereas 5.4.2 seems to define _WIN32_IE as needed.

Because I see it fail with mingw32 header version 5.0.2 also. But that's a minor observation so feel free to ignore!

[Albrecht Schlosser]

On 4/24/23 11:34 imacarthur wrote:

On Friday, 21 April 2023 at 15:05:03 UTC+1 Albrecht Schlosser wrote:

 

I propose to define _WIN32_IE (as in your example) right before '#include <shlobj.h>' to avoid other potential side effects. Please test attached diff if it works for you.

Tested in various configs; seems to be working fine, at least for me!

Thanks for testing!

 

There's one issue with this patch though: it could happen that it would #undef _WIN32_IE if it's not defined. Is this acceptable or would it perhaps issue compiler warnings? I tested such a case with 'gcc  -Wall -W -Wpedantic -Wextra' and didn't get warnings, but I don't know if the standard "allows" it and didn't test with MS compilers. We could make sure that this doesn't happen with another #if defined(_WIN32_IE) but if it's not necessary we could just use my patch as-is.

I do not know for sure - I have never (as far as I can recall, anyway!) ever seen a warning from #undef'ing a macro that was not actually defined, but TBH I have no idea what the standards say, or whether there may be some compiler that would complain.

OK, I left it as-is. Let's see if someone complains.

 

Ian, if you test the patch and it works, would it be OK if I committed it?

I'm happy with the patch, so go ahead with the commit.

If you are interested in a trivial "improvement" it might be to adjust the comment:

 // notably header versions 5.3.0 and earlier, whereas 5.4.2 seems to define _WIN32_IE as needed.

Because I see it fail with mingw32 header version 5.0.2 also. But that's a minor observation so feel free to ignore!

Thanks again, done in commit 6f77f40aadaaec8a including your modification.