* [PATCH] Makefile: Improve compiler header dependency check
@ 2011-08-27 8:41 David Aguilar
2011-08-27 16:26 ` Jonathan Nieder
0 siblings, 1 reply; 9+ messages in thread
From: David Aguilar @ 2011-08-27 8:41 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jonathan Nieder, Fredrik Kuivinen, git
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(-)
diff --git a/Makefile b/Makefile
index aa67142..30f3812 100644
--- a/Makefile
+++ b/Makefile
@@ -1251,7 +1251,8 @@ USE_COMPUTED_HEADER_DEPENDENCIES =
else
ifndef COMPUTE_HEADER_DEPENDENCIES
dep_check = $(shell sh -c \
- '$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \
+ '$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null \
+ $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) 2>&1; \
echo $$?')
ifeq ($(dep_check),0)
COMPUTE_HEADER_DEPENDENCIES=YesPlease
--
1.7.6.476.g57292
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Makefile: Improve compiler header dependency check
2011-08-27 8:41 [PATCH] Makefile: Improve compiler header dependency check David Aguilar
@ 2011-08-27 16:26 ` Jonathan Nieder
2011-08-27 21:00 ` [PATCH v2] " David Aguilar
2011-08-27 21:19 ` [PATCH] " David Aguilar
0 siblings, 2 replies; 9+ messages in thread
From: Jonathan Nieder @ 2011-08-27 16:26 UTC (permalink / raw
To: David Aguilar; +Cc: Junio C Hamano, Fredrik Kuivinen, git
David Aguilar wrote:
> 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.
Good catch; thanks. This information would be useful for the commit
message.
> gcc cannot generate header dependencies when
> multiple -arch statements are used
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.
[...]
> --- a/Makefile
> +++ b/Makefile
> @@ -1251,7 +1251,8 @@ USE_COMPUTED_HEADER_DEPENDENCIES =
> else
> ifndef COMPUTE_HEADER_DEPENDENCIES
> dep_check = $(shell sh -c \
> - '$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \
> + '$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null \
> + $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) 2>&1; \
> echo $$?')
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] Makefile: Improve compiler header dependency check
2011-08-27 16:26 ` Jonathan Nieder
@ 2011-08-27 21:00 ` David Aguilar
2011-08-28 11:47 ` Fredrik Kuivinen
2011-08-27 21:19 ` [PATCH] " David Aguilar
1 sibling, 1 reply; 9+ messages in thread
From: David Aguilar @ 2011-08-27 21:00 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jonathan Nieder, Fredrik Kuivinen, git
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(-)
diff --git a/Makefile b/Makefile
index aa67142..9446a4e 100644
--- a/Makefile
+++ b/Makefile
@@ -1250,8 +1250,9 @@ COMPUTE_HEADER_DEPENDENCIES =
USE_COMPUTED_HEADER_DEPENDENCIES =
else
ifndef COMPUTE_HEADER_DEPENDENCIES
-dep_check = $(shell sh -c \
- '$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \
+dep_check = $(shell $(SHELL_PATH) -c \
+ '$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null \
+ $(ALL_CFLAGS) 2>&1; \
echo $$?')
ifeq ($(dep_check),0)
COMPUTE_HEADER_DEPENDENCIES=YesPlease
--
1.7.7.rc0.308.gc820
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Makefile: Improve compiler header dependency check
2011-08-27 16:26 ` Jonathan Nieder
2011-08-27 21:00 ` [PATCH v2] " David Aguilar
@ 2011-08-27 21:19 ` David Aguilar
1 sibling, 0 replies; 9+ messages in thread
From: David Aguilar @ 2011-08-27 21:19 UTC (permalink / raw
To: Jonathan Nieder; +Cc: Junio C Hamano, Fredrik Kuivinen, git
On Sat, Aug 27, 2011 at 11:26:54AM -0500, Jonathan Nieder wrote:
> David Aguilar wrote:
>
> > 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.
>
> Good catch; thanks. This information would be useful for the commit
> message.
>
> > gcc cannot generate header dependencies when
> > multiple -arch statements are used
>
> 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.
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.
--
David
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Makefile: Improve compiler header dependency check
2011-08-27 21:00 ` [PATCH v2] " David Aguilar
@ 2011-08-28 11:47 ` Fredrik Kuivinen
2011-08-30 4:05 ` Jonathan Nieder
0 siblings, 1 reply; 9+ messages in thread
From: Fredrik Kuivinen @ 2011-08-28 11:47 UTC (permalink / raw
To: David Aguilar; +Cc: Junio C Hamano, Jonathan Nieder, git
On Sat, Aug 27, 2011 at 23:00, David Aguilar <davvid@gmail.com> wrote:
> Make the check use the same flags as the invocation to avoid
> false positives when user-configured compiler flags contain
> incompatible options.
[...]
> 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.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Makefile: Improve compiler header dependency check
2011-08-28 11:47 ` Fredrik Kuivinen
@ 2011-08-30 4:05 ` Jonathan Nieder
2011-08-30 8:27 ` [PATCH v3] " David Aguilar
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Nieder @ 2011-08-30 4:05 UTC (permalink / raw
To: Fredrik Kuivinen; +Cc: David Aguilar, Junio C Hamano, git
Hi,
Fredrik Kuivinen wrote:
> On Sat, Aug 27, 2011 at 23:00, David Aguilar <davvid@gmail.com> wrote:
>> 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.
>
> I'm not sure either. I just used what I saw at other places in the Makefile.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] Makefile: Improve compiler header dependency check
2011-08-30 4:05 ` Jonathan Nieder
@ 2011-08-30 8:27 ` David Aguilar
2011-08-30 8:33 ` Jonathan Nieder
2011-08-30 17:17 ` Junio C Hamano
0 siblings, 2 replies; 9+ messages in thread
From: David Aguilar @ 2011-08-30 8:27 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jonathan Nieder, Fredrik Kuivinen, git
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(-)
diff --git a/Makefile b/Makefile
index aa67142..0ea1a2b 100644
--- a/Makefile
+++ b/Makefile
@@ -1250,9 +1250,9 @@ COMPUTE_HEADER_DEPENDENCIES =
USE_COMPUTED_HEADER_DEPENDENCIES =
else
ifndef COMPUTE_HEADER_DEPENDENCIES
-dep_check = $(shell sh -c \
- '$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \
- echo $$?')
+dep_check = $(shell $(CC) $(ALL_CFLAGS) \
+ -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \
+ echo $$?)
ifeq ($(dep_check),0)
COMPUTE_HEADER_DEPENDENCIES=YesPlease
endif
--
1.7.7.rc0.326.gf688b5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] Makefile: Improve compiler header dependency check
2011-08-30 8:27 ` [PATCH v3] " David Aguilar
@ 2011-08-30 8:33 ` Jonathan Nieder
2011-08-30 17:17 ` Junio C Hamano
1 sibling, 0 replies; 9+ messages in thread
From: Jonathan Nieder @ 2011-08-30 8:33 UTC (permalink / raw
To: David Aguilar; +Cc: Junio C Hamano, Fredrik Kuivinen, git
David Aguilar wrote:
> 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.
What happened with this third paragraph? It seems to repeat
the first one...
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] Makefile: Improve compiler header dependency check
2011-08-30 8:27 ` [PATCH v3] " David Aguilar
2011-08-30 8:33 ` Jonathan Nieder
@ 2011-08-30 17:17 ` Junio C Hamano
1 sibling, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2011-08-30 17:17 UTC (permalink / raw
To: David Aguilar; +Cc: Jonathan Nieder, Fredrik Kuivinen, git
Thanks, both; will queue with the final paragraph updated to Jonathan's
suggestion.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-08-30 17:17 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-27 8:41 [PATCH] Makefile: Improve compiler header dependency check David Aguilar
2011-08-27 16:26 ` Jonathan Nieder
2011-08-27 21:00 ` [PATCH v2] " David Aguilar
2011-08-28 11:47 ` Fredrik Kuivinen
2011-08-30 4:05 ` Jonathan Nieder
2011-08-30 8:27 ` [PATCH v3] " David Aguilar
2011-08-30 8:33 ` Jonathan Nieder
2011-08-30 17:17 ` Junio C Hamano
2011-08-27 21:19 ` [PATCH] " David Aguilar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.