Bug 241446 - shells/elvish: Set version information during build
Summary: shells/elvish: Set version information during build
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Dmitri Goutnik
URL:
Keywords: buildisok
Depends on:
Blocks:
 
Reported: 2019-10-23 22:23 UTC by Adam Jimerson
Modified: 2019-10-25 13:55 UTC (History)
1 user (show)

See Also:


Attachments
Patch to Makefile to set version information (1.01 KB, patch)
2019-10-23 22:23 UTC, Adam Jimerson
no flags Details | Diff
elvish-0.12_1.patch v1 (555 bytes, patch)
2019-10-23 23:27 UTC, Dmitri Goutnik
no flags Details | Diff
elvish-0.12_1.patch v2 (648 bytes, patch)
2019-10-24 22:33 UTC, Dmitri Goutnik
dmgk: maintainer-approval?
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Jimerson 2019-10-23 22:23:03 UTC
Created attachment 208543 [details]
Patch to Makefile to set version information

Updated the Makefile to set the version in the binary when the port is built. Using version information from the port Makefile.
Comment 1 Automation User 2019-10-23 22:40:09 UTC
Build info is available at https://gitlab.com/swills/freebsd-ports/pipelines/91089520
Comment 2 Dmitri Goutnik freebsd_committer freebsd_triage 2019-10-23 23:27:44 UTC
Created attachment 208544 [details]
elvish-0.12_1.patch v1

This can be done without patching or gmake by setting GO_BUILDFLAGS in port Makefile. I'll commit the attached patch if you approve :)
Comment 3 Adam Jimerson 2019-10-24 14:38:09 UTC
(In reply to Dmitri Goutnik from comment #2)

I'm all for going with the simpler/easier route, would using the GO_BUILDFLAG ensure that the other variables get set at build time?

The "get" target in the Projects make file just does the following:

	go get -ldflags "-X github.com/elves/elvish/buildinfo.Version=$(VERSION) \
		-X github.com/elves/elvish/buildinfo.GoRoot=$(shell go env GOROOT) \
		-X github.com/elves/elvish/buildinfo.GoPath=$(shell go env GOPATH)" .

So to ensure that the build info also gets set correctly wouldn't the GO_BUILDFLAG need to be updated to the whole value of the ldflags argument i.e.

GO_BUILDFLAGS=	-ldflags="-X github.com/elves/elvish/buildinfo.Version=${DISTVERSIONPREFIX}${DISTVERSION} -X ithub.com/elves/elvish/buildinfo.GoRoot=$(shell go env GOROOT) -X github.com/elves/elvish/buildinfo.GoPath=$(shell go env GOPATH)"

To ensure that everything is set correctly? The main reason for wanting these set is if a user finds a bug/issue that should be reported upstream they may want/need this information in the upstream bug report.

Example (output from another box so version info may be a bit different):

[~]─> elvish -version
0.12.r530.ge2a3f2d8
[~]─> elvish -buildinfo
Version: 0.12.r530.ge2a3f2d8
Go version: go1.13
GOROOT at build time: unknown
GOPATH at build time: unknown

Having the GOROOT and GOPATH at build time may be something the upstream devs may want/need in debugging an issue

To me this feels like at that point we are recreating the projects "get" target in our Makefile. If this is what would be recommended then I'll be happy to reroll my patch and upload it here as i'm trying to learn this process and eran my "maintainer" status :)
Comment 4 Dmitri Goutnik freebsd_committer freebsd_triage 2019-10-24 16:25:48 UTC
I checked the sources and GoRoot/GoPath aren't used for anything except for printing back whatever was set with -ldflags. I'm at loss how knowing GOROOT/GOPATH would be useful for any debugging - the fact that executable was produced is an indicator that GOROOT/GOPATH were set to some reasonable values that worked. There's nothing else in the Makefile and buildall.sh that would be useful for building the port, quite the contrary, using it would make porting harder and bringing in gmake dependency just to build one simple target seems superfluous.
Comment 5 Adam Jimerson 2019-10-24 16:34:41 UTC
I only brought it up, as I'm sure the project had some reason why the would have variables and the '-buildinfo' flag. I was just trying to keep this build as close to how the upstream would would be following their make target. Like you I'm not sure what there reasoning is, I might reach out to the upstream devs and ask them.

It works just fine without it just spits out "unknown" like in the example in my previous comment. I'll reroll my patch without the build dependency on `gmake` and just set everything in the GO_BUILDFLAG variable.
Comment 6 Dmitri Goutnik freebsd_committer freebsd_triage 2019-10-24 20:21:15 UTC
(In reply to Adam Jimerson from comment #5)
https://github.com/elves/elvish/issues/861
Comment 7 Adam Jimerson 2019-10-24 21:19:39 UTC
(In reply to Dmitri Goutnik from comment #6)

Thanks, you beat me to it. Good to know that now that those two variables are considered not needed any more so won't bother setting them in the port.

In that case lets go with your suggested patch as it will be cleaner than mine.
Comment 8 Adam Jimerson 2019-10-24 21:22:24 UTC
(In reply to Adam Jimerson from comment #7)

It seems like the only change might be to add the `-trimpath` argument to the GO_BUILDFLAG as upstream tends to start using that as well https://github.com/elves/elvish/issues/861#issuecomment-546107146
Comment 9 Adam Jimerson 2019-10-24 21:26:33 UTC
Comment on attachment 208544 [details]
elvish-0.12_1.patch v1

This is better than mine.
Comment 10 Dmitri Goutnik freebsd_committer freebsd_triage 2019-10-24 22:33:42 UTC
Created attachment 208589 [details]
elvish-0.12_1.patch v2

With -trimpath and -s -w to omit debug info.
Comment 11 Adam Jimerson 2019-10-24 22:55:27 UTC
Comment on attachment 208589 [details]
elvish-0.12_1.patch v2

Looks good to me
Comment 12 Adam Jimerson 2019-10-24 22:57:55 UTC
Quick question, when I change the "?" to a "+" for the maintainer-approval flag on the patch you submitted on here shouldn't that show up somewhere?
Comment 13 Dmitri Goutnik freebsd_committer freebsd_triage 2019-10-24 23:09:16 UTC
(In reply to Adam Jimerson from comment #12)
Normally, Bugzilla should show "maintainer-approval+" on the patch when you set it to "+" and save, but it might not work with patches submitted by a committer (as I recall from pre-committer days).
Comment 14 Adam Jimerson 2019-10-24 23:14:43 UTC
(In reply to Dmitri Goutnik from comment #13)
Okay just curious as I didn't see any change
Comment 15 commit-hook freebsd_committer freebsd_triage 2019-10-25 12:08:26 UTC
A commit references this bug:

Author: dmgk
Date: Fri Oct 25 12:07:31 UTC 2019
New revision: 515605
URL: https://svnweb.freebsd.org/changeset/ports/515605

Log:
  shells/elvish: Set version information, omit debug info

  PR:		241446
  Approved by:	Adam Jimerson <vendion@gmail.com> (maintainer), araujo (mentor)
  Differential Revision:	https://reviews.freebsd.org/D22149

Changes:
  head/shells/elvish/Makefile
Comment 16 Dmitri Goutnik freebsd_committer freebsd_triage 2019-10-25 12:10:42 UTC
Committed, thanks!
Comment 17 Adam Jimerson 2019-10-25 13:55:21 UTC
Thank you for your help! :)