Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Stuart Henderson]

On 2016/05/05 11:29, Dmitrij D. Czarkoff wrote:
> Antoine Jacoutot said:
> > I am OK with this but why don't you use INSTALL_PROGRAM in the 
> > MODGO_INSTALL_TARGET then, instead of cp?
> > I'd give us more control about the owner/mode no?
> 
> The patch below adds this change.  But pkg_create already resets
> owner/mode anyway, so I am not sure it is really needed.

pkg_create checks but doesn't reset. (I run into package failures
all over the place when I'm on a machine with umask 077 and try
to build a package without remembering to loosen it...)

> FWIW I planned to switch MODGO_INSTALL_TARGET for libraries to
> INSTALL_DATA, but I don't think this should be done in this commit.
> 
> -- 
> Dmitrij D. Czarkoff
> 
> Index: lang/go/Makefile


ok

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Antoine Jacoutot]

On Thu, May 05, 2016 at 11:29:37AM +0200, Dmitrij D. Czarkoff wrote:
> Antoine Jacoutot said:
> > I am OK with this but why don't you use INSTALL_PROGRAM in the 
> > MODGO_INSTALL_TARGET then, instead of cp?
> > I'd give us more control about the owner/mode no?
> 
> The patch below adds this change.  But pkg_create already resets
> owner/mode anyway, so I am not sure it is really needed.
> 
> FWIW I planned to switch MODGO_INSTALL_TARGET for libraries to
> INSTALL_DATA, but I don't think this should be done in this commit.

OK aja


> 
> -- 
> Dmitrij D. Czarkoff
> 
> Index: lang/go/Makefile
> ===
> RCS file: /cvs/ports/lang/go/Makefile,v
> retrieving revision 1.29
> diff -u -p -r1.29 Makefile
> --- lang/go/Makefile  14 Apr 2016 17:43:26 -  1.29
> +++ lang/go/Makefile  4 May 2016 21:46:35 -
> @@ -8,6 +8,7 @@ VERSION = 1.5.4
>  EXTRACT_SUFX =   .src.tar.gz
>  DISTNAME =   go${VERSION}
>  PKGNAME =go-${VERSION}
> +REVISION =   0
>  CATEGORIES = lang
>  
>  HOMEPAGE =   https://golang.org/
> @@ -29,6 +30,8 @@ SUBST_VARS =GOCFG
>  
>  WRKDIST =${WRKDIR}/go
>  WRKSRC = ${WRKDIST}/src
> +
> +INSTALL_STRIP =
>  
>  GOOS =   openbsd
>  GOARCH = unknown
> Index: lang/go/go.port.mk
> ===
> RCS file: /cvs/ports/lang/go/go.port.mk,v
> retrieving revision 1.6
> diff -u -p -r1.6 go.port.mk
> --- lang/go/go.port.mk28 Feb 2016 13:24:16 -  1.6
> +++ lang/go/go.port.mk5 May 2016 08:28:24 -
> @@ -40,8 +40,9 @@ CATEGORIES +=   lang/go
>  MODGO_BUILD_TARGET = ${MODGO_BUILD_CMD} ${ALL_TARGET}
>  MODGO_FLAGS ?=   -x
>  
> +INSTALL_STRIP =
>  .if ${MODGO_TYPE:L:Mbin}
> -MODGO_INSTALL_TARGET =   cp ${MODGO_WORKSPACE}/bin/* ${PREFIX}/bin
> +MODGO_INSTALL_TARGET =   ${INSTALL_PROGRAM} ${MODGO_WORKSPACE}/bin/* 
> ${PREFIX}/bin
>  .endif
>  
>  # Go source files serve the purpose of libraries, so sources should be 
> included
> Index: lang/go-bootstrap/Makefile
> ===
> RCS file: /cvs/ports/lang/go-bootstrap/Makefile,v
> retrieving revision 1.1.1.1
> diff -u -p -r1.1.1.1 Makefile
> --- lang/go-bootstrap/Makefile4 Dec 2015 17:19:18 -   1.1.1.1
> +++ lang/go-bootstrap/Makefile4 May 2016 21:48:19 -
> @@ -8,6 +8,7 @@ VERSION = 1.4.3
>  EXTRACT_SUFX =   .src.tar.gz
>  DISTNAME =   go${VERSION}
>  PKGNAME =go-bootstrap-${VERSION}
> +REVISION =   0
>  CATEGORIES = lang
>  
>  HOMEPAGE =   https://golang.org/
> @@ -31,6 +32,8 @@ SUBST_VARS =GOEXE GOCFG
>  
>  WRKDIST =${WRKDIR}/go
>  WRKSRC = ${WRKDIST}/src
> +
> +INSTALL_STRIP =
>  
>  GOOS =   openbsd
>  GOARCH = unknown
> Index: net/syncthing/Makefile
> ===
> RCS file: /cvs/ports/net/syncthing/Makefile,v
> retrieving revision 1.2
> diff -u -p -r1.2 Makefile
> --- net/syncthing/Makefile23 Apr 2016 08:02:05 -  1.2
> +++ net/syncthing/Makefile4 May 2016 21:51:13 -
> @@ -34,8 +34,7 @@ do-test:
>   cd ${WRKSRC} && ${MODGO_CMD} run build.go test
>  
>  do-install:
> - # Note: Don't use INSTALL_PROGRAM. It strips, and go hates this.
> - ${INSTALL_SCRIPT} ${WRKSRC}/bin/syncthing ${PREFIX}/bin/
> + ${INSTALL_PROGRAM} ${WRKSRC}/bin/syncthing ${PREFIX}/bin/
>  .for sec in 1 5 7
>   ${INSTALL_MAN} ${WRKSRC}/man/*.${sec} ${PREFIX}/man/man${sec}/
>  .endfor
> Index: sysutils/terraform/Makefile
> ===
> RCS file: /cvs/ports/sysutils/terraform/Makefile,v
> retrieving revision 1.4
> diff -u -p -r1.4 Makefile
> --- sysutils/terraform/Makefile   3 May 2016 16:16:47 -   1.4
> +++ sysutils/terraform/Makefile   4 May 2016 21:53:15 -
> @@ -35,9 +35,6 @@ PROVIDERS=  atlas aws azure azurerm chef
>  
>  PROVISIONERS=chef file local-exec remote-exec
>  
> -# prevent stripping go binaries
> -INSTALL_STRIP=
> -
>  post-build:
>  .for provider in ${PROVIDERS}
>   cd ${WRKSRC} && \

-- 
Antoine

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Dmitrij D. Czarkoff]

Antoine Jacoutot said:
> I am OK with this but why don't you use INSTALL_PROGRAM in the 
> MODGO_INSTALL_TARGET then, instead of cp?
> I'd give us more control about the owner/mode no?

The patch below adds this change.  But pkg_create already resets
owner/mode anyway, so I am not sure it is really needed.

FWIW I planned to switch MODGO_INSTALL_TARGET for libraries to
INSTALL_DATA, but I don't think this should be done in this commit.

-- 
Dmitrij D. Czarkoff

Index: lang/go/Makefile
===
RCS file: /cvs/ports/lang/go/Makefile,v
retrieving revision 1.29
diff -u -p -r1.29 Makefile
--- lang/go/Makefile14 Apr 2016 17:43:26 -  1.29
+++ lang/go/Makefile4 May 2016 21:46:35 -
@@ -8,6 +8,7 @@ VERSION =   1.5.4
 EXTRACT_SUFX = .src.tar.gz
 DISTNAME = go${VERSION}
 PKGNAME =  go-${VERSION}
+REVISION = 0
 CATEGORIES =   lang
 
 HOMEPAGE = https://golang.org/
@@ -29,6 +30,8 @@ SUBST_VARS =  GOCFG
 
 WRKDIST =  ${WRKDIR}/go
 WRKSRC =   ${WRKDIST}/src
+
+INSTALL_STRIP =
 
 GOOS = openbsd
 GOARCH =   unknown
Index: lang/go/go.port.mk
===
RCS file: /cvs/ports/lang/go/go.port.mk,v
retrieving revision 1.6
diff -u -p -r1.6 go.port.mk
--- lang/go/go.port.mk  28 Feb 2016 13:24:16 -  1.6
+++ lang/go/go.port.mk  5 May 2016 08:28:24 -
@@ -40,8 +40,9 @@ CATEGORIES += lang/go
 MODGO_BUILD_TARGET =   ${MODGO_BUILD_CMD} ${ALL_TARGET}
 MODGO_FLAGS ?= -x
 
+INSTALL_STRIP =
 .if ${MODGO_TYPE:L:Mbin}
-MODGO_INSTALL_TARGET = cp ${MODGO_WORKSPACE}/bin/* ${PREFIX}/bin
+MODGO_INSTALL_TARGET = ${INSTALL_PROGRAM} ${MODGO_WORKSPACE}/bin/* 
${PREFIX}/bin
 .endif
 
 # Go source files serve the purpose of libraries, so sources should be included
Index: lang/go-bootstrap/Makefile
===
RCS file: /cvs/ports/lang/go-bootstrap/Makefile,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 Makefile
--- lang/go-bootstrap/Makefile  4 Dec 2015 17:19:18 -   1.1.1.1
+++ lang/go-bootstrap/Makefile  4 May 2016 21:48:19 -
@@ -8,6 +8,7 @@ VERSION =   1.4.3
 EXTRACT_SUFX = .src.tar.gz
 DISTNAME = go${VERSION}
 PKGNAME =  go-bootstrap-${VERSION}
+REVISION = 0
 CATEGORIES =   lang
 
 HOMEPAGE = https://golang.org/
@@ -31,6 +32,8 @@ SUBST_VARS =  GOEXE GOCFG
 
 WRKDIST =  ${WRKDIR}/go
 WRKSRC =   ${WRKDIST}/src
+
+INSTALL_STRIP =
 
 GOOS = openbsd
 GOARCH =   unknown
Index: net/syncthing/Makefile
===
RCS file: /cvs/ports/net/syncthing/Makefile,v
retrieving revision 1.2
diff -u -p -r1.2 Makefile
--- net/syncthing/Makefile  23 Apr 2016 08:02:05 -  1.2
+++ net/syncthing/Makefile  4 May 2016 21:51:13 -
@@ -34,8 +34,7 @@ do-test:
cd ${WRKSRC} && ${MODGO_CMD} run build.go test
 
 do-install:
-   # Note: Don't use INSTALL_PROGRAM. It strips, and go hates this.
-   ${INSTALL_SCRIPT} ${WRKSRC}/bin/syncthing ${PREFIX}/bin/
+   ${INSTALL_PROGRAM} ${WRKSRC}/bin/syncthing ${PREFIX}/bin/
 .for sec in 1 5 7
${INSTALL_MAN} ${WRKSRC}/man/*.${sec} ${PREFIX}/man/man${sec}/
 .endfor
Index: sysutils/terraform/Makefile
===
RCS file: /cvs/ports/sysutils/terraform/Makefile,v
retrieving revision 1.4
diff -u -p -r1.4 Makefile
--- sysutils/terraform/Makefile 3 May 2016 16:16:47 -   1.4
+++ sysutils/terraform/Makefile 4 May 2016 21:53:15 -
@@ -35,9 +35,6 @@ PROVIDERS=atlas aws azure azurerm chef
 
 PROVISIONERS=  chef file local-exec remote-exec
 
-# prevent stripping go binaries
-INSTALL_STRIP=
-
 post-build:
 .for provider in ${PROVIDERS}
cd ${WRKSRC} && \

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Antoine Jacoutot]

On Thu, May 05, 2016 at 12:07:01AM +0200, Dmitrij D. Czarkoff wrote:
> Dmitrij D. Czarkoff said:
> > Stuart Henderson said:
> > > On 2016/04/21 19:35, Christian Weisgerber wrote:
> > >> Dmitrij D. Czarkoff:
> > >> 
> >  I think you should just set
> >  INSTALL_STRIP =
> >  in the ports (or modules) that require it and be done with it.
> >  No point in introducing yet another variable.
> > >>> 
> > >>> INSTALL_STRIP is mentioned in mk.conf(5), so it is a user setting.
> > >>> /etc/mk.conf may contain INSTALL_STRIP=-s, and that would override
> > >>> INSTALL_STRIP= from port.
> > >> 
> > >> In practice, INSTALL_STRIP has two possible values: "-s" and "".
> > >> "-s" is already the default, so the only user setting that makes
> > >> any sense is "".  Having some ports already set it to "" is just
> > >> fine.
> > >> 
> > >> A few ports already do this.
> > > 
> > > Makes sense to me. go-bootstrap needs it too.
> > 
> > Well, then asking for OKs for the following patch.
> 
> I totally forgot about this.  Updated version of the patch follows.  It
> touchs directly:
> 
>  * lang/go
>  * lang/go-bootstrap
>  * net/syncthing
>  * sysutils/terraform
> 
> and indirectly every port with MODULES=lang/go.  That said, only lang/go
> and lang/go-bootstrap suffer changes in binaries - other ports either
> include own workarounds I remove or use MODGO_INSTALL_TARGET from
> lang/go MODULE, which never had this issue in the first place.

I am OK with this but why don't you use INSTALL_PROGRAM in the 
MODGO_INSTALL_TARGET then, instead of cp?
I'd give us more control about the owner/mode no?


> -- 
> Dmitrij D. Czarkoff
> 
> Index: lang/go/Makefile
> ===
> RCS file: /cvs/ports/lang/go/Makefile,v
> retrieving revision 1.29
> diff -u -p -r1.29 Makefile
> --- lang/go/Makefile  14 Apr 2016 17:43:26 -  1.29
> +++ lang/go/Makefile  4 May 2016 21:46:35 -
> @@ -8,6 +8,7 @@ VERSION = 1.5.4
>  EXTRACT_SUFX =   .src.tar.gz
>  DISTNAME =   go${VERSION}
>  PKGNAME =go-${VERSION}
> +REVISION =   0
>  CATEGORIES = lang
>  
>  HOMEPAGE =   https://golang.org/
> @@ -29,6 +30,8 @@ SUBST_VARS =GOCFG
>  
>  WRKDIST =${WRKDIR}/go
>  WRKSRC = ${WRKDIST}/src
> +
> +INSTALL_STRIP =
>  
>  GOOS =   openbsd
>  GOARCH = unknown
> Index: lang/go/go.port.mk
> ===
> RCS file: /cvs/ports/lang/go/go.port.mk,v
> retrieving revision 1.6
> diff -u -p -r1.6 go.port.mk
> --- lang/go/go.port.mk28 Feb 2016 13:24:16 -  1.6
> +++ lang/go/go.port.mk4 May 2016 21:47:45 -
> @@ -53,6 +53,10 @@ MODGO_INSTALL_TARGET = ${INSTALL_DATA_DI
>   ${MODGO_PACKAGE_PATH};
>  .endif
>  
> +# Although this module does not use INSTALL_PROGRAM, unset INSTALL_STRIP to
> +# prevent stripping go programs in ports with custom do-install targets.
> +INSTALL_STRIP =
> +
>  MODGO_TEST_TARGET =  ${MODGO_TEST_CMD} ${TEST_TARGET}
>  
>  .if empty(CONFIGURE_STYLE)
> Index: lang/go-bootstrap/Makefile
> ===
> RCS file: /cvs/ports/lang/go-bootstrap/Makefile,v
> retrieving revision 1.1.1.1
> diff -u -p -r1.1.1.1 Makefile
> --- lang/go-bootstrap/Makefile4 Dec 2015 17:19:18 -   1.1.1.1
> +++ lang/go-bootstrap/Makefile4 May 2016 21:48:19 -
> @@ -8,6 +8,7 @@ VERSION = 1.4.3
>  EXTRACT_SUFX =   .src.tar.gz
>  DISTNAME =   go${VERSION}
>  PKGNAME =go-bootstrap-${VERSION}
> +REVISION =   0
>  CATEGORIES = lang
>  
>  HOMEPAGE =   https://golang.org/
> @@ -31,6 +32,8 @@ SUBST_VARS =GOEXE GOCFG
>  
>  WRKDIST =${WRKDIR}/go
>  WRKSRC = ${WRKDIST}/src
> +
> +INSTALL_STRIP =
>  
>  GOOS =   openbsd
>  GOARCH = unknown
> Index: net/syncthing/Makefile
> ===
> RCS file: /cvs/ports/net/syncthing/Makefile,v
> retrieving revision 1.2
> diff -u -p -r1.2 Makefile
> --- net/syncthing/Makefile23 Apr 2016 08:02:05 -  1.2
> +++ net/syncthing/Makefile4 May 2016 21:51:13 -
> @@ -34,8 +34,7 @@ do-test:
>   cd ${WRKSRC} && ${MODGO_CMD} run build.go test
>  
>  do-install:
> - # Note: Don't use INSTALL_PROGRAM. It strips, and go hates this.
> - ${INSTALL_SCRIPT} ${WRKSRC}/bin/syncthing ${PREFIX}/bin/
> + ${INSTALL_PROGRAM} ${WRKSRC}/bin/syncthing ${PREFIX}/bin/
>  .for sec in 1 5 7
>   ${INSTALL_MAN} ${WRKSRC}/man/*.${sec} ${PREFIX}/man/man${sec}/
>  .endfor
> Index: sysutils/terraform/Makefile
> ===
> RCS file: /cvs/ports/sysutils/terraform/Makefile,v
> retrieving revision 1.4
> diff 

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Dmitrij D. Czarkoff]

Dmitrij D. Czarkoff said:
> Stuart Henderson said:
> > On 2016/04/21 19:35, Christian Weisgerber wrote:
> >> Dmitrij D. Czarkoff:
> >> 
>  I think you should just set
>  INSTALL_STRIP =
>  in the ports (or modules) that require it and be done with it.
>  No point in introducing yet another variable.
> >>> 
> >>> INSTALL_STRIP is mentioned in mk.conf(5), so it is a user setting.
> >>> /etc/mk.conf may contain INSTALL_STRIP=-s, and that would override
> >>> INSTALL_STRIP= from port.
> >> 
> >> In practice, INSTALL_STRIP has two possible values: "-s" and "".
> >> "-s" is already the default, so the only user setting that makes
> >> any sense is "".  Having some ports already set it to "" is just
> >> fine.
> >> 
> >> A few ports already do this.
> > 
> > Makes sense to me. go-bootstrap needs it too.
> 
> Well, then asking for OKs for the following patch.

I totally forgot about this.  Updated version of the patch follows.  It
touchs directly:

 * lang/go
 * lang/go-bootstrap
 * net/syncthing
 * sysutils/terraform

and indirectly every port with MODULES=lang/go.  That said, only lang/go
and lang/go-bootstrap suffer changes in binaries - other ports either
include own workarounds I remove or use MODGO_INSTALL_TARGET from
lang/go MODULE, which never had this issue in the first place.

-- 
Dmitrij D. Czarkoff

Index: lang/go/Makefile
===
RCS file: /cvs/ports/lang/go/Makefile,v
retrieving revision 1.29
diff -u -p -r1.29 Makefile
--- lang/go/Makefile14 Apr 2016 17:43:26 -  1.29
+++ lang/go/Makefile4 May 2016 21:46:35 -
@@ -8,6 +8,7 @@ VERSION =   1.5.4
 EXTRACT_SUFX = .src.tar.gz
 DISTNAME = go${VERSION}
 PKGNAME =  go-${VERSION}
+REVISION = 0
 CATEGORIES =   lang
 
 HOMEPAGE = https://golang.org/
@@ -29,6 +30,8 @@ SUBST_VARS =  GOCFG
 
 WRKDIST =  ${WRKDIR}/go
 WRKSRC =   ${WRKDIST}/src
+
+INSTALL_STRIP =
 
 GOOS = openbsd
 GOARCH =   unknown
Index: lang/go/go.port.mk
===
RCS file: /cvs/ports/lang/go/go.port.mk,v
retrieving revision 1.6
diff -u -p -r1.6 go.port.mk
--- lang/go/go.port.mk  28 Feb 2016 13:24:16 -  1.6
+++ lang/go/go.port.mk  4 May 2016 21:47:45 -
@@ -53,6 +53,10 @@ MODGO_INSTALL_TARGET =   ${INSTALL_DATA_DI
${MODGO_PACKAGE_PATH};
 .endif
 
+# Although this module does not use INSTALL_PROGRAM, unset INSTALL_STRIP to
+# prevent stripping go programs in ports with custom do-install targets.
+INSTALL_STRIP =
+
 MODGO_TEST_TARGET =${MODGO_TEST_CMD} ${TEST_TARGET}
 
 .if empty(CONFIGURE_STYLE)
Index: lang/go-bootstrap/Makefile
===
RCS file: /cvs/ports/lang/go-bootstrap/Makefile,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 Makefile
--- lang/go-bootstrap/Makefile  4 Dec 2015 17:19:18 -   1.1.1.1
+++ lang/go-bootstrap/Makefile  4 May 2016 21:48:19 -
@@ -8,6 +8,7 @@ VERSION =   1.4.3
 EXTRACT_SUFX = .src.tar.gz
 DISTNAME = go${VERSION}
 PKGNAME =  go-bootstrap-${VERSION}
+REVISION = 0
 CATEGORIES =   lang
 
 HOMEPAGE = https://golang.org/
@@ -31,6 +32,8 @@ SUBST_VARS =  GOEXE GOCFG
 
 WRKDIST =  ${WRKDIR}/go
 WRKSRC =   ${WRKDIST}/src
+
+INSTALL_STRIP =
 
 GOOS = openbsd
 GOARCH =   unknown
Index: net/syncthing/Makefile
===
RCS file: /cvs/ports/net/syncthing/Makefile,v
retrieving revision 1.2
diff -u -p -r1.2 Makefile
--- net/syncthing/Makefile  23 Apr 2016 08:02:05 -  1.2
+++ net/syncthing/Makefile  4 May 2016 21:51:13 -
@@ -34,8 +34,7 @@ do-test:
cd ${WRKSRC} && ${MODGO_CMD} run build.go test
 
 do-install:
-   # Note: Don't use INSTALL_PROGRAM. It strips, and go hates this.
-   ${INSTALL_SCRIPT} ${WRKSRC}/bin/syncthing ${PREFIX}/bin/
+   ${INSTALL_PROGRAM} ${WRKSRC}/bin/syncthing ${PREFIX}/bin/
 .for sec in 1 5 7
${INSTALL_MAN} ${WRKSRC}/man/*.${sec} ${PREFIX}/man/man${sec}/
 .endfor
Index: sysutils/terraform/Makefile
===
RCS file: /cvs/ports/sysutils/terraform/Makefile,v
retrieving revision 1.4
diff -u -p -r1.4 Makefile
--- sysutils/terraform/Makefile 3 May 2016 16:16:47 -   1.4
+++ sysutils/terraform/Makefile 4 May 2016 21:53:15 -
@@ -35,9 +35,6 @@ PROVIDERS=atlas aws azure azurerm chef
 
 PROVISIONERS=  chef file local-exec remote-exec
 
-# prevent stripping go binaries
-INSTALL_STRIP=
-
 post-build:
 .for provider in ${PROVIDERS}
cd ${WRKSRC} && \

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Stuart Henderson]

On 2016/04/21 23:10, Dmitrij D. Czarkoff wrote:
> Stuart Henderson said:
> > On 2016/04/21 19:35, Christian Weisgerber wrote:
> >> Dmitrij D. Czarkoff:
> >> 
>  I think you should just set
>  INSTALL_STRIP =
>  in the ports (or modules) that require it and be done with it.
>  No point in introducing yet another variable.
> >>> 
> >>> INSTALL_STRIP is mentioned in mk.conf(5), so it is a user setting.
> >>> /etc/mk.conf may contain INSTALL_STRIP=-s, and that would override
> >>> INSTALL_STRIP= from port.
> >> 
> >> In practice, INSTALL_STRIP has two possible values: "-s" and "".
> >> "-s" is already the default, so the only user setting that makes
> >> any sense is "".  Having some ports already set it to "" is just
> >> fine.
> >> 
> >> A few ports already do this.
> > 
> > Makes sense to me. go-bootstrap needs it too.
> 
> Well, then asking for OKs for the following patch.

Apart from logstash/forwarder these will be changing the packages, so
need bumps. Otherwise OK with me.

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Dmitrij D. Czarkoff]

Stuart Henderson said:
> On 2016/04/21 23:19, Dmitrij D. Czarkoff wrote:
> > 
> > While at it, maybe we should move description of INSTALL_STRIP from
> > mk.conf(5) to bsd.port.mk(5), as apparently it is now used this way?
> 
> Should stay in mk.conf(5), it affects src too.

To my understanding, in src it is only useful with DEBUG, which unsets
it automatically.

-- 
Dmitrij D. Czarkoff

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Jeremie Courreges-Anglas]

"Dmitrij D. Czarkoff"  writes:

> Dmitrij D. Czarkoff said:
>> Stuart Henderson said:
>> > On 2016/04/21 19:35, Christian Weisgerber wrote:
>> >> Dmitrij D. Czarkoff:
>> >> 
>>  I think you should just set
>>  INSTALL_STRIP =
>>  in the ports (or modules) that require it and be done with it.
>>  No point in introducing yet another variable.
>> >>> 
>> >>> INSTALL_STRIP is mentioned in mk.conf(5), so it is a user setting.
>> >>> /etc/mk.conf may contain INSTALL_STRIP=-s, and that would override
>> >>> INSTALL_STRIP= from port.
>> >> 
>> >> In practice, INSTALL_STRIP has two possible values: "-s" and "".
>> >> "-s" is already the default, so the only user setting that makes
>> >> any sense is "".  Having some ports already set it to "" is just
>> >> fine.
>> >> 
>> >> A few ports already do this.
>> > 
>> > Makes sense to me. go-bootstrap needs it too.
>> 
>> Well, then asking for OKs for the following patch.
>
> While at it, maybe we should move description of INSTALL_STRIP from
> mk.conf(5) to bsd.port.mk(5), as apparently it is now used this way?

I don't think so, INSTALL_STRIP comes from bsd.own.mk.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Stuart Henderson]

On 2016/04/21 23:19, Dmitrij D. Czarkoff wrote:
> 
> While at it, maybe we should move description of INSTALL_STRIP from
> mk.conf(5) to bsd.port.mk(5), as apparently it is now used this way?

Should stay in mk.conf(5), it affects src too.

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Dmitrij D. Czarkoff]

Dmitrij D. Czarkoff said:
> Stuart Henderson said:
> > On 2016/04/21 19:35, Christian Weisgerber wrote:
> >> Dmitrij D. Czarkoff:
> >> 
>  I think you should just set
>  INSTALL_STRIP =
>  in the ports (or modules) that require it and be done with it.
>  No point in introducing yet another variable.
> >>> 
> >>> INSTALL_STRIP is mentioned in mk.conf(5), so it is a user setting.
> >>> /etc/mk.conf may contain INSTALL_STRIP=-s, and that would override
> >>> INSTALL_STRIP= from port.
> >> 
> >> In practice, INSTALL_STRIP has two possible values: "-s" and "".
> >> "-s" is already the default, so the only user setting that makes
> >> any sense is "".  Having some ports already set it to "" is just
> >> fine.
> >> 
> >> A few ports already do this.
> > 
> > Makes sense to me. go-bootstrap needs it too.
> 
> Well, then asking for OKs for the following patch.

While at it, maybe we should move description of INSTALL_STRIP from
mk.conf(5) to bsd.port.mk(5), as apparently it is now used this way?

-- 
Dmitrij D. Czarkoff

Index: bsd.port.mk.5
===
RCS file: /cvs/src/share/man/man5/bsd.port.mk.5,v
retrieving revision 1.434
diff -u -p -r1.434 bsd.port.mk.5
--- bsd.port.mk.5   22 Mar 2016 16:02:02 -  1.434
+++ bsd.port.mk.5   21 Apr 2016 21:16:27 -
@@ -1743,6 +1743,14 @@ User settings.
 If set to
 .Sq Yes ,
 do not print anything when ignoring a port.
+.It Ev INSTALL_STRIP
+Flag for
+.Ev INSTALL_PROGRAM
+macro for stripping binaries.
+Default is
+.Sq -s .
+Set to empty value to disable stripping.
+.Sq
 .It Ev INSTALL_{PROGRAM,SCRIPT,DATA,MAN}[_DIR]
 Macros to use to install a program, a script, data, or a man page (or the
 corresponding directory), respectively.
Index: mk.conf.5
===
RCS file: /cvs/src/share/man/man5/mk.conf.5,v
retrieving revision 1.31
diff -u -p -r1.31 mk.conf.5
--- mk.conf.5   31 Mar 2016 15:53:25 -  1.31
+++ mk.conf.5   21 Apr 2016 21:13:55 -
@@ -64,7 +64,7 @@ The following variables are set by
 .In bsd.own.mk ,
 if they are not already defined.
 Defaults are in brackets.
-.Bl -tag -width INSTALL_STRIP
+.Bl -tag -width INSTALL_COPY
 .It Ev BINGRP
 Binary group.
 .Bq bin
@@ -110,16 +110,6 @@ This is to be used when building an inst
 system can either be installed with copies, or copy-if-different using a
 single knob.
 .Bq Fl c
-.It Ev INSTALL_STRIP
-The flag passed to the install program to cause the binary to be stripped.
-This is to be used when building an install script so that the entire
-system can be made stripped/not-stripped using a single knob.
-Note that
-.Ev INSTALL_STRIP
-is not set if
-.Ev ${DEBUG}
-is defined.
-.Bq Fl s
 .It Ev LIBDIR
 Base path for library installation.
 .Bq Pa /usr/lib

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Dmitrij D. Czarkoff]

Stuart Henderson said:
> On 2016/04/21 19:35, Christian Weisgerber wrote:
>> Dmitrij D. Czarkoff:
>> 
 I think you should just set
 INSTALL_STRIP =
 in the ports (or modules) that require it and be done with it.
 No point in introducing yet another variable.
>>> 
>>> INSTALL_STRIP is mentioned in mk.conf(5), so it is a user setting.
>>> /etc/mk.conf may contain INSTALL_STRIP=-s, and that would override
>>> INSTALL_STRIP= from port.
>> 
>> In practice, INSTALL_STRIP has two possible values: "-s" and "".
>> "-s" is already the default, so the only user setting that makes
>> any sense is "".  Having some ports already set it to "" is just
>> fine.
>> 
>> A few ports already do this.
> 
> Makes sense to me. go-bootstrap needs it too.

Well, then asking for OKs for the following patch.

-- 
Dmitrij D. Czarkoff

Index: lang/go/Makefile
===
RCS file: /cvs/ports/lang/go/Makefile,v
retrieving revision 1.29
diff -u -p -r1.29 Makefile
--- lang/go/Makefile14 Apr 2016 17:43:26 -  1.29
+++ lang/go/Makefile21 Apr 2016 20:52:41 -
@@ -30,6 +30,8 @@ SUBST_VARS =  GOCFG
 WRKDIST =  ${WRKDIR}/go
 WRKSRC =   ${WRKDIST}/src
 
+INSTALL_STRIP =
+
 GOOS = openbsd
 GOARCH =   unknown
 GOROOT =   ${PREFIX}/go
Index: lang/go/go.port.mk
===
RCS file: /cvs/ports/lang/go/go.port.mk,v
retrieving revision 1.6
diff -u -p -r1.6 go.port.mk
--- lang/go/go.port.mk  28 Feb 2016 13:24:16 -  1.6
+++ lang/go/go.port.mk  21 Apr 2016 21:07:40 -
@@ -53,6 +53,10 @@ MODGO_INSTALL_TARGET =   ${INSTALL_DATA_DI
${MODGO_PACKAGE_PATH};
 .endif
 
+# Although this module does not use INSTALL_PROGRAM, unset INSTALL_STRIP to
+# prevent stripping go programs in ports with custom do-install targets.
+INSTALL_STRIP =
+
 MODGO_TEST_TARGET =${MODGO_TEST_CMD} ${TEST_TARGET}
 
 .if empty(CONFIGURE_STYLE)
Index: lang/go-bootstrap/Makefile
===
RCS file: /cvs/ports/lang/go-bootstrap/Makefile,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 Makefile
--- lang/go-bootstrap/Makefile  4 Dec 2015 17:19:18 -   1.1.1.1
+++ lang/go-bootstrap/Makefile  21 Apr 2016 20:52:56 -
@@ -32,6 +32,8 @@ SUBST_VARS =  GOEXE GOCFG
 WRKDIST =  ${WRKDIR}/go
 WRKSRC =   ${WRKDIST}/src
 
+INSTALL_STRIP =
+
 GOOS = openbsd
 GOARCH =   unknown
 GOROOT =   ${PREFIX}/go/bootstrap
Index: sysutils/logstash/forwarder//Makefile
===
RCS file: /cvs/ports/sysutils/logstash/forwarder/Makefile,v
retrieving revision 1.8
diff -u -p -r1.8 Makefile
--- sysutils/logstash/forwarder//Makefile   21 Apr 2016 00:07:08 -  
1.8
+++ sysutils/logstash/forwarder//Makefile   21 Apr 2016 21:05:45 -
@@ -16,9 +16,8 @@ WANTLIB +=c pthread
 pre-configure:
${SUBST_CMD} ${WRKSRC}/logstash-forwarder.conf.example
 
-# Use INSTALL_SCRIPT to prevent stripping go binaries
 do-install:
-   ${INSTALL_SCRIPT} ${MODGO_WORKSPACE}/bin/logstash-forwarder \
+   ${INSTALL_PROGRAM} ${MODGO_WORKSPACE}/bin/logstash-forwarder \
${PREFIX}/sbin/
${INSTALL_DATA_DIR} ${PREFIX}/share/{doc,examples}/logstash-forwarder/
${INSTALL_DATA} ${WRKSRC}/README.md \

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Christian Weisgerber]

On 2016-04-21, Raul Miller  wrote:

[INSTALL_STRIP=""]
>> A few ports already do this.
>
>  Do those ports crash with INSTALL_STRIP=-s ?

astro/xworld attaches its data as an ELF segment.  I don't know if
it will crash, but it certainly won't work if you strip it.

emulators/gxemul, I have no idea.

emulators/qemu uses it in a debug flavor.

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Stuart Henderson]

On 2016/04/21 19:35, Christian Weisgerber wrote:
> Dmitrij D. Czarkoff:
> 
> > > I think you should just set
> > > INSTALL_STRIP =
> > > in the ports (or modules) that require it and be done with it.
> > > No point in introducing yet another variable.
> > 
> > INSTALL_STRIP is mentioned in mk.conf(5), so it is a user setting.
> > /etc/mk.conf may contain INSTALL_STRIP=-s, and that would override
> > INSTALL_STRIP= from port.
> 
> In practice, INSTALL_STRIP has two possible values: "-s" and "".
> "-s" is already the default, so the only user setting that makes
> any sense is "".  Having some ports already set it to "" is just
> fine.
> 
> A few ports already do this.
> 
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 

Makes sense to me. go-bootstrap needs it too.

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Raul Miller]

On Thu, Apr 21, 2016 at 1:35 PM, Christian Weisgerber
 wrote:
> A few ports already do this.

 Do those ports crash with INSTALL_STRIP=-s ?

Thanks,

-- 
Raul

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Christian Weisgerber]

Dmitrij D. Czarkoff:

> > I think you should just set
> > INSTALL_STRIP =
> > in the ports (or modules) that require it and be done with it.
> > No point in introducing yet another variable.
> 
> INSTALL_STRIP is mentioned in mk.conf(5), so it is a user setting.
> /etc/mk.conf may contain INSTALL_STRIP=-s, and that would override
> INSTALL_STRIP= from port.

In practice, INSTALL_STRIP has two possible values: "-s" and "".
"-s" is already the default, so the only user setting that makes
any sense is "".  Having some ports already set it to "" is just
fine.

A few ports already do this.

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Dmitrij D. Czarkoff]

Christian Weisgerber said:
> Dmitrij D. Czarkoff:
> 
> > --- infrastructure/mk/bsd.port.mk   20 Mar 2016 20:07:20 -  1.1310
> > +++ infrastructure/mk/bsd.port.mk   21 Apr 2016 13:10:29 -
> > @@ -729,6 +729,12 @@ UNZIP ?= unzip
> >  BZIP2 ?= bzip2
> >  
> >  
> > +# disable stripping if requested
> > +INSTALL_NO_STRIP ?=
> > +.if ${INSTALL_NO_STRIP:L} == "yes"
> > +INSTALL_STRIP =
> > +.endif
> > +
> 
> I think you should just set
> 
> INSTALL_STRIP =
> 
> in the ports (or modules) that require it and be done with it.
> No point in introducing yet another variable.

INSTALL_STRIP is mentioned in mk.conf(5), so it is a user setting.
/etc/mk.conf may contain INSTALL_STRIP=-s, and that would override
INSTALL_STRIP= from port.

-- 
Dmitrij D. Czarkoff

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Christian Weisgerber]

Dmitrij D. Czarkoff:

> --- infrastructure/mk/bsd.port.mk 20 Mar 2016 20:07:20 -  1.1310
> +++ infrastructure/mk/bsd.port.mk 21 Apr 2016 13:10:29 -
> @@ -729,6 +729,12 @@ UNZIP ?= unzip
>  BZIP2 ?= bzip2
>  
>  
> +# disable stripping if requested
> +INSTALL_NO_STRIP ?=
> +.if ${INSTALL_NO_STRIP:L} == "yes"
> +INSTALL_STRIP =
> +.endif
> +

I think you should just set

INSTALL_STRIP =

in the ports (or modules) that require it and be done with it.
No point in introducing yet another variable.

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Dmitrij D. Czarkoff]

Adam Wolk said:
> On Thu, 21 Apr 2016 15:12:24 +0200
> "Dmitrij D. Czarkoff"  wrote:
> 
>> Dmitrij D. Czarkoff said:
>>> Edd Barrett said:  
 On Thu, Apr 21, 2016 at 01:38:54AM +0200, Adam Wolk wrote:  
> +# Use INSTALL_SCRIPT to prevent stripping go binaries  
 
 Is it worth adding a INSTALL_GO_PROGRAM in the lang/go MODULE to
 avoid confusion?  
>>> 
>>> I'd rather go with more generic change.  Patch below adds
>>> INSTALL_NO_STRIP macro to bsd.port.mk.  Setting this macro to "Yes"
>>> makes INSTALL_STRIP empty.  go.port.mk sets INSTALL_NO_STRIP to
>>> "Yes".
>>> 
>>> Diff against bsd.port.mk.5 in src also included.  
>> 
>> Forgot to define INSTALL_NO_STRIP.
>> 
> 
> OK awolk@ on condition that INSTALL_NO_STRIP = Yes is added to
> lang/go/Makefile. The go port itself doesn't use it's module but
> installs go binaries and has the same warnings. Looks ok with your diff
> plus  the additional INSTALL_NO_STRIP.
> 
> Reverting logstash/forwarder change from yesterday will also be
> required. I can do that if you commit the go port changes.

I could do it all in one go.  But I'd rather have more OKs for diff that
changes bsd.port.mk.

(Diff below is updated with lang/go/Makefile chunk.)

-- 
Dmitrij D. Czarkoff

Index: infrastructure/mk/bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1310
diff -u -p -r1.1310 bsd.port.mk
--- infrastructure/mk/bsd.port.mk   20 Mar 2016 20:07:20 -  1.1310
+++ infrastructure/mk/bsd.port.mk   21 Apr 2016 13:10:29 -
@@ -729,6 +729,12 @@ UNZIP ?= unzip
 BZIP2 ?= bzip2
 
 
+# disable stripping if requested
+INSTALL_NO_STRIP ?=
+.if ${INSTALL_NO_STRIP:L} == "yes"
+INSTALL_STRIP =
+.endif
+
 # copy selected info from bsd.own.mk
 MAKE_ENV += COMPILER_VERSION=${COMPILER_VERSION} \
PICFLAG="${PICFLAG}" ASPICFLAG=${ASPICFLAG} \
Index: Makefile
===
RCS file: /cvs/ports/lang/go/Makefile,v
retrieving revision 1.29
diff -u -p -r1.29 Makefile
--- Makefile14 Apr 2016 17:43:26 -  1.29
+++ Makefile21 Apr 2016 14:27:24 -
@@ -30,6 +30,8 @@ SUBST_VARS =  GOCFG
 WRKDIST =  ${WRKDIR}/go
 WRKSRC =   ${WRKDIST}/src
 
+INSTALL_NO_STRIP = Yes
+
 GOOS = openbsd
 GOARCH =   unknown
 GOROOT =   ${PREFIX}/go
Index: lang/go/go.port.mk
===
RCS file: /cvs/ports/lang/go/go.port.mk,v
retrieving revision 1.6
diff -u -p -r1.6 go.port.mk
--- lang/go/go.port.mk  28 Feb 2016 13:24:16 -  1.6
+++ lang/go/go.port.mk  21 Apr 2016 13:03:21 -
@@ -2,6 +2,8 @@
 
 ONLY_FOR_ARCHS ?=  ${GO_ARCHS}
 
+INSTALL_NO_STRIP = Yes
+
 MODGO_BUILDDEP ?=  Yes
 
 MODGO_RUN_DEPENDS =lang/go
Index: share/man/man5/bsd.port.mk.5
===
RCS file: /cvs/src/share/man/man5/bsd.port.mk.5,v
retrieving revision 1.434
diff -u -p -r1.434 bsd.port.mk.5
--- share/man/man5/bsd.port.mk.522 Mar 2016 16:02:02 -  1.434
+++ share/man/man5/bsd.port.mk.521 Apr 2016 13:02:45 -
@@ -1743,6 +1743,11 @@ User settings.
 If set to
 .Sq Yes ,
 do not print anything when ignoring a port.
+.It Ev INSTALL_NO_STRIP
+If set to
+.Sq Yes ,
+.Ev INSTALL_PROGRAM
+macro does not strip binaries.
 .It Ev INSTALL_{PROGRAM,SCRIPT,DATA,MAN}[_DIR]
 Macros to use to install a program, a script, data, or a man page (or the
 corresponding directory), respectively.

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Adam Wolk]

On Thu, 21 Apr 2016 15:12:24 +0200
"Dmitrij D. Czarkoff"  wrote:

> Dmitrij D. Czarkoff said:
> > Edd Barrett said:  
> > > On Thu, Apr 21, 2016 at 01:38:54AM +0200, Adam Wolk wrote:  
> > > > +# Use INSTALL_SCRIPT to prevent stripping go binaries  
> > > 
> > > Is it worth adding a INSTALL_GO_PROGRAM in the lang/go MODULE to
> > > avoid confusion?  
> > 
> > I'd rather go with more generic change.  Patch below adds
> > INSTALL_NO_STRIP macro to bsd.port.mk.  Setting this macro to "Yes"
> > makes INSTALL_STRIP empty.  go.port.mk sets INSTALL_NO_STRIP to
> > "Yes".
> > 
> > Diff against bsd.port.mk.5 in src also included.  
> 
> Forgot to define INSTALL_NO_STRIP.
> 

OK awolk@ on condition that INSTALL_NO_STRIP = Yes is added to
lang/go/Makefile. The go port itself doesn't use it's module but
installs go binaries and has the same warnings. Looks ok with your diff
plus  the additional INSTALL_NO_STRIP.

Reverting logstash/forwarder change from yesterday will also be
required. I can do that if you commit the go port changes.

Patch to revert logstash:
Index: Makefile
===
RCS file: /cvs/ports/sysutils/logstash/forwarder/Makefile,v
retrieving revision 1.8
diff -u -p -r1.8 Makefile
--- Makefile21 Apr 2016 00:07:08 -  1.8
+++ Makefile21 Apr 2016 14:11:54 -
@@ -5,7 +5,7 @@ COMMENT=collect logs locally in prepara
 GH_ACCOUNT=elastic
 GH_PROJECT=logstash-forwarder
 GH_TAGNAME=v0.4.0
-REVISION=  2
+REVISION=  3
 
 MODULES=   lang/go
 
@@ -16,9 +16,8 @@ WANTLIB +=c pthread
 pre-configure:
${SUBST_CMD} ${WRKSRC}/logstash-forwarder.conf.example
 
-# Use INSTALL_SCRIPT to prevent stripping go binaries
 do-install:
-   ${INSTALL_SCRIPT} ${MODGO_WORKSPACE}/bin/logstash-forwarder \
+   ${INSTALL_PROGRAM} ${MODGO_WORKSPACE}/bin/logstash-forwarder \
${PREFIX}/sbin/
${INSTALL_DATA_DIR} ${PREFIX}/share/{doc,examples}/logstash-forwarder/
${INSTALL_DATA} ${WRKSRC}/README.md \

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Dmitrij D. Czarkoff]

Dmitrij D. Czarkoff said:
> Edd Barrett said:
> > On Thu, Apr 21, 2016 at 01:38:54AM +0200, Adam Wolk wrote:
> > > +# Use INSTALL_SCRIPT to prevent stripping go binaries
> > 
> > Is it worth adding a INSTALL_GO_PROGRAM in the lang/go MODULE to avoid
> > confusion?
> 
> I'd rather go with more generic change.  Patch below adds
> INSTALL_NO_STRIP macro to bsd.port.mk.  Setting this macro to "Yes"
> makes INSTALL_STRIP empty.  go.port.mk sets INSTALL_NO_STRIP to "Yes".
> 
> Diff against bsd.port.mk.5 in src also included.

Forgot to define INSTALL_NO_STRIP.

-- 
Dmitrij D. Czarkoff

Index: infrastructure/mk/bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1310
diff -u -p -r1.1310 bsd.port.mk
--- infrastructure/mk/bsd.port.mk   20 Mar 2016 20:07:20 -  1.1310
+++ infrastructure/mk/bsd.port.mk   21 Apr 2016 13:10:29 -
@@ -729,6 +729,12 @@ UNZIP ?= unzip
 BZIP2 ?= bzip2
 
 
+# disable stripping if requested
+INSTALL_NO_STRIP ?=
+.if ${INSTALL_NO_STRIP:L} == "yes"
+INSTALL_STRIP =
+.endif
+
 # copy selected info from bsd.own.mk
 MAKE_ENV += COMPILER_VERSION=${COMPILER_VERSION} \
PICFLAG="${PICFLAG}" ASPICFLAG=${ASPICFLAG} \
Index: lang/go/go.port.mk
===
RCS file: /cvs/ports/lang/go/go.port.mk,v
retrieving revision 1.6
diff -u -p -r1.6 go.port.mk
--- lang/go/go.port.mk  28 Feb 2016 13:24:16 -  1.6
+++ lang/go/go.port.mk  21 Apr 2016 13:03:21 -
@@ -2,6 +2,8 @@
 
 ONLY_FOR_ARCHS ?=  ${GO_ARCHS}
 
+INSTALL_NO_STRIP = Yes
+
 MODGO_BUILDDEP ?=  Yes
 
 MODGO_RUN_DEPENDS =lang/go
Index: share/man/man5/bsd.port.mk.5
===
RCS file: /cvs/src/share/man/man5/bsd.port.mk.5,v
retrieving revision 1.434
diff -u -p -r1.434 bsd.port.mk.5
--- share/man/man5/bsd.port.mk.522 Mar 2016 16:02:02 -  1.434
+++ share/man/man5/bsd.port.mk.521 Apr 2016 13:02:45 -
@@ -1743,6 +1743,11 @@ User settings.
 If set to
 .Sq Yes ,
 do not print anything when ignoring a port.
+.It Ev INSTALL_NO_STRIP
+If set to
+.Sq Yes ,
+.Ev INSTALL_PROGRAM
+macro does not strip binaries.
 .It Ev INSTALL_{PROGRAM,SCRIPT,DATA,MAN}[_DIR]
 Macros to use to install a program, a script, data, or a man page (or the
 corresponding directory), respectively.

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Dmitrij D. Czarkoff]

Edd Barrett said:
> On Thu, Apr 21, 2016 at 01:38:54AM +0200, Adam Wolk wrote:
> > +# Use INSTALL_SCRIPT to prevent stripping go binaries
> 
> Is it worth adding a INSTALL_GO_PROGRAM in the lang/go MODULE to avoid
> confusion?

I'd rather go with more generic change.  Patch below adds
INSTALL_NO_STRIP macro to bsd.port.mk.  Setting this macro to "Yes"
makes INSTALL_STRIP empty.  go.port.mk sets INSTALL_NO_STRIP to "Yes".

Diff against bsd.port.mk.5 in src also included.

-- 
Dmitrij D. Czarkoff

Index: infrastructure/mk/bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1310
diff -u -p -r1.1310 bsd.port.mk
--- infrastructure/mk/bsd.port.mk   20 Mar 2016 20:07:20 -  1.1310
+++ infrastructure/mk/bsd.port.mk   21 Apr 2016 12:57:11 -
@@ -729,6 +729,11 @@ UNZIP ?= unzip
 BZIP2 ?= bzip2
 
 
+# disable stripping if requested
+.if ${INSTALL_NO_STRIP:L} == "yes"
+INSTALL_STRIP =
+.endif
+
 # copy selected info from bsd.own.mk
 MAKE_ENV += COMPILER_VERSION=${COMPILER_VERSION} \
PICFLAG="${PICFLAG}" ASPICFLAG=${ASPICFLAG} \
Index: lang/go/go.port.mk
===
RCS file: /cvs/ports/lang/go/go.port.mk,v
retrieving revision 1.6
diff -u -p -r1.6 go.port.mk
--- lang/go/go.port.mk  28 Feb 2016 13:24:16 -  1.6
+++ lang/go/go.port.mk  21 Apr 2016 13:03:21 -
@@ -2,6 +2,8 @@
 
 ONLY_FOR_ARCHS ?=  ${GO_ARCHS}
 
+INSTALL_NO_STRIP = Yes
+
 MODGO_BUILDDEP ?=  Yes
 
 MODGO_RUN_DEPENDS =lang/go
Index: share/man/man5/bsd.port.mk.5
===
RCS file: /cvs/src/share/man/man5/bsd.port.mk.5,v
retrieving revision 1.434
diff -u -p -r1.434 bsd.port.mk.5
--- share/man/man5/bsd.port.mk.522 Mar 2016 16:02:02 -  1.434
+++ share/man/man5/bsd.port.mk.521 Apr 2016 13:02:45 -
@@ -1743,6 +1743,11 @@ User settings.
 If set to
 .Sq Yes ,
 do not print anything when ignoring a port.
+.It Ev INSTALL_NO_STRIP
+If set to
+.Sq Yes ,
+.Ev INSTALL_PROGRAM
+macro does not strip binaries.
 .It Ev INSTALL_{PROGRAM,SCRIPT,DATA,MAN}[_DIR]
 Macros to use to install a program, a script, data, or a man page (or the
 corresponding directory), respectively.

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Adam Wolk]

On Thu, 21 Apr 2016 12:35:41 +0100
Edd Barrett  wrote:

> On Thu, Apr 21, 2016 at 01:38:54AM +0200, Adam Wolk wrote:
> > +# Use INSTALL_SCRIPT to prevent stripping go binaries  
> 
> Is it worth adding a INSTALL_GO_PROGRAM in the lang/go MODULE to avoid
> confusion?
> 
> I note that I saw such weird errors when building lang/go itself, so
> perhaps we should be applying similar changes there?
> 

I sent a patch for lang/go yesterday for verification to jsing@ & sthen@

Regards,
Adam

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Edd Barrett]

On Thu, Apr 21, 2016 at 01:38:54AM +0200, Adam Wolk wrote:
> +# Use INSTALL_SCRIPT to prevent stripping go binaries

Is it worth adding a INSTALL_GO_PROGRAM in the lang/go MODULE to avoid
confusion?

I note that I saw such weird errors when building lang/go itself, so
perhaps we should be applying similar changes there?

-- 
Best Regards
Edd Barrett

http://www.theunixzoo.co.uk

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Jasper Lievisse Adriaanse]

On Thu, Apr 21, 2016 at 01:38:54AM +0200, Adam Wolk wrote:
> Hi,
> 
> While testing a port for net/syncthing sthen@ noticed the following
> warnings:
> 
> 00:44 < sthen> hm, there are some funny messages from that port in
> 'make fake' 00:45 < sthen>
> BFD: /usr/obj/ports/syncthing-0.12.22/fake-amd64/usr/local/bin//stVnev2l:
> warning: allocated section `.gosymtab' not in segment 00:45 < sthen>
> BFD: /usr/obj/ports/syncthing-0.12.22/fake-amd64/usr/local/bin//stVnev2l:
> warning: allocated section `.gnu.version_r' not in segment
> 
> the cause for those errors is trying to strip a Go binary which doesn't
> output all expected segments. Upstream officially doesn't support
> stripping and it might lead to undefined behavior.
> 
> Debian Bug report logs - #717172
> Stripping golang binaries causes crashes:
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=717172
> 
> go get ... fails with SIGILL on armhf
> https://bugs.launchpad.net/ubuntu/+source/golang/+bug/1200255
> 
> docker: Stripping Binaries
> https://github.com/docker/docker/blob/2a95488f7843a773de2b541a47d9b971a635bfff/project/PACKAGERS.md#stripping-binaries
> 
> relevant blog post:
> https://blog.filippo.io/shrink-your-go-binaries-with-this-one-weird-trick/
> 
> I went through existing go ports to see how many of them actually strip
> the binaries.
> 
> https://gist.github.com/mulander/a0dd5d5b49ca81001628d690b70ba111
> 
> Apparently from the existing ports only sysutils/logstash/forwarder is
> affected.
> This issue was initially reported by Fritjof Bornebusch (thanks!) in
> July last year: http://marc.info/?l=openbsd-ports=143766040011892=2
> 
> I'm inlining a patch for sysutils/logstash/forwarder replacing the
> usage of INSTALL_PROGRAM with INSTALL_SCRIPT and a comment informing
> why this choice is made. Any OK's?
OK with me; go binaries can still be stripped but that's a compile/link option
and not something strip(1) should get involved with.

> Index: Makefile
> ===
> RCS file: /cvs/ports/sysutils/logstash/forwarder/Makefile,v
> retrieving revision 1.7
> diff -u -p -r1.7 Makefile
> --- Makefile  6 Jan 2016 20:02:19 -   1.7
> +++ Makefile  20 Apr 2016 23:37:17 -
> @@ -5,7 +5,7 @@ COMMENT=  collect logs locally in prepara
>  GH_ACCOUNT=  elastic
>  GH_PROJECT=  logstash-forwarder
>  GH_TAGNAME=  v0.4.0
> -REVISION=1
> +REVISION=2
>  
>  MODULES= lang/go
>  
> @@ -16,8 +16,9 @@ WANTLIB +=  c pthread
>  pre-configure:
>   ${SUBST_CMD} ${WRKSRC}/logstash-forwarder.conf.example
>  
> +# Use INSTALL_SCRIPT to prevent stripping go binaries
>  do-install:
> - ${INSTALL_PROGRAM} ${MODGO_WORKSPACE}/bin/logstash-forwarder \
> + ${INSTALL_SCRIPT} ${MODGO_WORKSPACE}/bin/logstash-forwarder \
>   ${PREFIX}/sbin/
>   ${INSTALL_DATA_DIR} ${PREFIX}/share/{doc,examples}/logstash-forwarder/
>   ${INSTALL_DATA} ${WRKSRC}/README.md \
> 

-- 
jasper

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Adam Wolk]

On Thu, 21 Apr 2016 01:54:38 +0200
"Dmitrij D. Czarkoff"  wrote:

> Adam Wolk said:
> > I'm inlining a patch for sysutils/logstash/forwarder replacing the
> > usage of INSTALL_PROGRAM with INSTALL_SCRIPT and a comment informing
> > why this choice is made. Any OK's?  
> 
> OK czarkoff@.  A cleaner approach would be to set
> 

Thanks. Committed.

> | INSTALL_STRIP=
> 
> but that is a user setting which may be overriden in mk.conf.
> 

I agree but if it's known to produce crashing binaries and is
officially not supported by upstream then giving people a knob to turn
off binary stripping is not what we should do?

Regards,
Adam

Re: sysutils/logstash/forwarder should not strip lang/go binaries

[Dmitrij D. Czarkoff]

Adam Wolk said:
> I'm inlining a patch for sysutils/logstash/forwarder replacing the
> usage of INSTALL_PROGRAM with INSTALL_SCRIPT and a comment informing
> why this choice is made. Any OK's?

OK czarkoff@.  A cleaner approach would be to set

| INSTALL_STRIP=

but that is a user setting which may be overriden in mk.conf.

-- 
Dmitrij D. Czarkoff