{"thread":{"id":"42860","subject":"[PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+","startedAt":"2016-07-15T20:44:20Z","lastAt":"2016-07-20T19:22:10Z","messageCount":8,"participants":["Parker Moore","Jeff King","Junio C Hamano","Shawn Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"291564","messageId":"CAOiOGAcqsdMD0j_ZUdvvGiA9Bd6bUmfvxziN06Rw81f_pO7cgg@mail.gmail.com","threadId":"42860","inReplyTo":null,"subject":"[PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+","fromName":"Parker Moore","fromEmail":"parkrmoore@gmail.com","sentAt":"2016-07-15T20:44:14Z","receivedAt":"2016-07-15T20:44:20Z","isPatch":true,"sender":{"key":"parkrmoore@gmail.com","avatar":"https://avatars.githubusercontent.com/u/237985?v=4"},"body":"From: Parker Moore \n\nThis fixes contrib/persistent-https builds for Go v1.7+ and is\ncompatible with Go v1.5+.\n\nRunning `make all` in `contrib/persistent-https` results in a failure\non Go 1.7 and above.\n\nSpecifically, the error is:\n\n go build -o git-remote-persistent-https \\\n -ldflags \"-X main._BUILD_EMBED_LABEL 1468613136\"\n # _/Users/parkr/github/git/contrib/persistent-https\n /usr/local/Cellar/go/1.7rc1/libexec/pkg/tool/darwin_amd64/link: -X\nflag requires argument of the form importpath.name=value\n make: *** [git-remote-persistent-https] Error 2\n\nThis `name=value` syntax for the -X flag was introduced in Go v1.5\n(released Aug 19, 2015):\n\n- release notes: https://golang.org/doc/go1.5#link\n- commit: https://github.com/golang/go/commit/12795c02f3d6fc54ece09a86e70aaa40a94d5131\n\nIn Go v1.7, support for the old syntax was removed:\n\n- release notes: https://tip.golang.org/doc/go1.7#compiler\n- commit: https://github.com/golang/go/commit/51b624e6a29b135ce0fadb22b678acf4998ff16f\n\nThis patch includes the `=` to fix builds with Go v1.7+.\n---\n contrib/persistent-https/Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/persistent-https/Makefile\nb/contrib/persistent-https/Makefile\nindex 92baa3be..033140b 100644\n--- a/contrib/persistent-https/Makefile\n+++ b/contrib/persistent-https/Makefile\n@@ -26,7 +26,7 @@ git-remote-persistent-http: git-remote-persistent-https\n\n git-remote-persistent-https:\n go build -o git-remote-persistent-https \\\n- -ldflags \"-X main._BUILD_EMBED_LABEL $(BUILD_LABEL)\"\n+ -ldflags \"-X main._BUILD_EMBED_LABEL=$(BUILD_LABEL)\"\n\n clean:\n rm -f git-remote-persistent-http* *.tar.gz\n"},{"id":"291584","messageId":"20160716051800.GA2845@sigill.intra.peff.net","threadId":"42860","inReplyTo":"CAOiOGAcqsdMD0j_ZUdvvGiA9Bd6bUmfvxziN06Rw81f_pO7cgg@mail.gmail.com","subject":"Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-16T05:18:01Z","receivedAt":"2016-07-16T05:18:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Shawn, who participated in the original discussion, as I don't\n think Colby really works on git any more]\n\nOn Fri, Jul 15, 2016 at 01:44:14PM -0700, Parker Moore wrote:\n\n> From: Parker Moore \n> \n> This fixes contrib/persistent-https builds for Go v1.7+ and is\n> compatible with Go v1.5+.\n> \n> Running `make all` in `contrib/persistent-https` results in a failure\n> on Go 1.7 and above.\n> \n> Specifically, the error is:\n> \n> go build -o git-remote-persistent-https \\\n> -ldflags \"-X main._BUILD_EMBED_LABEL 1468613136\"\n> # _/Users/parkr/github/git/contrib/persistent-https\n> /usr/local/Cellar/go/1.7rc1/libexec/pkg/tool/darwin_amd64/link: -X\n> flag requires argument of the form importpath.name=value\n> make: *** [git-remote-persistent-https] Error 2\n> \n> This `name=value` syntax for the -X flag was introduced in Go v1.5\n> (released Aug 19, 2015):\n> \n> - release notes: https://golang.org/doc/go1.5#link\n> - commit: https://github.com/golang/go/commit/12795c02f3d6fc54ece09a86e70aaa40a94d5131\n> \n> In Go v1.7, support for the old syntax was removed:\n> \n> - release notes: https://tip.golang.org/doc/go1.7#compiler\n> - commit: https://github.com/golang/go/commit/51b624e6a29b135ce0fadb22b678acf4998ff16f\n> \n> This patch includes the `=` to fix builds with Go v1.7+.\n\nWith the disclaimer that I have very little experience with Go, this\nseems like a good, well-explained change. My only question would be\nwhether people still use pre-v1.5 versions of Go, since it sounds like\nthis would adversely affect them if they do. (If it does, it seems the\nGo people did not give a very good deprecation period for the syntax\nchange, if people are using both the pre-new-syntax and post-old-syntax\nversions simultaneously). I'm not sure what the alternative is, beyond\nperhaps checking the version of Go dynamically in the Makefile.\n\n-Peff\n"},{"id":"291682","messageId":"xmqqfur6isyr.fsf@gitster.mtv.corp.google.com","threadId":"42860","inReplyTo":"20160716051800.GA2845@sigill.intra.peff.net","subject":"Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-18T20:36:44Z","receivedAt":"2016-07-18T20:37:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> This `name=value` syntax for the -X flag was introduced in Go v1.5\n>> (released Aug 19, 2015):\n>> \n>> - release notes: https://golang.org/doc/go1.5#link\n>> - commit: https://github.com/golang/go/commit/12795c02f3d6fc54ece09a86e70aaa40a94d5131\n>> \n>> In Go v1.7, support for the old syntax was removed:\n>> \n>> - release notes: https://tip.golang.org/doc/go1.7#compiler\n>> - commit: https://github.com/golang/go/commit/51b624e6a29b135ce0fadb22b678acf4998ff16f\n>> \n>> This patch includes the `=` to fix builds with Go v1.7+.\n>\n> With the disclaimer that I have very little experience with Go, this\n> seems like a good, well-explained change. My only question would be\n> whether people still use pre-v1.5 versions of Go, since it sounds like\n> this would adversely affect them if they do. (If it does, it seems the\n\nYeah, you get something like this:\n\n $ ./git-remote-persistent-https --print_label\n 2016/07/18 13:34:33 unlabeled build; build with \"make\" to label\n\nwhich is probably not the end of the world. The label does not even\nidentify the version of the source in any way, so I am not sure how\npeople are depending on that feature anyway ;-)\n\n\n\n"},{"id":"291704","messageId":"CAOiOGAfysmjBxcK-VwscLWBdun4rNsRs85p=foBxETvQw0J1Jw@mail.gmail.com","threadId":"42860","inReplyTo":"xmqqfur6isyr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+","fromName":"Parker Moore","fromEmail":"parkrmoore@gmail.com","sentAt":"2016-07-19T04:32:24Z","receivedAt":"2016-07-19T04:32:31Z","isPatch":true,"sender":{"key":"parkrmoore@gmail.com","avatar":"https://avatars.githubusercontent.com/u/237985?v=4"},"body":"> The label does not even identify the version of the source in any way, so I am not sure how people are depending on that feature anyway ;-)\n\nWould it be a better solution simply to remove this build flag?\nAlternatively, if Git wished to support Go v1.5 and below, I would be\nmore than happy to send a patch with a dynamic lookup in the Makefile\nbased on the output of `go version`. I would be more than happy to\nsubmit either patch.\n"},{"id":"291705","messageId":"CAJo=hJviMd3tWe_HUmh0M0Y2TOCwwrZX14hdyivPSYQ1ck052w@mail.gmail.com","threadId":"42860","inReplyTo":"CAOiOGAfysmjBxcK-VwscLWBdun4rNsRs85p=foBxETvQw0J1Jw@mail.gmail.com","subject":"Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2016-07-19T04:49:15Z","receivedAt":"2016-07-19T04:49:40Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Mon, Jul 18, 2016 at 9:32 PM, Parker Moore wrote:\n>> The label does not even identify the version of the source in any way, so I am not sure how people are depending on that feature anyway ;-)\n>\n> Would it be a better solution simply to remove this build flag?\n> Alternatively, if Git wished to support Go v1.5 and below, I would be\n> more than happy to send a patch with a dynamic lookup in the Makefile\n> based on the output of `go version`. I would be more than happy to\n> submit either patch.\n\nI think we could remove that BUILD_LABEL entirely. Colby liked having\na marker so he knows what \"version\" a user is running, but without any\ncorrelation to source here it just isn't that useful.\n"},{"id":"291731","messageId":"xmqqoa5th84d.fsf@gitster.mtv.corp.google.com","threadId":"42860","inReplyTo":"CAJo=hJviMd3tWe_HUmh0M0Y2TOCwwrZX14hdyivPSYQ1ck052w@mail.gmail.com","subject":"Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-19T17:04:34Z","receivedAt":"2016-07-19T17:04:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> On Mon, Jul 18, 2016 at 9:32 PM, Parker Moore wrote:\n>>> The label does not even identify the version of the source in any way, so I am not sure how people are depending on that feature anyway ;-)\n>>\n>> Would it be a better solution simply to remove this build flag?\n>> Alternatively, if Git wished to support Go v1.5 and below, I would be\n>> more than happy to send a patch with a dynamic lookup in the Makefile\n>> based on the output of `go version`. I would be more than happy to\n>> submit either patch.\n>\n> I think we could remove that BUILD_LABEL entirely. Colby liked having\n> a marker so he knows what \"version\" a user is running, but without any\n> correlation to source here it just isn't that useful.\n\nInside an organization where people use it as a tool supplied by\nsomebody else, who is the designated supplier of it, build-stamp may\nbe sufficient to identify what \"version\" a user is running. If we\nwanted to do a \"here is the source that was built from\", the logical\nplace to pull that information from would be ../../GIT-VERSION-FILE,\nbut the mechanism to embed the information would still be -X var=val\n(or \"-X var val\" for older Go).\n\nSo unless the \"dynamic lookup in the Makefile\" turns out to be too\ngross, we would want to keep the mechanism and just make it usable\nfor versions before 1.5 and also after 1.7, I would guess.\n"},{"id":"291780","messageId":"CAOiOGAdhMvMfDquviuZSJr12UXK-0Fjc8xWeNJc=Wgp1jYmS2w@mail.gmail.com","threadId":"42860","inReplyTo":"xmqqoa5th84d.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+","fromName":"Parker Moore","fromEmail":"parkrmoore@gmail.com","sentAt":"2016-07-19T23:32:48Z","receivedAt":"2016-07-19T23:32:55Z","isPatch":true,"sender":{"key":"parkrmoore@gmail.com","avatar":"https://avatars.githubusercontent.com/u/237985?v=4"},"body":"> the logical place to pull that information from would be ../../GIT-VERSION-FILE,\n\nI agree. It would make more sense to build this to a specific version\nor git revision rather\nthan a time. Perhaps that would be a different patch?\n\n> So unless the \"dynamic lookup in the Makefile\" turns out to be too\n> gross, we would want to keep the mechanism and just make it usable\n> for versions before 1.5 and also after 1.7, I would guess.\n\nA dynamic lookup of the go version would look for go 1.0, 1.1, 1.2,\n1.3, 1.4 and 1.5.0.\nThese versions would be incompatible with the `X var=val` syntax. I am\nnot too familiar\nwith Makefile syntax for numerical comparison, but I believe this\nwould be fairly simple.\nWould you like me to whip up a patch for it?\n"},{"id":"291849","messageId":"xmqq60s0dsj1.fsf@gitster.mtv.corp.google.com","threadId":"42860","inReplyTo":"CAOiOGAdhMvMfDquviuZSJr12UXK-0Fjc8xWeNJc=Wgp1jYmS2w@mail.gmail.com","subject":"Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-20T19:21:54Z","receivedAt":"2016-07-20T19:22:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Parker Moore writes:\n\n>> the logical place to pull that information from would be ../../GIT-VERSION-FILE,\n>\n> I agree. It would make more sense to build this to a specific version\n> or git revision rather\n> than a time. Perhaps that would be a different patch?\n\nIt would definitely be a separate (and optional) patch and must come\nafter \"do we use = in between or send var/val as two separate\narguments?\" patch. That was what I meant to say with \"... would be\nversion file, but the mechanism to embed it would be the same as\ntoday\".\n\n>> So unless the \"dynamic lookup in the Makefile\" turns out to be too\n>> gross, we would want to keep the mechanism and just make it usable\n>> for versions before 1.5 and also after 1.7, I would guess.\n>\n> A dynamic lookup of the go version would look for go 1.0, 1.1, 1.2,\n> 1.3, 1.4 and 1.5.0.\n> These versions would be incompatible with the `X var=val` syntax. I am\n> not too familiar\n> with Makefile syntax for numerical comparison, but I believe this\n> would be fairly simple.\n\n$ go version\ngo version go1.3.3 linux/amd64\n\nis what I seem to be locally getting, so I'd imagine it would be\nsomething like this perhaps?\n\ngit-remote-persistent-https:\n\tcase $$(go version) in \\\n\t\"go version go\"1.[0-5].*) EQ=\" \" ;; *) EQ=\"=\" ;; esac && \\\n\tgo build -o git-remote-persistent-https \\\n\t\t-ldflags \"-X main._BUILD_EMBED_LABEL$${EQ}$(BUILD_LABEL)\"\n\n> Would you like me to whip up a patch for it?\n\nSurely, and thanks.\n"}]}