fix mingw build fail - redux #427

Merged
merged 3 commits into from Sep 21, 2012

4 participants

@jonforums

This pull request fixes the mingw build fail issue (and supersedes the fix) described in #421 by giving the mingw builds ninja -t msvc build helper superpowers.

gtest results from both Win7 SDK 7.1 and mingw/mingw-w64 toolchains: https://gist.github.com/3756271

There are a couple of testing problems (file name typo, intermittent BuildLogTest.Truncate fail with MSVC) not related to this issue.

In addition to gtest testing, ninja.exe versions built with both WinSDK and mingw correctly build ninja in mingw-with-mingw, msvc-with-msvc, mingw-with-msvc, and mingw-with-msvc combinations.

jonforums added some commits Sep 20, 2012
@jonforums jonforums Give MinGW builds MSVC build helper superpowers
Note: _WIN32 is used instead of WIN32 to enable builds with MSVC
IDE, Windows SDK non-IDE command line tools, and mingw/mingw-w64
based toolchains
3b3e1c8
@jonforums jonforums Silence bothersome warning from -Wextra
Struct initializations such as those in `CLWrapper::Run` of file
`src/msvc_helper-win32.cc` causes MinGW GCC to spew warnings.
7c3b845
@buildhive

Evan Martin » ninja #175 SUCCESS
This pull request looks good
(what's this?)

@evmar

It's harmless to always include stdio, so why not drop the ifdefs?

Agreed. I'll double check and update

@evmar

I think this patch is correct as is: we should build the msvc support into ninja even when using mingw to build it.
But I'm a little confused by the larger goal here -- is the problem it that ninja's configure.py uses -t msvc even when building under mingw? I think the right fix for that is to not use -t msvc when building via mingw.

@syntheticpp syntheticpp commented on an outdated diff Sep 20, 2012
src/msvc_helper_main-win32.cc
@@ -16,6 +16,10 @@
#include <windows.h>
+#ifdef __MINGW32__

A assume it make no problem to include also when using msvc, avoids the ifdef.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonforums

The original problem was a link problem on MinGW because configure.py didn't include the impl for MSVCHelperMain in libninja.a as shown in #421 description.

I didn't see the -t msvc situation you mention when testing. What scenario do you want me to double check when I update the patch to remove the ifdef's?

@syntheticpp

if possible check if it builds with msvc after removing the ifdef, more could not happen.

@jonforums

To clarify, when using the mingw built ninja.exe to build using WinSDK, configure.py generates the following in build.ninja. Both ninja and ninja ninja_test (using the mingw built ninja.exe) then work correctly when building the exes with WinSDK 7.1 cmd line tools.


rule cxx
  command = ninja -t msvc -o $out -- $cxx /showIncludes $cflags -c $in /Fo$out
  description = CXX $out
  depfile = $out.d

By contrast, when using the mingw built ninja.exe to build using ming/mingw-w64, configure.py (with --platform=mingw) generates the following in build.ninja. Both ninja and ninja ninja_test work correctly


rule cxx
  command = $cxx -MMD -MT $out -MF $out.d $cflags -c $in -o $out
  description = CXX $out
  depfile = $out.d
@buildhive

Evan Martin » ninja #178 SUCCESS
This pull request looks good
(what's this?)

@nico nico commented on the diff Sep 21, 2012
configure.py
@@ -140,6 +140,7 @@ def binary(name):
'-fno-rtti',
'-fno-exceptions',
'-fvisibility=hidden', '-pipe',
+ '-Wno-missing-field-initializers',
@nico
nico added a note Sep 21, 2012

What is this needed for?

@nico
nico added a note Sep 21, 2012

Nevermind, I just saw the commit comment. Clang doesn't warn about this initialization style, and recent gccs probably don't either (though I can't check right now). Does mingw ship a fairly old gcc? Would it be possible to disable this warning only for mingw?

I tested with mingw gcc 4.6.2 and 4.7.1 which gave the warnings. I'd originally tried to disable the warnings for only mingw lower in configure.py (e.g. next hunk) via cflags but by that time cflags has already been written to build.ninja.

It's easy to add a mingw-specific cflags += ['-Wno-missing-field-initializers'] earlier in configure.py if that's best. I'll update patch if Evan thinks it's needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@evmar evmar merged commit e4eef72 into ninja-build:master Sep 21, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment