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

Fix build with MSYS2: require --use-windows-headers before including win... #118

Closed
wants to merge 1 commit into from

Conversation

rpavlik
Copy link

@rpavlik rpavlik commented Feb 20, 2015

...def.h

Otherwise, you pull in a lot more than just typedefs for DWORD,
including conflicting DELETE macros, etc.

The recent change to use existing Windows definitions of DWORD, etc. might work fine for the RDiscount team with their toolchain, but when using a (pretty tame) MSYS2/MinGW64-i686 toolchain, that Windows header pulls in a lot of mess, and I ended up with enough warnings that I wasn't confident the resulting binaries would work as intended.

So, I've placed that behavior behind a configure flag (since the previous behavior worked for me).

I've also improved that alternate behavior by making sure that if we're using WinDef.h, we still have a substitution for @DWORD@. Rather than forcing a windows header into mkdio.h I opted to let the existing type detection compute the correct substitution, just not the definition, especially since the configure system seemed to be designed to make that possible/easy.

…windef.h

Otherwise, you pull in a lot more than just typedefs for DWORD,
including conflicting DELETE macros, etc.
@davidfstr
Copy link

My 2¢: Having a million configure options is confusing. I'd advocate for getting a single solution that works well for both scenarios.

@rpavlik: Could you provide more information about the specific undesirable behavior you are seeing prior to this patch? That would give more context to finding a solution that works for you.

@rpavlik
Copy link
Author

rpavlik commented Feb 20, 2015

Yeah, I know, I tried for that first. Before this patch, I get errors about @DWORD@ being left unsubstituted in mkdio.h. I initially tried adding an AC_SUB 'DWORD' 'DWORD' - that got me to a DWORD not defined error - because mkdio.h didn't have the #include <WinDef.h> in it. So, I added an extra substitution field in mkdio.h to be able to add an #include <WinDef.h> line (and eventually, before it, some defines in an attempts to reduce the amount of Windows stuff that got dragged in). I got builds that way, but I got a ton of redefined macro errors, primarily about DELETE() in winnt.h. After I couldn't see any way to turn off additional includes (disabling winnt.h is both hacky and didn't work), I opted for the solution in this pull request.

@davidfstr
Copy link

I'm sorry that explanation is too low-level for me to interpret.

Could you provide specific repro steps for the issue that you're seeing. It sounded like you compiled Discount in some configuration in Windows and you received several warnings. And it was the warnings that prompted you to file this issue and create this pull request. Please confirm and/or elaborate.

@rpavlik
Copy link
Author

rpavlik commented Feb 20, 2015

Sorry about that - I already closed the terminal where I had the build errors, and I didn't keep any commits of the inadequate solutions.

I tried building just plain old Discount 2.1.8 with MSYS2's 32-bit toolchain. I had succeeded with 2.1.6 doing the same thing:

CC=gcc ./configure.sh
make

Doing this on 2.1.8 resulted first in a compiler/parse error on make because @DWORD@ was left in the mkdio.h header file - the process that takes place in configure.inc to shortcut the DWORD definition doesn't define a substitution for @DWORD@ so it's left in the source code making it invalid.

If you pick up from my explanation now at the second sentence, keeping in mind I'm primarily modifying the configure.inc file and the mkdio.h.in file, it should hopefully make more sense. Basically once I fixed this first error, I found I had to add an include file to the header to fix a second compiler error. Once I had added in the include file, I got builds that apparently succeeded but that I don't trust because of the "Warning: redefining DELETE()" messages caused by the extraneous stuff that including windef.h brings in besides just DWORD - it basically brings in much of the Windows header tree at least on this toolchain, and doing that to get a typedef for DWORD is both dangerous (see the warnings) and overkill.

I'm assuming you didn't see this in RDiscount either because of not using the configure script as bundled, or you're using a toolchain that unconditionally defines DWORD or includes a header that does so from the single header included by mkdio.h

@Orc
Copy link
Owner

Orc commented Feb 21, 2015

I'm not sure what the problem is? Is it that windef.h exists in the msys environment, but it doesn't include WORD & DWORD, or does it include WORD & DWORD plus a whole crapload of other stuff that clashes with the discount headers?

Can you run a vanilla configure and let me see your config.log plus the output from a make?

@Orc Orc self-assigned this Feb 21, 2015
@Orc
Copy link
Owner

Orc commented Feb 21, 2016

Any followup? I'd like to try and resolve this, but if it worked out by itself I'll just close the pull request.

@Orc Orc closed this Jan 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants