Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compile fails with mingw due to cludgy defines #74

Closed
DJQuardaboff opened this issue Nov 25, 2019 · 18 comments
Closed

compile fails with mingw due to cludgy defines #74

DJQuardaboff opened this issue Nov 25, 2019 · 18 comments

Comments

@DJQuardaboff
Copy link

@DJQuardaboff DJQuardaboff commented Nov 25, 2019

Many importers and converters include files which include windows.h. In mingw, windows.h (and other files) contain a line like:
#define interface struct
...
oof

/* Some kludge for Obj-C.
   For Obj-C the 'interface' is a keyword, but interface is used
   in midl-code too.  To resolve this conflict for at least the
   main windows API header, we define it here temporary.  */

It obviously does not work well with Corrade::PluginManager::Implementation::StaticPlugin in AbstractManager.h, which contains a const char* interface, and when referenced produces this error for me:

In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/oleidl.h:7,
                 from C:/msys64/mingw64/x86_64-w64-mingw32/include/ole2.h:38,
                 from C:/msys64/mingw64/x86_64-w64-mingw32/include/wtypes.h:12,
                 from C:/msys64/mingw64/x86_64-w64-mingw32/include/winscard.h:10,
                 from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:97,
                 from C:/git/magnum-plugins/src/external/dr/dr_flac.h:3630,
                 from C:/git/magnum-plugins/src/MagnumPlugins/DrFlacAudioImporter/DrFlacImporter.cpp:37:
C:/git/magnum-plugins/src/MagnumPlugins/DrFlacAudioImporter/DrFlacImporter.cpp: In function 'int pluginImporter_DrFlacAudioImporter()':
C:/git/magnum-plugins/src/MagnumPlugins/DrFlacAudioImporter/DrFlacImporter.cpp:195:1: error: expected unqualified-id before 'struct'
  195 | CORRADE_PLUGIN_REGISTER(DrFlacAudioImporter, Magnum::Audio::DrFlacImporter,
      | ^~~~~~~~~~~~~~~~~~~~~~~

I temporarily solved it by slapping an #undef interface right before the CORRADE_PLUGIN_REGISTER.

@DJQuardaboff

This comment has been minimized.

Copy link
Author

@DJQuardaboff DJQuardaboff commented Nov 25, 2019

libs affected for me were:
DrFlacAudioImporter
JpegImageConverter
JpegImporter
TinyGltfImporter

@mosra mosra added this to the 2019.1c milestone Nov 25, 2019
@mosra mosra added this to TODO in Project management via automation Nov 25, 2019
@mosra

This comment has been minimized.

Copy link
Owner

@mosra mosra commented Nov 25, 2019

Uh oh. I'm aware of this silly define, and here I'm attempting to #undef it: https://github.com/mosra/corrade/blob/9584912024ea813073ffc15b3c13e3a68a33975d/src/Corrade/PluginManager/AbstractManager.h#L45-L49

The MinGW build on the CI compiles the plugins fine and I'm not aware of any similar reports, so this has to be either new in recent MinGW or you have something special in your build that defines it again after it's undef'd. What's the MinGW version you have?

@williamjcm are you running into this issue as well?

@DJQuardaboff

This comment has been minimized.

Copy link
Author

@DJQuardaboff DJQuardaboff commented Nov 25, 2019

Thank you for your fast replies

I'm not sure if this is good enough for the mingw version

MINGWBASEDIR=C:\msys64\mingw64
gcc version 9.1.0 (Rev3, Built by MSYS2 project)
gcc version 9.1.0 (Rev3, Built by MSYS2 project)
GNU gdb (GDB) 8.3
GNU ld (GNU Binutils) 2.30
GNU windres (GNU Binutils) 2.30
GNU dlltool (GNU Binutils) 2.30
GNU Make 4.2.1

It looks like windows.h gets included after the interface undef.
DrFlacImporter.cpp:

#include "DrFlacImporter.h" // << this is where interface is undef'd

#include <Corrade/Containers/ScopeGuard.h>
#include <Corrade/Utility/Assert.h>
#include <Corrade/Utility/Debug.h>
#include <Corrade/Utility/Endianness.h>

#include <Magnum/Math/Packing.h>

#define DR_FLAC_IMPLEMENTATION
#include "dr_flac.h" // << this seems to include windows.h
@williamjcm

This comment has been minimized.

Copy link
Contributor

@williamjcm williamjcm commented Nov 26, 2019

@williamjcm are you running into this issue as well?

Nope. Though at the same time, I don't use any of the mentioned plugins.

EDIT: Also, IIRC, the magnum-plugins package in the MSYS2 repos has those plugins enabled, but it builds without issues.

@mosra

This comment has been minimized.

Copy link
Owner

@mosra mosra commented Nov 26, 2019

Argh, I have no idea why there would be a difference 😅 If I can't reproduce this, I can't reliably fix all occurences. I can probably manage to fix this for DrFlac by defining DR_FLAC_NO_STDIO, but I have no idea why libjpeg would need windows.h.

@DJQuardaboff if you add -DWIN32_LEAN_AND_MEAN to your CMAKE_CXX_FLAGS, does this go away? Or, what's the exact compiler command line that is used?

@DJQuardaboff

This comment has been minimized.

Copy link
Author

@DJQuardaboff DJQuardaboff commented Nov 27, 2019

The magnum-plugins package in the MSYS2 repos is fine because it's v2019.01. The struct with member interface wasn't in corrade in 2019.01.

Command line:

cd /C/git/magnum-plugins/cmake-build-release-mingw64/src/MagnumPlugins/DrFlacAudioImporter && /C/msys64/mingw64/bin/g++.exe  -DCORRADE_STATIC_PLUGIN -DUNICODE -D_UNICODE -I/C/git/magnum-plugins/src -I/C/git/magnum-plugins/cmake-build-release-mingw64/src -isystem /C/git/magnum-plugins/src/external/dr -isystem /C/msys64/mingw64/include/AL  -O3 -DNDEBUG   -std=c++11 -Wall -Wextra -Wold-style-cast -Winit-self -Werror=return-type -Wmissing-declarations -pedantic -fvisibility=hidden -fvisibility-inlines-hidden -Wzero-as-null-pointer-constant -Wdouble-promotion -o CMakeFiles/DrFlacAudioImporter.dir/DrFlacImporter.cpp.obj -c /C/git/magnum-plugins/src/MagnumPlugins/DrFlacAudioImporter/DrFlacImporter.cpp

Adding -DWIN32_LEAN_AND_MEAN fixed both DrFlacAudioImporter and TinyGltfImporter, but JpegImageConverter and JpegImporter fail because they include shlwapi.h
JpegImporter error:

In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/wtypes.h:7,
                 from C:/msys64/mingw64/x86_64-w64-mingw32/include/shtypes.h:23,
                 from C:/msys64/mingw64/x86_64-w64-mingw32/include/shlwapi.h:17,
                 from C:/msys64/mingw64/include/jmorecfg.h:19,
                 from C:/msys64/mingw64/include/jpeglib.h:31,
                 from C:/git/magnum-plugins/src/MagnumPlugins/JpegImporter/JpegImporter.cpp:40:
C:/git/magnum-plugins/src/MagnumPlugins/JpegImporter/JpegImporter.cpp: In function 'int pluginImporter_JpegImporter()':
C:/git/magnum-plugins/src/MagnumPlugins/JpegImporter/JpegImporter.cpp:153:1: error: expected unqualified-id before 'struct'
  153 | CORRADE_PLUGIN_REGISTER(JpegImporter, Magnum::Trade::JpegImporter,
      | ^~~~~~~~~~~~~~~~~~~~~~~

(rpc.h is where the actual #define interface struct is)
In jmorecfg.h:

#include <shlwapi.h> /* typedefs INT16 and INT32 */

It compiles even when commented out, so the flag NOSHLWAPI seems useful (and does solve the problem for me)
Actually, NOSHLWAPI, seems to fix everything, huh (kinda ominous tbh. I looked through pre-processor #if statements for a reason this works and I didn't find one)
btw, rpc.h is where all 4 errors point to. #define interface struct seems to be everywhere
btw again, this is at the bottom of windows.h (and ONLY windows.h):

/* Restore old value of interface for Obj-C.  See above.  */
#ifdef __OBJC__
#pragma pop_macro("interface")
#endif

DR_FLAC_NO_WIN32_IO also works for DrFlacAudioImporter.

@williamjcm

This comment has been minimized.

Copy link
Contributor

@williamjcm williamjcm commented Nov 29, 2019

The magnum-plugins package in the MSYS2 repos is fine because it's v2019.01

Nope, it's 2019.10. However, I noticed the magnum-integration package is still 2019.01, so I'll mention that to the repos' maintainers.

@DJQuardaboff

This comment has been minimized.

Copy link
Author

@DJQuardaboff DJQuardaboff commented Dec 1, 2019

Yes, my bad I didn't refresh the pacman database.
Since this seems like a non-issue now, should i close it?

@mosra

This comment has been minimized.

Copy link
Owner

@mosra mosra commented Dec 1, 2019

@DJQuardaboff ah, so if I understand correctly, the issue disappears when updating to latest? :)

I'll add DR_FLAC_NO_WIN32_IO because it seems like a good thing to do regardless.

@DJQuardaboff

This comment has been minimized.

Copy link
Author

@DJQuardaboff DJQuardaboff commented Dec 9, 2019

The problem is still there if compiling v2019.10 on mingw, but the mingw pacman has precompiled binary for it. My specific problem is solved.

@mosra

This comment has been minimized.

Copy link
Owner

@mosra mosra commented Dec 9, 2019

Can you upload your CMakeCache.txt from the build directory where the compilation fails? Or the whole thing, including cloned sources? I'll try to investigate.

@DJQuardaboff

This comment has been minimized.

Copy link
Author

@DJQuardaboff DJQuardaboff commented Dec 10, 2019

https://drive.google.com/drive/folders/1gXT692jvbpmFkk-9Zcm6F51xCCmW8F9U?usp=sharing
Running git status on corrade and magnum-plugins return the same thing:

HEAD detached at v2019.10
Untracked files:
  (use "git add <file>..." to include in what will be committed)

        cmake-build-release-mingw64/

ps. I tried install a fresh copy of mingw to make sure I wasn't crazy and it didn't play nice with cmake... :(

@mosra

This comment has been minimized.

Copy link
Owner

@mosra mosra commented Dec 11, 2019

Yay, thank you! This helped me to finally reproduce -- https://ci.appveyor.com/project/mosra/magnum-plugins/build/next-1030/job/72472c9idmuweyaw ... somehow this happens only when the plugins are built static (and I forgot to ask about that before 😅).

Investigating why, will push a fix once I know what's to blame.

@mosra

This comment has been minimized.

Copy link
Owner

@mosra mosra commented Dec 11, 2019

With 5aca223 I added static builds of plugins to the CI, which uncovered two more unrelated issues that are also fixed now (😅). Then, mosra/corrade@b70e4f9 defines WIN32_MEAN_AND_LEAN not just on MSVC but also on MinGW, fixing 90% of the issues, and then 4018164 fixes the issue with libjpeg. After some painful searching I discovered the shlwapi.h is not included in any mainline libjpeg (turbo, mozjpeg, ...) implementation, there's only an old patch in the MSYS package (https://github.com/msys2/MINGW-packages/blob/e9badcb5e7ee88e87f6cb6a23b6e0ba69468135b/mingw-w64-libjpeg-turbo/0001-header-compat.mingw.patch) and it's there since 2015, so I have no idea if it's still relevant.

So current master should have everything fixed again. @DJQuardaboff thanks for your patience and the extensive investigation you did -- it helped a lot. Sorry it took me ages to fix :)

If you can confirm everything is building properly again, I'll be grateful. Thanks again!

@mosra mosra closed this Dec 11, 2019
Project management automation moved this from TODO to Done Dec 11, 2019
@DJQuardaboff

This comment has been minimized.

Copy link
Author

@DJQuardaboff DJQuardaboff commented Dec 12, 2019

...!! yes! compiles!
wow! What a gem to be buried in a patch file.

@williamjcm

This comment has been minimized.

Copy link
Contributor

@williamjcm williamjcm commented Dec 13, 2019

so I have no idea if it's still relevant.

Usually, if a patch stays in the repo, it's relevant. And in this particular case, it's still used in the PKGBUILD, so, it is.

@mosra

This comment has been minimized.

Copy link
Owner

@mosra mosra commented Dec 13, 2019

Yes, but commenting the thing out didn't break anything ... and compiling vanilla unpatched libjpeg-turbo on mingw (on magnum's CI) works as well, so I have my doubts.

@williamjcm

This comment has been minimized.

Copy link
Contributor

@williamjcm williamjcm commented Dec 14, 2019

Huh. I'll try mentioning it on MSYS2's Gitter room, then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.