patchMakefile: Improve compiler header dependency check

The Makefile enables CHECK_HEADER_DEPENDENCIES when the compiler supports generating header dependencies. Make the check use the same flags as the invocation to avoid a false positive when user-configured compiler flags contain incompatible options.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
I fired up git's next branch on a mac laptop where I
have a config.mak that builds universal git binaries:

CFLAGS = -arch i386 -arch x86_64

This configuration broke when 111ee18c31f9bac9436426399355facc79238566 was merged into next. gcc cannot generate header dependencies when multiple -arch statements are used but the test generated a false positive.

 Makefile |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

David Aguilar wrote:

Good catch; thanks. This information would be useful for the commit message.

Sounds like a bug. Any idea why it behaves that way? What error message does it write?

If it is a bug, it might be worth reporting this to the gcc devs while at it.

[...]

EXTRA_CPPFLAGS is a target-specific variable and would always be empty, So I think this would be clearer without.

While we're touching this line, do you know if the "sh -c" is necessary? I would expect $(shell ...) to run its arguments in a shell.

Thanks --- despite the nitpicks above, this one looks good. Jonathan

Make the check use the same flags as the invocation to avoid false positives when user-configured compiler flags contain incompatible options.

For example, it is possible to build universal git binaries on OS X with the following snippet in config.mak:

	CFLAGS = -arch i386 -arch x86_64

111ee18c31f9bac9436426399355facc79238566 breaks this setup and results in the following error message:

	gcc-4.2: -E, -S, -save-temps and -M options are
	not allowed with multiple -arch flags

Include ALL_CFLAGS so that this and other conditions are caught. Use SHELL_PATH instead of assuming that "sh" is a sane shell.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
I'm not sure if "sh -c" is necessary but I did notice that other
parts of the Makefile use $(SHELL_PATH).  The check was adjusted
to use that as well.

 Makefile |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

On Sat, Aug 27, 2011 at 23:00, David Aguilar <davvid@gmail.com> wrote:

[...]

I'm not sure either. I just used what I saw at other places in the Makefile.

[patch snipped]

Looks good to me. Thanks!

- Fredrik

Hi,

Fredrik Kuivinen wrote:

It is not needed, and imho it makes it harder to read. I believe the current uses of "sh -c" near the top of the Makefile are to emphasize that a POSIX shell has not been determined yet (so POSIXy constructs cannot be used at that point on platforms like Solaris).

Aside from that, this seems good, though. While at it, the log message could be simplified to something closer to the original version:

	The Makefile enables CHECK_HEADER_DEPENDENCIES when the
	compiler supports generating header dependencies.
	Make the check use the same flags as the invocation
	to avoid a false positive when user-configured compiler
	flags contain incompatible options.

	For example, without this patch, trying to build universal
	binaries on a Mac using CFLAGS='-arch i386 -arch x86_64'
	produces

		gcc-4.2: -E, -S, -save-temps and -M options are
		not allowed with multiple -arch flags

	While at it, remove "sh -c" in the command passed to $(shell);
	at this point in the Makefile, SHELL has already been set to
	a sensible shell and it is better not to override that.

Thanks again and sorry for the fuss.

Cheers, Jonathan

The Makefile enables CHECK_HEADER_DEPENDENCIES when the compiler supports generating header dependencies. Make the check use the same flags as the invocation to avoid a false positive when user-configured compiler flags contain incompatible options.

For example, without this patch, trying to build universal binaries on a Mac using CFLAGS='-arch i386 -arch x86_64' produces:

	gcc-4.2: -E, -S, -save-temps and -M options are
	not allowed with multiple -arch flags

Make the check use the same flags as the invocation to avoid false positives when user-configured compiler flags contain incompatible options.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Makefile |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

David Aguilar wrote:

What happened with this third paragraph? It seems to repeat the first one...

Thanks, both; will queue with the final paragraph updated to Jonathan's suggestion.

On Sat, Aug 27, 2011 at 11:26:54AM -0500, Jonathan Nieder wrote:

This has been the behavior for as long as I can remember. I don't think it's a bug. I included the error message in the commit message for [PATCH v2]:

	gcc-4.2: -E, -S, -save-temps and -M options are
	not allowed with multiple -arch flags

I don't think it's a gcc bug. This is just another one of those annoying mac-isms. When building against multiple archs gcc will include a different set of headers for each arch which is likely why the gcc devs do not support it.

I sent a v2 version of the patch that uses $(SHELL_PATH) and omits $(EXTRA_CPPFLAGS).

I think the ideal situation would be for the dependency check to emit headers required across all architectures but that's not how it works. Perhaps there are some internal architectural limitations in gcc that prevent it from being done that way.

My experience has been that most projects DTRT when CFLAGS is configured this way. It's always one or two that require intrusive Makefile or libtool hacks to make them build universal and those are no fun ;-)

BTW after applying this patch I immediately ran into the compat/obstack.[ch] portability problem that I responded to in another thread. I was finally able to make git build with the patch that I included inline there but I think it still needs work. I'll keep an eye on that thread so that we can get a final patch for it.

I also noticed that a few of our compat/ files have gcc/autoconf-isms such as:

#ifdef HAVE_CONFIG_H #include "config.h" #endif

Should I clean these up?  They seem unnecessary.