[PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Parker Moore]

From: Parker Moore <parkrmoore@gmail.com>

This fixes contrib/persistent-https builds for Go v1.7+ and is
compatible with Go v1.5+.

Running `make all` in `contrib/persistent-https` results in a failure
on Go 1.7 and above.

Specifically, the error is:

    go build -o git-remote-persistent-https \
   -ldflags "-X main._BUILD_EMBED_LABEL 1468613136"
    # _/Users/parkr/github/git/contrib/persistent-https
    /usr/local/Cellar/go/1.7rc1/libexec/pkg/tool/darwin_amd64/link: -X
flag requires argument of the form importpath.name=value
    make: *** [git-remote-persistent-https] Error 2

This `name=value` syntax for the -X flag was introduced in Go v1.5
(released Aug 19, 2015):

- release notes: https://golang.org/doc/go1.5#link
- commit: https://github.com/golang/go/commit/12795c02f3d6fc54ece09a86e70aaa40a94d5131

In Go v1.7, support for the old syntax was removed:

- release notes: https://tip.golang.org/doc/go1.7#compiler
- commit: https://github.com/golang/go/commit/51b624e6a29b135ce0fadb22b678acf4998ff16f

This patch includes the `=` to fix builds with Go v1.7+.
---
 contrib/persistent-https/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/persistent-https/Makefile
b/contrib/persistent-https/Makefile
index 92baa3be..033140b 100644
--- a/contrib/persistent-https/Makefile
+++ b/contrib/persistent-https/Makefile
@@ -26,7 +26,7 @@ git-remote-persistent-http: git-remote-persistent-https

 git-remote-persistent-https:
  go build -o git-remote-persistent-https \
- -ldflags "-X main._BUILD_EMBED_LABEL $(BUILD_LABEL)"
+ -ldflags "-X main._BUILD_EMBED_LABEL=$(BUILD_LABEL)"

 clean:
  rm -f git-remote-persistent-http* *.tar.gz

Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Jeff King]

[+cc Shawn, who participated in the original discussion, as I don't
  think Colby really works on git any more]

On Fri, Jul 15, 2016 at 01:44:14PM -0700, Parker Moore wrote:

> From: Parker Moore <parkrmoore@gmail.com>
> 
> This fixes contrib/persistent-https builds for Go v1.7+ and is
> compatible with Go v1.5+.
> 
> Running `make all` in `contrib/persistent-https` results in a failure
> on Go 1.7 and above.
> 
> Specifically, the error is:
> 
>     go build -o git-remote-persistent-https \
>    -ldflags "-X main._BUILD_EMBED_LABEL 1468613136"
>     # _/Users/parkr/github/git/contrib/persistent-https
>     /usr/local/Cellar/go/1.7rc1/libexec/pkg/tool/darwin_amd64/link: -X
> flag requires argument of the form importpath.name=value
>     make: *** [git-remote-persistent-https] Error 2
> 
> This `name=value` syntax for the -X flag was introduced in Go v1.5
> (released Aug 19, 2015):
> 
> - release notes: https://golang.org/doc/go1.5#link
> - commit: https://github.com/golang/go/commit/12795c02f3d6fc54ece09a86e70aaa40a94d5131
> 
> In Go v1.7, support for the old syntax was removed:
> 
> - release notes: https://tip.golang.org/doc/go1.7#compiler
> - commit: https://github.com/golang/go/commit/51b624e6a29b135ce0fadb22b678acf4998ff16f
> 
> This patch includes the `=` to fix builds with Go v1.7+.

With the disclaimer that I have very little experience with Go, this
seems like a good, well-explained change. My only question would be
whether people still use pre-v1.5 versions of Go, since it sounds like
this would adversely affect them if they do. (If it does, it seems the
Go people did not give a very good deprecation period for the syntax
change, if people are using both the pre-new-syntax and post-old-syntax
versions simultaneously). I'm not sure what the alternative is, beyond
perhaps checking the version of Go dynamically in the Makefile.

-Peff

Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> This `name=value` syntax for the -X flag was introduced in Go v1.5
>> (released Aug 19, 2015):
>> 
>> - release notes: https://golang.org/doc/go1.5#link
>> - commit: https://github.com/golang/go/commit/12795c02f3d6fc54ece09a86e70aaa40a94d5131
>> 
>> In Go v1.7, support for the old syntax was removed:
>> 
>> - release notes: https://tip.golang.org/doc/go1.7#compiler
>> - commit: https://github.com/golang/go/commit/51b624e6a29b135ce0fadb22b678acf4998ff16f
>> 
>> This patch includes the `=` to fix builds with Go v1.7+.
>
> With the disclaimer that I have very little experience with Go, this
> seems like a good, well-explained change. My only question would be
> whether people still use pre-v1.5 versions of Go, since it sounds like
> this would adversely affect them if they do. (If it does, it seems the

Yeah, you get something like this:

    $ ./git-remote-persistent-https --print_label
    2016/07/18 13:34:33 unlabeled build; build with "make" to label

which is probably not the end of the world.  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 ;-)

Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Parker Moore]

> 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 ;-)

Would it be a better solution simply to remove this build flag?
Alternatively, if Git wished to support Go v1.5 and below, I would be
more than happy to send a patch with a dynamic lookup in the Makefile
based on the output of `go version`. I would be more than happy to
submit either patch.

Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Shawn Pearce]

On Mon, Jul 18, 2016 at 9:32 PM, Parker Moore <parkrmoore@gmail.com> wrote:
>> 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 ;-)
>
> Would it be a better solution simply to remove this build flag?
> Alternatively, if Git wished to support Go v1.5 and below, I would be
> more than happy to send a patch with a dynamic lookup in the Makefile
> based on the output of `go version`. I would be more than happy to
> submit either patch.

I think we could remove that BUILD_LABEL entirely. Colby liked having
a marker so he knows what "version" a user is running, but without any
correlation to source here it just isn't that useful.

Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> On Mon, Jul 18, 2016 at 9:32 PM, Parker Moore <parkrmoore@gmail.com> wrote:
>>> 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 ;-)
>>
>> Would it be a better solution simply to remove this build flag?
>> Alternatively, if Git wished to support Go v1.5 and below, I would be
>> more than happy to send a patch with a dynamic lookup in the Makefile
>> based on the output of `go version`. I would be more than happy to
>> submit either patch.
>
> I think we could remove that BUILD_LABEL entirely. Colby liked having
> a marker so he knows what "version" a user is running, but without any
> correlation to source here it just isn't that useful.

Inside an organization where people use it as a tool supplied by
somebody else, who is the designated supplier of it, build-stamp may
be sufficient to identify what "version" a user is running.  If we
wanted to do a "here is the source that was built from", the logical
place to pull that information from would be ../../GIT-VERSION-FILE,
but the mechanism to embed the information would still be -X var=val
(or "-X var val" for older Go).

So unless the "dynamic lookup in the Makefile" turns out to be too
gross, we would want to keep the mechanism and just make it usable
for versions before 1.5 and also after 1.7, I would guess.

Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Parker Moore]

> the logical place to pull that information from would be ../../GIT-VERSION-FILE,

I agree. It would make more sense to build this to a specific version
or git revision rather
than a time. Perhaps that would be a different patch?

> So unless the "dynamic lookup in the Makefile" turns out to be too
> gross, we would want to keep the mechanism and just make it usable
> for versions before 1.5 and also after 1.7, I would guess.

A dynamic lookup of the go version would look for go 1.0, 1.1, 1.2,
1.3, 1.4 and 1.5.0.
These versions would be incompatible with the `X var=val` syntax. I am
not too familiar
with Makefile syntax for numerical comparison, but I believe this
would be fairly simple.
Would you like me to whip up a patch for it?

Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Junio C Hamano]

Parker Moore <parkrmoore@gmail.com> writes:

>> the logical place to pull that information from would be ../../GIT-VERSION-FILE,
>
> I agree. It would make more sense to build this to a specific version
> or git revision rather
> than a time. Perhaps that would be a different patch?

It would definitely be a separate (and optional) patch and must come
after "do we use = in between or send var/val as two separate
arguments?" patch.  That was what I meant to say with "... would be
version file, but the mechanism to embed it would be the same as
today".

>> So unless the "dynamic lookup in the Makefile" turns out to be too
>> gross, we would want to keep the mechanism and just make it usable
>> for versions before 1.5 and also after 1.7, I would guess.
>
> A dynamic lookup of the go version would look for go 1.0, 1.1, 1.2,
> 1.3, 1.4 and 1.5.0.
> These versions would be incompatible with the `X var=val` syntax. I am
> not too familiar
> with Makefile syntax for numerical comparison, but I believe this
> would be fairly simple.

$ go version
go version go1.3.3 linux/amd64

is what I seem to be locally getting, so I'd imagine it would be
something like this perhaps?

git-remote-persistent-https:
	case $$(go version) in \
	"go version go"1.[0-5].*) EQ=" " ;; *) EQ="=" ;; esac && \
	go build -o git-remote-persistent-https \
		-ldflags "-X main._BUILD_EMBED_LABEL$${EQ}$(BUILD_LABEL)"

> Would you like me to whip up a patch for it?

Surely, and thanks.

[PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Parker Moore]

From: Parker Moore <parkrmoore@gmail.com>

This fixes contrib/persistent-https builds for Go v1.7+ and is
compatible with Go v1.0+.

Running `make all` in `contrib/persistent-https` results in a failure
on Go 1.7 and above.

Specifically, the error is:

    go build -o git-remote-persistent-https \
   -ldflags "-X main._BUILD_EMBED_LABEL 1468613136"
    # _/Users/parkr/github/git/contrib/persistent-https
    /usr/local/Cellar/go/1.7rc1/libexec/pkg/tool/darwin_amd64/link: -X
flag requires argument of the form importpath.name=value
    make: *** [git-remote-persistent-https] Error 2

This `name=value` syntax for the -X flag was introduced in Go v1.5
(released Aug 19, 2015):

- release notes: https://golang.org/doc/go1.5#link
- commit: https://github.com/golang/go/commit/12795c02f3d6fc54ece09a86e70aaa40a94d5131

In Go v1.7, support for the old syntax was removed:

- release notes: https://tip.golang.org/doc/go1.7#compiler
- commit: https://github.com/golang/go/commit/51b624e6a29b135ce0fadb22b678acf4998ff16f

This patch includes the `=` to fix builds with Go v1.7+.

Thanks-to: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Parker Moore <parkrmoore@gmail.com>
---
 contrib/persistent-https/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/persistent-https/Makefile
b/contrib/persistent-https/Makefile
index 92baa3be..2b4173d 100644
--- a/contrib/persistent-https/Makefile
+++ b/contrib/persistent-https/Makefile
@@ -25,8 +25,10 @@ git-remote-persistent-http: git-remote-persistent-https
        ln -f -s git-remote-persistent-https git-remote-persistent-http

 git-remote-persistent-https:
+       case $$(go version) in \
+               "go version go"1.[0-5].*) EQ=" " ;; *) EQ="=" ;; esac && \
        go build -o git-remote-persistent-https \
-               -ldflags "-X main._BUILD_EMBED_LABEL $(BUILD_LABEL)"
+               -ldflags "-X main._BUILD_EMBED_LABEL$${EQ}$(BUILD_LABEL)"

 clean:
        rm -f git-remote-persistent-http* *.tar.gz

Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Junio C Hamano]

Parker Moore <parkrmoore@gmail.com> writes:

> From: Parker Moore <parkrmoore@gmail.com>
>
> This fixes contrib/persistent-https builds for Go v1.7+ and is
> compatible with Go v1.0+.

Thanks, applied.