{"thread":{"id":"37814","subject":"[PATCH 2/3] Makefile: Reorder linker flags in the git executable rule","startedAt":"2014-10-26T17:33:53Z","lastAt":"2014-10-28T22:12:23Z","messageCount":7,"participants":["David Michael","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"251064","messageId":"87mw8iag72.fsf@gmail.com","threadId":"37814","inReplyTo":null,"subject":"[PATCH 2/3] Makefile: Reorder linker flags in the git executable rule","fromName":"David Michael","fromEmail":"fedora.dm0@gmail.com","sentAt":"2014-10-26T17:33:53Z","receivedAt":"2014-10-26T17:33:53Z","isPatch":true,"sender":{"key":"fedora.dm0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1379865?v=4"},"body":"The XL C compiler can fail due to mixing library path and object\nfile arguments, for example when linking git while building with\n\"gmake LDFLAGS=-L$prefix/lib\".  This moves the ALL_LDFLAGS variable\nexpansion in the git executable rule to be consistent with all the\nother linking rules.\n\nSigned-off-by: David Michael <fedora.dm0@gmail.com>\n---\n Makefile | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex fcd51ac..827006b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1610,8 +1610,8 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \\\n \t'-DGIT_INFO_PATH=\"$(infodir_relative_SQ)\"'\n \n git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)\n-\t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \\\n-\t\t$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)\n+\t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) git.o \\\n+\t\t$(BUILTIN_OBJS) $(LIBS)\n \n help.sp help.s help.o: common-cmds.h\n \n-- \n1.9.3\n"},{"id":"251066","messageId":"CAPig+cRUxXw4b2z1Gu4p6GKjnYrt_70h3kbR+jzbMP_jY24Sjg@mail.gmail.com","threadId":"37814","inReplyTo":"87mw8iag72.fsf@gmail.com","subject":"Re: [PATCH 2/3] Makefile: Reorder linker flags in the git executable rule","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-10-26T17:45:10Z","receivedAt":"2014-10-26T17:45:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 26, 2014 at 1:33 PM, David Michael <fedora.dm0@gmail.com> wrote:\n> The XL C compiler can fail due to mixing library path and object\n\nCan you explain in the commit message the actual nature of the failure\nso that readers can understand more precisely how this change helps?\n\n> file arguments, for example when linking git while building with\n> \"gmake LDFLAGS=-L$prefix/lib\".  This moves the ALL_LDFLAGS variable\n> expansion in the git executable rule to be consistent with all the\n> other linking rules.\n>\n> Signed-off-by: David Michael <fedora.dm0@gmail.com>\n> ---\n>  Makefile | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index fcd51ac..827006b 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1610,8 +1610,8 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \\\n>         '-DGIT_INFO_PATH=\"$(infodir_relative_SQ)\"'\n>\n>  git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)\n> -       $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \\\n> -               $(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)\n> +       $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) git.o \\\n> +               $(BUILTIN_OBJS) $(LIBS)\n>\n>  help.sp help.s help.o: common-cmds.h\n>\n> --\n> 1.9.3\n"},{"id":"251068","messageId":"20141026183530.GA18144@peff.net","threadId":"37814","inReplyTo":"CAPig+cRUxXw4b2z1Gu4p6GKjnYrt_70h3kbR+jzbMP_jY24Sjg@mail.gmail.com","subject":"Re: [PATCH 2/3] Makefile: Reorder linker flags in the git executable rule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-26T18:35:30Z","receivedAt":"2014-10-26T18:35:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 26, 2014 at 01:45:10PM -0400, Eric Sunshine wrote:\n\n> On Sun, Oct 26, 2014 at 1:33 PM, David Michael <fedora.dm0@gmail.com> wrote:\n> > The XL C compiler can fail due to mixing library path and object\n> \n> Can you explain in the commit message the actual nature of the failure\n> so that readers can understand more precisely how this change helps?\n\nBased on past experience, it is probably \"the compiler complains and\nrefuses to run\" (or optionally \"the compiler silently ignores your\nLDFLAGS\" depending on how irritating it wants to be). But it would not\nhurt to be specific.\n\nEither way, the patch looks good; the whole point of LDFLAGS versus LIBS\nis to make this distinction in command-line positioning.\n\n-Peff\n"},{"id":"251070","messageId":"CAEvUa7nMYn1EJhrX+Yo-T53-tqB80p_ym9i+Ua6PMLqZrAFmQw@mail.gmail.com","threadId":"37814","inReplyTo":"20141026183530.GA18144@peff.net","subject":"Re: [PATCH 2/3] Makefile: Reorder linker flags in the git executable rule","fromName":"David Michael","fromEmail":"fedora.dm0@gmail.com","sentAt":"2014-10-26T18:54:56Z","receivedAt":"2014-10-26T18:54:56Z","isPatch":true,"sender":{"key":"fedora.dm0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1379865?v=4"},"body":"On Sun, Oct 26, 2014 at 2:35 PM, Jeff King <peff@peff.net> wrote:\n> On Sun, Oct 26, 2014 at 01:45:10PM -0400, Eric Sunshine wrote:\n>\n>> On Sun, Oct 26, 2014 at 1:33 PM, David Michael <fedora.dm0@gmail.com> wrote:\n>> > The XL C compiler can fail due to mixing library path and object\n>>\n>> Can you explain in the commit message the actual nature of the failure\n>> so that readers can understand more precisely how this change helps?\n>\n> Based on past experience, it is probably \"the compiler complains and\n> refuses to run\" (or optionally \"the compiler silently ignores your\n> LDFLAGS\" depending on how irritating it wants to be). But it would not\n> hurt to be specific.\n\nYes, the compiler refuses to run by default when a \"-L\" option occurs\nafter a source/object file.  It tries to interpret it as another file\nname and fails.\n\nI believe I can work around the error with an \"export _C89_CCMODE=1\",\nbut I thought I'd send the patch since this is the only occurrence of\nthe problem, and the argument order is inconsistent with other linker\ncommands in the file.\n\nIBM documentation has this to say on the noted environment variable:\n\"The default behavior of the c89/cc/c++ command is to expect all\noptions to precede all operands. Setting this variable allows\ncompatibility with historical implementations (other cc commands).\nWhen set to 1, the c89/cc/c++ command operates as follows: Options and\noperands can be interspersed. [...]\"\n\nDo you want me to resend the patch and reference the IBM documentation\nin the message?\n\nThanks.\n\nDavid\n"},{"id":"251087","messageId":"20141027051705.GC2996@peff.net","threadId":"37814","inReplyTo":"CAEvUa7nMYn1EJhrX+Yo-T53-tqB80p_ym9i+Ua6PMLqZrAFmQw@mail.gmail.com","subject":"Re: [PATCH 2/3] Makefile: Reorder linker flags in the git executable rule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-27T05:17:05Z","receivedAt":"2014-10-27T05:17:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 26, 2014 at 02:54:56PM -0400, David Michael wrote:\n\n> Yes, the compiler refuses to run by default when a \"-L\" option occurs\n> after a source/object file.  It tries to interpret it as another file\n> name and fails.\n\nYeah, I think I have seen similar behavior before, but it has been long\nenough that I no longer remember the compiler in use.\n\n> I believe I can work around the error with an \"export _C89_CCMODE=1\",\n> but I thought I'd send the patch since this is the only occurrence of\n> the problem, and the argument order is inconsistent with other linker\n> commands in the file.\n\nI don't think working around it makes sense. That would fix your case,\nbut nobody else's (though given how long it has been that way without\ncomplaints, I suspect any other compilers this picky may have died off).\n\n> Do you want me to resend the patch and reference the IBM documentation\n> in the message?\n\nI don't think you need to. More interesting than documentation is the\nreal-world breakage you experienced and the analysis of the situation.\nI'd be fine taking the patch as-is, or if changing anything, mentioning\nthe failure mode in the commit message.\n\n-Peff\n"},{"id":"251123","messageId":"xmqq61f5flz6.fsf@gitster.dls.corp.google.com","threadId":"37814","inReplyTo":"20141027051705.GC2996@peff.net","subject":"Re: [PATCH 2/3] Makefile: Reorder linker flags in the git executable rule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-27T17:42:21Z","receivedAt":"2014-10-27T17:42:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Oct 26, 2014 at 02:54:56PM -0400, David Michael wrote:\n>\n>> Yes, the compiler refuses to run by default when a \"-L\" option occurs\n>> after a source/object file.  It tries to interpret it as another file\n>> name and fails.\n>\n> Yeah, I think I have seen similar behavior before, but it has been long\n> enough that I no longer remember the compiler in use.\n>\n>> I believe I can work around the error with an \"export _C89_CCMODE=1\",\n>> but I thought I'd send the patch since this is the only occurrence of\n>> the problem, and the argument order is inconsistent with other linker\n>> commands in the file.\n>\n> I don't think working around it makes sense. That would fix your case,\n> but nobody else's (though given how long it has been that way without\n> complaints, I suspect any other compilers this picky may have died off).\n\nI think you meant s/nobody else's/breaks &/;\n\nWith that, I agree with your assessment.  The diff itself is\nprobably fine as-is (I didn't look at it for more than 10 seconds,\nthough ;-).  And I agree that it needs to be better explained.\n\n\n>> Do you want me to resend the patch and reference the IBM documentation\n>> in the message?\n>\n> I don't think you need to. More interesting than documentation is the\n> real-world breakage you experienced and the analysis of the situation.\n> I'd be fine taking the patch as-is, or if changing anything, mentioning\n> the failure mode in the commit message.\n>\n> -Peff\n"},{"id":"251167","messageId":"20141028221223.GA20722@peff.net","threadId":"37814","inReplyTo":"xmqq61f5flz6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] Makefile: Reorder linker flags in the git executable rule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-28T22:12:23Z","receivedAt":"2014-10-28T22:12:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 27, 2014 at 10:42:21AM -0700, Junio C Hamano wrote:\n\n> >> I believe I can work around the error with an \"export _C89_CCMODE=1\",\n> >> but I thought I'd send the patch since this is the only occurrence of\n> >> the problem, and the argument order is inconsistent with other linker\n> >> commands in the file.\n> >\n> > I don't think working around it makes sense. That would fix your case,\n> > but nobody else's (though given how long it has been that way without\n> > complaints, I suspect any other compilers this picky may have died off).\n> \n> I think you meant s/nobody else's/breaks &/;\n\nI meant \"using the _C_89_CCMODE workaround does not help anybody else,\nbecause their compiler will not support it; instead we should fix the\nMakefile as David originally proposed\".\n\nI think we are still agreeing, though. :)\n\n-Peff\n"}]}