Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+
Parker Moorewrites: > From: Parker Moore > > This fixes contrib/persistent-https builds for Go v1.7+ and is > compatible with Go v1.0+. Thanks, applied. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+
Parker Moorewrites: >> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+
> 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+
Shawn Pearcewrites: > On Mon, Jul 18, 2016 at 9:32 PM, Parker Moore 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+
On Mon, Jul 18, 2016 at 9:32 PM, Parker Moorewrote: >> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+
> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+
Jeff Kingwrites: >> 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 ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+
[+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> > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html