{"thread":{"id":"28234","subject":"[PATCH] Makefile: Improve compiler header dependency check","startedAt":"2011-08-27T08:41:10Z","lastAt":"2011-08-30T17:17:32Z","messageCount":9,"participants":["David Aguilar","Jonathan Nieder","Fredrik Kuivinen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"174373","messageId":"1314434470-7988-1-git-send-email-davvid@gmail.com","threadId":"28234","inReplyTo":null,"subject":"[PATCH] Makefile: Improve compiler header dependency check","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2011-08-27T08:41:10Z","receivedAt":"2011-08-27T08:41:10Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The Makefile enables CHECK_HEADER_DEPENDENCIES when the\ncompiler supports generating header dependencies.\nMake the check use the same flags as the invocation\nto avoid a false positive when user-configured compiler\nflags contain incompatible options.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nI fired up git's next branch on a mac laptop where I\nhave a config.mak that builds universal git binaries:\n\nCFLAGS = -arch i386 -arch x86_64\n\nThis configuration broke when 111ee18c31f9bac9436426399355facc79238566\nwas merged into next.  gcc cannot generate header dependencies when\nmultiple -arch statements are used but the test generated a false\npositive.\n\n Makefile |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex aa67142..30f3812 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1251,7 +1251,8 @@ USE_COMPUTED_HEADER_DEPENDENCIES =\n else\n ifndef COMPUTE_HEADER_DEPENDENCIES\n dep_check = $(shell sh -c \\\n-\t'$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \\\n+\t'$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null \\\n+\t$(ALL_CFLAGS) $(EXTRA_CPPFLAGS) 2>&1; \\\n \techo $$?')\n ifeq ($(dep_check),0)\n COMPUTE_HEADER_DEPENDENCIES=YesPlease\n-- \n1.7.6.476.g57292\n"},{"id":"174383","messageId":"20110827162645.GA10476@elie.gateway.2wire.net","threadId":"28234","inReplyTo":"1314434470-7988-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] Makefile: Improve compiler header dependency check","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-27T16:26:54Z","receivedAt":"2011-08-27T16:26:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Aguilar wrote:\n\n> I fired up git's next branch on a mac laptop where I\n> have a config.mak that builds universal git binaries:\n>\n> CFLAGS = -arch i386 -arch x86_64\n>\n> This configuration broke when 111ee18c31f9bac9436426399355facc79238566\n> was merged into next.\n\nGood catch; thanks.  This information would be useful for the commit\nmessage.\n\n> gcc cannot generate header dependencies when\n> multiple -arch statements are used\n\nSounds like a bug.  Any idea why it behaves that way?  What error message\ndoes it write?\n\nIf it is a bug, it might be worth reporting this to the gcc devs while\nat it.\n\n[...]\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1251,7 +1251,8 @@ USE_COMPUTED_HEADER_DEPENDENCIES =\n>  else\n>  ifndef COMPUTE_HEADER_DEPENDENCIES\n>  dep_check = $(shell sh -c \\\n> -\t'$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \\\n> +\t'$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null \\\n> +\t$(ALL_CFLAGS) $(EXTRA_CPPFLAGS) 2>&1; \\\n>  \techo $$?')\n\nEXTRA_CPPFLAGS is a target-specific variable and would always be empty,\nSo I think this would be clearer without.\n\nWhile we're touching this line, do you know if the \"sh -c\" is\nnecessary?  I would expect $(shell ...) to run its arguments in a\nshell.\n\nThanks --- despite the nitpicks above, this one looks good.\nJonathan\n"},{"id":"174395","messageId":"1314478844-55379-1-git-send-email-davvid@gmail.com","threadId":"28234","inReplyTo":"20110827162645.GA10476@elie.gateway.2wire.net","subject":"[PATCH v2] Makefile: Improve compiler header dependency check","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2011-08-27T21:00:44Z","receivedAt":"2011-08-27T21:00:44Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Make the check use the same flags as the invocation to avoid\nfalse positives when user-configured compiler flags contain\nincompatible options.\n\nFor example, it is possible to build universal git binaries on\nOS X with the following snippet in config.mak:\n\n\tCFLAGS = -arch i386 -arch x86_64\n\n111ee18c31f9bac9436426399355facc79238566 breaks this setup and\nresults in the following error message:\n\n\tgcc-4.2: -E, -S, -save-temps and -M options are\n\tnot allowed with multiple -arch flags\n\nInclude ALL_CFLAGS so that this and other conditions are caught.\nUse SHELL_PATH instead of assuming that \"sh\" is a sane shell.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nI'm not sure if \"sh -c\" is necessary but I did notice that other\nparts of the Makefile use $(SHELL_PATH).  The check was adjusted\nto use that as well.\n\n Makefile |    5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex aa67142..9446a4e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1250,8 +1250,9 @@ COMPUTE_HEADER_DEPENDENCIES =\n USE_COMPUTED_HEADER_DEPENDENCIES =\n else\n ifndef COMPUTE_HEADER_DEPENDENCIES\n-dep_check = $(shell sh -c \\\n-\t'$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \\\n+dep_check = $(shell $(SHELL_PATH) -c \\\n+\t'$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null \\\n+\t$(ALL_CFLAGS) 2>&1; \\\n \techo $$?')\n ifeq ($(dep_check),0)\n COMPUTE_HEADER_DEPENDENCIES=YesPlease\n-- \n1.7.7.rc0.308.gc820\n"},{"id":"174396","messageId":"20110827211958.GA45286@gmail.com","threadId":"28234","inReplyTo":"20110827162645.GA10476@elie.gateway.2wire.net","subject":"Re: [PATCH] Makefile: Improve compiler header dependency check","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2011-08-27T21:19:59Z","receivedAt":"2011-08-27T21:19:59Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Aug 27, 2011 at 11:26:54AM -0500, Jonathan Nieder wrote:\n> David Aguilar wrote:\n> \n> > I fired up git's next branch on a mac laptop where I\n> > have a config.mak that builds universal git binaries:\n> >\n> > CFLAGS = -arch i386 -arch x86_64\n> >\n> > This configuration broke when 111ee18c31f9bac9436426399355facc79238566\n> > was merged into next.\n> \n> Good catch; thanks.  This information would be useful for the commit\n> message.\n> \n> > gcc cannot generate header dependencies when\n> > multiple -arch statements are used\n> \n> Sounds like a bug.  Any idea why it behaves that way?  What error message\n> does it write?\n> \n> If it is a bug, it might be worth reporting this to the gcc devs while\n> at it.\n\nThis has been the behavior for as long as I can remember.\nI don't think it's a bug.  I included the error message\nin the commit message for [PATCH v2]:\n\n\tgcc-4.2: -E, -S, -save-temps and -M options are\n\tnot allowed with multiple -arch flags\n\nI don't think it's a gcc bug.  This is just another one of\nthose annoying mac-isms.  When building against multiple archs\ngcc will include a different set of headers for each arch\nwhich is likely why the gcc devs do not support it.\n\nI sent a v2 version of the patch that uses $(SHELL_PATH)\nand omits $(EXTRA_CPPFLAGS).\n\nI think the ideal situation would be for the dependency\ncheck to emit headers required across all architectures\nbut that's not how it works.  Perhaps there are some\ninternal architectural limitations in gcc that prevent\nit from being done that way.\n\nMy experience has been that most projects DTRT when CFLAGS\nis configured this way.  It's always one or two that require\nintrusive Makefile or libtool hacks to make them build\nuniversal and those are no fun ;-)\n\nBTW after applying this patch I immediately ran into the\ncompat/obstack.[ch] portability problem that I responded\nto in another thread.  I was finally able to make git\nbuild with the patch that I included inline there but\nI think it still needs work.  I'll keep an eye on that\nthread so that we can get a final patch for it.\n\nI also noticed that a few of our compat/ files have\ngcc/autoconf-isms such as:\n\n#ifdef HAVE_CONFIG_H\n#include \"config.h\"\n#endif\n\nShould I clean these up?  They seem unnecessary.\n-- \n\t\t\t\t\tDavid\n"},{"id":"174412","messageId":"CALx8hKTx3r=ow+=jsCyvZGRJ6Yr+w9TT7=Uyi4y4+beOou45AA@mail.gmail.com","threadId":"28234","inReplyTo":"1314478844-55379-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2] Makefile: Improve compiler header dependency check","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2011-08-28T11:47:46Z","receivedAt":"2011-08-28T11:47:46Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Sat, Aug 27, 2011 at 23:00, David Aguilar <davvid@gmail.com> wrote:\n> Make the check use the same flags as the invocation to avoid\n> false positives when user-configured compiler flags contain\n> incompatible options.\n\n[...]\n\n> I'm not sure if \"sh -c\" is necessary but I did notice that other\n> parts of the Makefile use $(SHELL_PATH).  The check was adjusted\n> to use that as well.\n\nI'm not sure either. I just used what I saw at other places in the Makefile.\n\n[patch snipped]\n\nLooks good to me. Thanks!\n\n- Fredrik\n"},{"id":"174535","messageId":"20110830040515.GC6647@elie.gateway.2wire.net","threadId":"28234","inReplyTo":"CALx8hKTx3r=ow+=jsCyvZGRJ6Yr+w9TT7=Uyi4y4+beOou45AA@mail.gmail.com","subject":"Re: [PATCH v2] Makefile: Improve compiler header dependency check","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-30T04:05:15Z","receivedAt":"2011-08-30T04:05:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nFredrik Kuivinen wrote:\n> On Sat, Aug 27, 2011 at 23:00, David Aguilar <davvid@gmail.com> wrote:\n\n>> I'm not sure if \"sh -c\" is necessary but I did notice that other\n>> parts of the Makefile use $(SHELL_PATH).  The check was adjusted\n>> to use that as well.\n>\n> I'm not sure either. I just used what I saw at other places in the Makefile.\n\nIt is not needed, and imho it makes it harder to read.  I believe the\ncurrent uses of \"sh -c\" near the top of the Makefile are to emphasize\nthat a POSIX shell has not been determined yet (so POSIXy constructs\ncannot be used at that point on platforms like Solaris).\n\nAside from that, this seems good, though.  While at it, the log\nmessage could be simplified to something closer to the original\nversion:\n\n\tThe Makefile enables CHECK_HEADER_DEPENDENCIES when the\n\tcompiler supports generating header dependencies.\n\tMake the check use the same flags as the invocation\n\tto avoid a false positive when user-configured compiler\n\tflags contain incompatible options.\n\n\tFor example, without this patch, trying to build universal\n\tbinaries on a Mac using CFLAGS='-arch i386 -arch x86_64'\n\tproduces\n\n\t\tgcc-4.2: -E, -S, -save-temps and -M options are\n\t\tnot allowed with multiple -arch flags\n\n\tWhile at it, remove \"sh -c\" in the command passed to $(shell);\n\tat this point in the Makefile, SHELL has already been set to\n\ta sensible shell and it is better not to override that.\n\nThanks again and sorry for the fuss.\n\nCheers,\nJonathan\n"},{"id":"174537","messageId":"1314692855-75113-1-git-send-email-davvid@gmail.com","threadId":"28234","inReplyTo":"20110830040515.GC6647@elie.gateway.2wire.net","subject":"[PATCH v3] Makefile: Improve compiler header dependency check","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2011-08-30T08:27:35Z","receivedAt":"2011-08-30T08:27:35Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The Makefile enables CHECK_HEADER_DEPENDENCIES when the\ncompiler supports generating header dependencies.\nMake the check use the same flags as the invocation\nto avoid a false positive when user-configured compiler\nflags contain incompatible options.\n\nFor example, without this patch, trying to build universal\nbinaries on a Mac using CFLAGS='-arch i386 -arch x86_64'\nproduces:\n\n\tgcc-4.2: -E, -S, -save-temps and -M options are\n\tnot allowed with multiple -arch flags\n\nMake the check use the same flags as the invocation to avoid\nfalse positives when user-configured compiler flags contain\nincompatible options.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n Makefile |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex aa67142..0ea1a2b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1250,9 +1250,9 @@ COMPUTE_HEADER_DEPENDENCIES =\n USE_COMPUTED_HEADER_DEPENDENCIES =\n else\n ifndef COMPUTE_HEADER_DEPENDENCIES\n-dep_check = $(shell sh -c \\\n-\t'$(CC) -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \\\n-\techo $$?')\n+dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n+\t-c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \\\n+\techo $$?)\n ifeq ($(dep_check),0)\n COMPUTE_HEADER_DEPENDENCIES=YesPlease\n endif\n-- \n1.7.7.rc0.326.gf688b5\n"},{"id":"174539","messageId":"20110830083311.GB23643@elie.gateway.2wire.net","threadId":"28234","inReplyTo":"1314692855-75113-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v3] Makefile: Improve compiler header dependency check","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-30T08:33:33Z","receivedAt":"2011-08-30T08:33:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Aguilar wrote:\n\n> The Makefile enables CHECK_HEADER_DEPENDENCIES when the\n> compiler supports generating header dependencies.\n> Make the check use the same flags as the invocation\n> to avoid a false positive when user-configured compiler\n> flags contain incompatible options.\n>\n> For example, without this patch, trying to build universal\n> binaries on a Mac using CFLAGS='-arch i386 -arch x86_64'\n> produces:\n>\n> \tgcc-4.2: -E, -S, -save-temps and -M options are\n> \tnot allowed with multiple -arch flags\n>\n> Make the check use the same flags as the invocation to avoid\n> false positives when user-configured compiler flags contain\n> incompatible options.\n\nWhat happened with this third paragraph?  It seems to repeat\nthe first one...\n"},{"id":"174562","messageId":"7vbov63jkj.fsf@alter.siamese.dyndns.org","threadId":"28234","inReplyTo":"1314692855-75113-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v3] Makefile: Improve compiler header dependency check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-30T17:17:32Z","receivedAt":"2011-08-30T17:17:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, both; will queue with the final paragraph updated to Jonathan's\nsuggestion.\n"}]}