[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Cam Hutchison]

Allow the CGO_ENABLED variable to be controlled per package and per
build target, instead of having the value determined by whether or not
the toolchain has threads.

Some build targets may not build with CGO_ENABLED=1, so allowing a per
package and build target override will allow those targets to be built
with Buildroot.

Signed-off-by: Cam Hutchison <camh@xdna.net>
---
 docs/manual/adding-packages-golang.txt | 14 ++++++++++++++
 package/pkg-golang.mk                  | 17 +++++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/docs/manual/adding-packages-golang.txt b/docs/manual/adding-packages-golang.txt
index efcf696867..2683774861 100644
--- a/docs/manual/adding-packages-golang.txt
+++ b/docs/manual/adding-packages-golang.txt
@@ -99,6 +99,20 @@ therefore only use a few of them, or none.
    +FOO_BUILD_TARGETS = cmd/docker cmd/dockerd+ the binaries produced
    are +docker+ and +dockerd+.
 
+* +FOO_CGO_ENABLED+ overrides the default value for the +CGO_ENABLED+
+  flag passed to the Go compiler. By default +CGO_ENABLED+ is set
+  to 1 if the toolchain supports threads, but you can force it to
+  0 for the package by setting this variable. If the toolchain does
+  not support threads and you set +FOO_CGO_ENABLED+ to 1, an error
+  will be generated.
+
+* +FOO_BUILD_TARGET_CGO_ENABLED_<target>+ can be used to override
+  the +CGO_ENABLED+ flag passed to the Go compiler for a specific
+  build target, similar to +FOO_CGO_ENABLED+. +<target>+ references
+  a target listed in +FOO_BUILD_TARGETS+. If the toolchain does
+  not support threads and you set this variable to 1, an error will
+  be generated.
+
 * +FOO_INSTALL_BINS+ can be used to pass the list of binaries that
   should be installed in +/usr/bin+ on the target. If
   +FOO_INSTALL_BINS+ is not specified, it defaults to the lower-case
diff --git a/package/pkg-golang.mk b/package/pkg-golang.mk
index bf178622b5..2be3bd4c14 100644
--- a/package/pkg-golang.mk
+++ b/package/pkg-golang.mk
@@ -28,8 +28,20 @@ GO_BIN = $(HOST_DIR)/bin/go
 GO_TARGET_ENV = \
 	$(HOST_GO_TARGET_ENV) \
 	PATH=$(BR_PATH) \
-	GOBIN= \
-	CGO_ENABLED=$(HOST_GO_CGO_ENABLED)
+	GOBIN=
+
+# Allow CGO_ENABLED to be defined per package and per build target, but if
+# CGO is enabled for a package or a build target and the toolchain does not
+# support that, generate an error.
+GO_CGO_ENABLED = $(strip \
+	$(if $(filter 10,$($(1)_CGO_ENABLED)$(HOST_GO_CGO_ENABLED)),\
+		$(error Toolchain does not support CGO_ENABLED=1 for package $(1)),\
+		$(or $($(1)_CGO_ENABLED),$(HOST_GO_CGO_ENABLED))))
+
+GO_TARGET_CGO_ENABLED = $(strip \
+	$(if $(filter 10,$($(1)_BUILD_TARGET_CGO_ENABLED_$(2))$(HOST_GO_CGO_ENABLED)),\
+		$(error Toolchain does not support CGO_ENABLED=1 for target $(2)),\
+		$(or $($(1)_BUILD_TARGET_CGO_ENABLED_$(2)),$(call GO_CGO_ENABLED,$(1)))))
 
 ################################################################################
 # inner-golang-package -- defines how the configuration, compilation and
@@ -100,6 +112,7 @@ define $(2)_BUILD_CMDS
 	$$(foreach d,$$($(2)_BUILD_TARGETS),\
 		cd $$($(2)_SRC_PATH); \
 		$$(GO_TARGET_ENV) \
+			CGO_ENABLED=$$(call GO_TARGET_CGO_ENABLED,$(2),$$(d)) \
 			GOPATH="$$(@D)/$$($(2)_WORKSPACE)" \
 			$$($(2)_GO_ENV) \
 			$$(GO_BIN) build -v $$($(2)_BUILD_OPTS) \
-- 
2.11.0

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Thomas Petazzoni]

Hello Cam,

+Christian and Anisse in Cc.

Sorry for the very slow feedback.

On Fri, 27 Jul 2018 12:47:20 +1000
Cam Hutchison <camh@xdna.net> wrote:

> Allow the CGO_ENABLED variable to be controlled per package and per
> build target, instead of having the value determined by whether or not
> the toolchain has threads.
> 
> Some build targets may not build with CGO_ENABLED=1, so allowing a per
> package and build target override will allow those targets to be built
> with Buildroot.

Could you give some specific examples instead of the fuzzy "Some build
targets" wording ?

Christian, Anisse, what do you think about this patch ? Do we need
something like this in our Go support ?

I'm leaving the full patch below. You can also find it at
http://patchwork.ozlabs.org/patch/949972/.

Thanks!

Thomas

> Signed-off-by: Cam Hutchison <camh@xdna.net>
> ---
>  docs/manual/adding-packages-golang.txt | 14 ++++++++++++++
>  package/pkg-golang.mk                  | 17 +++++++++++++++--
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/manual/adding-packages-golang.txt b/docs/manual/adding-packages-golang.txt
> index efcf696867..2683774861 100644
> --- a/docs/manual/adding-packages-golang.txt
> +++ b/docs/manual/adding-packages-golang.txt
> @@ -99,6 +99,20 @@ therefore only use a few of them, or none.
>     +FOO_BUILD_TARGETS = cmd/docker cmd/dockerd+ the binaries produced
>     are +docker+ and +dockerd+.
>  
> +* +FOO_CGO_ENABLED+ overrides the default value for the +CGO_ENABLED+
> +  flag passed to the Go compiler. By default +CGO_ENABLED+ is set
> +  to 1 if the toolchain supports threads, but you can force it to
> +  0 for the package by setting this variable. If the toolchain does
> +  not support threads and you set +FOO_CGO_ENABLED+ to 1, an error
> +  will be generated.
> +
> +* +FOO_BUILD_TARGET_CGO_ENABLED_<target>+ can be used to override
> +  the +CGO_ENABLED+ flag passed to the Go compiler for a specific
> +  build target, similar to +FOO_CGO_ENABLED+. +<target>+ references
> +  a target listed in +FOO_BUILD_TARGETS+. If the toolchain does
> +  not support threads and you set this variable to 1, an error will
> +  be generated.
> +
>  * +FOO_INSTALL_BINS+ can be used to pass the list of binaries that
>    should be installed in +/usr/bin+ on the target. If
>    +FOO_INSTALL_BINS+ is not specified, it defaults to the lower-case
> diff --git a/package/pkg-golang.mk b/package/pkg-golang.mk
> index bf178622b5..2be3bd4c14 100644
> --- a/package/pkg-golang.mk
> +++ b/package/pkg-golang.mk
> @@ -28,8 +28,20 @@ GO_BIN = $(HOST_DIR)/bin/go
>  GO_TARGET_ENV = \
>  	$(HOST_GO_TARGET_ENV) \
>  	PATH=$(BR_PATH) \
> -	GOBIN= \
> -	CGO_ENABLED=$(HOST_GO_CGO_ENABLED)
> +	GOBIN=
> +
> +# Allow CGO_ENABLED to be defined per package and per build target, but if
> +# CGO is enabled for a package or a build target and the toolchain does not
> +# support that, generate an error.
> +GO_CGO_ENABLED = $(strip \
> +	$(if $(filter 10,$($(1)_CGO_ENABLED)$(HOST_GO_CGO_ENABLED)),\
> +		$(error Toolchain does not support CGO_ENABLED=1 for package $(1)),\
> +		$(or $($(1)_CGO_ENABLED),$(HOST_GO_CGO_ENABLED))))
> +
> +GO_TARGET_CGO_ENABLED = $(strip \
> +	$(if $(filter 10,$($(1)_BUILD_TARGET_CGO_ENABLED_$(2))$(HOST_GO_CGO_ENABLED)),\
> +		$(error Toolchain does not support CGO_ENABLED=1 for target $(2)),\
> +		$(or $($(1)_BUILD_TARGET_CGO_ENABLED_$(2)),$(call GO_CGO_ENABLED,$(1)))))
>  
>  ################################################################################
>  # inner-golang-package -- defines how the configuration, compilation and
> @@ -100,6 +112,7 @@ define $(2)_BUILD_CMDS
>  	$$(foreach d,$$($(2)_BUILD_TARGETS),\
>  		cd $$($(2)_SRC_PATH); \
>  		$$(GO_TARGET_ENV) \
> +			CGO_ENABLED=$$(call GO_TARGET_CGO_ENABLED,$(2),$$(d)) \
>  			GOPATH="$$(@D)/$$($(2)_WORKSPACE)" \
>  			$$($(2)_GO_ENV) \
>  			$$(GO_BIN) build -v $$($(2)_BUILD_OPTS) \



-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Anisse Astier]

Hi Cam,

Sorry for the delay,

(+Angelo in cc)

On Fri, Jul 27, 2018 at 12:47:20PM +1000, Cam Hutchison wrote:
> Allow the CGO_ENABLED variable to be controlled per package and per
> build target, instead of having the value determined by whether or not
> the toolchain has threads.
> 
> Some build targets may not build with CGO_ENABLED=1, so allowing a per
> package and build target override will allow those targets to be built
> with Buildroot.

Just like Thomas, I'm wondering what specific use-case you have in mind;
in particular, I'm interested why you needed build-target granularity in
addition to package-level configuration.

To provide context: 

Disabling CGO isn't an issue, until you try doing DNS requests, which
are usually handled by your libc. If you have anything configured out of
the ordinary /etc/resolv.conf, a cgo-disabled binary won't be able to do
a DNS request. Same goes for user/group and user home dir resolution:
anything outside of /etc/{passwd,group} (pam, NSS, etc.), won't work.

Disabling CGO allows you to have static binaries; if you don't use any
native libraries.

I don't know in which context it might be useful in buildroot.

Regards,

Anisse

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Christian Stewart]

Hi Anisse, Cam, all,

Overriding CGO_ENABLED when we know that the toolchain doesn't work
properly with cgo doesn't seem correct.

Anisse Astier <anisse@astier.eu> writes:
>> Some build targets may not build with CGO_ENABLED=1, so allowing a per
>> package and build target override will allow those targets to be built
>> with Buildroot.

Why not just add the override in the existing build flag overrides?

> Just like Thomas, I'm wondering what specific use-case you have in mind;
> in particular, I'm interested why you needed build-target granularity in
> addition to package-level configuration.

If you really need to do something this custom, I recommend just
overriding the build stage entirely.

> Disabling CGO isn't an issue, until you try doing DNS requests, which
> are usually handled by your libc. If you have anything configured out of
> the ordinary /etc/resolv.conf, a cgo-disabled binary won't be able to do
> a DNS request. Same goes for user/group and user home dir resolution:
> anything outside of /etc/{passwd,group} (pam, NSS, etc.), won't work.

Are you referring to the case where a Go-implemented DNS is used? This
should be the case with cgo disabled, and I don't know if the behavior
differs from the regular libc dns.

> I don't know in which context it might be useful in buildroot.

I don't see a reason for it either.

Best,
Christian

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Thomas Petazzoni]

Hello,

On Sun, 07 Apr 2019 19:00:20 -0700
Christian Stewart <christian@paral.in> wrote:

> > I don't know in which context it might be useful in buildroot.  
> 
> I don't see a reason for it either.

Thanks for the feedback, I'll mark the patch as Rejected for now. We
can of course revisit if there's some further details about why this
would be needed.

Anisse, Christian: nobody is listed in the DEVELOPERS file for the
package/pkg-golang.mk file. Would you be willing to be listed as
DEVELOPERS for this file ? It would be useful as you would be Cc'ed on
patches touching this file, which would help in situations like this
one. If you're fine with this, send a patch to add yourself :)

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Anisse Astier]

On Sun, Apr 07, 2019 at 07:00:20PM -0700, Christian Stewart wrote:
> Hi Anisse, Cam, all,
> 
> Overriding CGO_ENABLED when we know that the toolchain doesn't work
> properly with cgo doesn't seem correct.
> 
> Anisse Astier <anisse@astier.eu> writes:
> >> Some build targets may not build with CGO_ENABLED=1, so allowing a per
> >> package and build target override will allow those targets to be built
> >> with Buildroot.
> 
> Why not just add the override in the existing build flag overrides?
> 
> > Just like Thomas, I'm wondering what specific use-case you have in mind;
> > in particular, I'm interested why you needed build-target granularity in
> > addition to package-level configuration.
> 
> If you really need to do something this custom, I recommend just
> overriding the build stage entirely.
> 
> > Disabling CGO isn't an issue, until you try doing DNS requests, which
> > are usually handled by your libc. If you have anything configured out of
> > the ordinary /etc/resolv.conf, a cgo-disabled binary won't be able to do
> > a DNS request. Same goes for user/group and user home dir resolution:
> > anything outside of /etc/{passwd,group} (pam, NSS, etc.), won't work.
> 
> Are you referring to the case where a Go-implemented DNS is used? This
> should be the case with cgo disabled, and I don't know if the behavior
> differs from the regular libc dns.

Yes, I'm talking about the go-implemented dns resolution when cgo is
disabled.

On a glibc system you might not simply rely on /etc/{hosts,resolv.conf}
since hostname resolution is pluggable and configurable with
nsswitch.conf.

Regards,

Anisse

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Anisse Astier]

On Mon, Apr 08, 2019 at 09:04:58AM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> On Sun, 07 Apr 2019 19:00:20 -0700
> Christian Stewart <christian@paral.in> wrote:
> 
> > > I don't know in which context it might be useful in buildroot.  
> > 
> > I don't see a reason for it either.
> 
> Thanks for the feedback, I'll mark the patch as Rejected for now. We
> can of course revisit if there's some further details about why this
> would be needed.

Agreed, thanks.

> 
> Anisse, Christian: nobody is listed in the DEVELOPERS file for the
> package/pkg-golang.mk file. Would you be willing to be listed as
> DEVELOPERS for this file ? It would be useful as you would be Cc'ed on
> patches touching this file, which would help in situations like this
> one. If you're fine with this, send a patch to add yourself :)

Good catch, I was wondering why I didn't see the previous emails. I'll
send a patch to add myself and add better email filtering on my side.

Regards,

Anisse

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Cam Hutchison]

On Mon, 8 Apr 2019 at 17:05, Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Sun, 07 Apr 2019 19:00:20 -0700
> Christian Stewart <christian@paral.in> wrote:
>
> > > I don't know in which context it might be useful in buildroot.
> >
> > I don't see a reason for it either.
>
> Thanks for the feedback, I'll mark the patch as Rejected for now.

Well that's rather demoralising - waiting 9 months for some feedback
and given only 10 hours to respond before the patch is rejected.

My use case for this was for building Kubernetes, which builds some
components with CGO_ENABLED=0 and others without it. If I tried to
build some components with without it, I got linker errors. Since I
didn't feel like trying to debug/understand the whole Kubernetes build
process, I just went with building it the same way in buildroot as it
builds outside it. But for that I needed to be able to selectively
control CGO_ENABLED on a per-target basis.

But never mind. I can maintain this patch in my local buildroot patch
queue. I just thought it might be useful for others.

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Christian Stewart]

Cam,

On Mon, Apr 8, 2019, 5:22 PM Cam Hutchison <camh@xdna.net> wrote:

> But never mind. I can maintain this patch in my local buildroot patch
> queue. I just thought it might be useful for others.
>

Rejected doesn't mean rejected permanently, just rejected as is. This means
that more information is needed in the commit message, or maybe something
should be changed in the commit itself.

Fwiw, I'm also integrating kubernetes with buildroot at the moment. I don't
see where cgo is required in this way.

I'd be happy to compare notes if you'd like?

Best,
Christian

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20190408/fd2b9731/attachment-0001.html>

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Thomas Petazzoni]

Hello Cam,

On Tue, 9 Apr 2019 10:22:19 +1000
Cam Hutchison <camh@xdna.net> wrote:

> > Thanks for the feedback, I'll mark the patch as Rejected for now.  
> 
> Well that's rather demoralising - waiting 9 months for some feedback
> and given only 10 hours to respond before the patch is rejected.

Sorry for this. Taking a step back, I can understand how my very direct
decision can be disappointing.

However, as Christian explained, it's definitely not a "reject
forever", not at all. The status of patches in patchwork can always be
flipped back to whatever state we want.

As you have seen, we have a huge backlog of patches, so when the
feedback on a patch is fairly negative, I tend to immediately update
the patch state to not leave it pending forever. Otherwise, I forget
about it among the 200+ patches that are pending.

But clearly, if some additional explanation is given, either a new
iteration of the patch is sent, or alternatively we could apply the
original version after amending the commit log and/or adjusting the
implementation.

> My use case for this was for building Kubernetes, which builds some
> components with CGO_ENABLED=0 and others without it. If I tried to
> build some components with without it, I got linker errors. Since I
> didn't feel like trying to debug/understand the whole Kubernetes build
> process, I just went with building it the same way in buildroot as it
> builds outside it. But for that I needed to be able to selectively
> control CGO_ENABLED on a per-target basis.

So Christian said he would try to integrate Kubernetes, and see where
it goes. Would that work for you ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Cam Hutchison]

On Tue, 9 Apr 2019 at 13:22, Christian Stewart <christian@paral.in> wrote:
>
> Fwiw, I'm also integrating kubernetes with buildroot at the moment. I don't see where cgo is required in this way.
>
> I'd be happy to compare notes if you'd like?

My local BR2_EXTERNAL kubernetes config is at
https://github.com/camh-/clusterpi/tree/master/package/kubernetes.
Feel free to take anything from there that is useful.

I'm going to look at k3s instead now (https://github.com/rancher/k3s)
for its smaller size - probably more appropriate for my targets.

Regards,
Cam

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Cam Hutchison]

On Mon, 8 Apr 2019 at 06:28, Thomas Petazzoni <thomas.petazzoni@bootlin.com>
wrote:

> Hello Cam,
>
> +Christian and Anisse in Cc.
>
> Sorry for the very slow feedback.
>
> On Fri, 27 Jul 2018 12:47:20 +1000
> Cam Hutchison <camh@xdna.net> wrote:
>
> > Allow the CGO_ENABLED variable to be controlled per package and per
> > build target, instead of having the value determined by whether or not
> > the toolchain has threads.
> >
> > Some build targets may not build with CGO_ENABLED=1, so allowing a per
> > package and build target override will allow those targets to be built
> > with Buildroot.
>
> Could you give some specific examples instead of the fuzzy "Some build
> targets" wording ?
>

Sorry for my lack of detail. Here are my notes that I made at the time:

* kube-apiserver is not linking, emitting the error `direct call too far`.
If I
  override the go.mk to force CGO_ENABLED=0, it works. The kube build script
  builds a bunch of components with CGO_ENABLED=0, but the buildroot golang
  infra does not support setting CGO_ENABLED - it is hard-coded based on
  whether the toolchain has threads (BR2_TOOLCHAIN_HAS_THREADS). I will need
  to somehow override that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20190413/dffbe778/attachment.html>

[Buildroot] [PATCH 1/1] pkg-golang: Allow per package/target CGO_ENABLED setting

[Cam Hutchison]

On Sat, 13 Apr 2019 at 07:29, Cam Hutchison <camh@xdna.net> wrote:

> On Tue, 9 Apr 2019 at 13:22, Christian Stewart <christian@paral.in> wrote:
> >
> > Fwiw, I'm also integrating kubernetes with buildroot at the moment. I
> don't see where cgo is required in this way.
> >
> > I'd be happy to compare notes if you'd like?
>
> My local BR2_EXTERNAL kubernetes config is at
> https://github.com/camh-/clusterpi/tree/master/package/kubernetes.
> Feel free to take anything from there that is useful.
>

Also, here are my notes I made at the time. Perhaps they might be useful:

* Success building kubelet from buildroot
  + I think I've done that before, but I will next try other components
using
    the same pattern
  + Kubernetes buildroot config directly uses go to build binaries, does not
    use kube Makefile or build scripts.
* Success with kube-proxy and kubeadm
* kube-proxy, kubelet and kubeadm are the node binaries, all successfully
built
  from a single buildroot invocation. All were built with CGO_ENABLED=1.
  kube-proxy and kubeadm would normally be build with CGO_ENABLED=0 using
the
  kube build scripts (KUBE_STATIC_LIBRARIES)
* The buildroot golang support does not allow mixed CGO_ENABLED builds of
  different binaries. The value for CGO_ENABLED seems to be hard-coded at
  toolchain build time. I should investigate what it would take to build
both
  CGO_ENABLED=0 and CGO_ENABLED=1 together in the toolchain.
* kube-apiserver build failed. It needs openapi generated files. I had
thought
  that the openapi generation stuff was for docs - not so. I will have to
see
  what needs to be done to hook in the generated files build too. Shouldn't
be
  too hard as the go compiler can build both host and target binaries, so we
  don't need a second host-targetting go compiler.
* Extending the KUBERNETES_BUILD_CMDS to run `make generated_files` worked
just
  fine.
* kube-apiserver is not linking, emitting the error `direct call too far`.
If I
  override the go.mk to force CGO_ENABLED=0, it works. The kube build script
  builds a bunch of components with CGO_ENABLED=0, but the buildroot golang
  infra does not support setting CGO_ENABLED - it is hard-coded based on
  whether the toolchain has threads (BR2_TOOLCHAIN_HAS_THREADS). I will need
  to somehow override that.
* Everything builds now (including hyperkube) using the right CGO settings
for
  the right targets.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20190413/48fa4586/attachment.html>