[Buildroot] [RFC, v2, 2/4] package/pkg-golang: new package infrastructure

[Buildroot] [RFC, v2, 2/4] package/pkg-golang: new package infrastructure

[Christian Stewart]

Hi Angelo,

Angelo Compagnucci wrote:
> This patch adds a new infrastructure for golang based packages.

I have tested this new infrastructure against the docker series of packages:

https://github.com/paralin/buildroot/commits/docker-package-infra

 - https://github.com/paralin/buildroot/commit/74a1dd8913409b65a230c7c15acfd2df8fdc3d50
 - https://github.com/paralin/buildroot/commit/3873fdf90c4900fba2b27097eecdaadb73816ce3
 - https://github.com/paralin/buildroot/commit/c2a73eef568f9c6f656cf3344e2f6005cec8f5ca

In general though the changes seem to just be:

diff --git a/package/docker-engine/docker-engine.mk
b/package/docker-engine/docker-engine.mk
index 8494efdce7..70ab4a531a 100644
--- a/package/docker-engine/docker-engine.mk
+++ b/package/docker-engine/docker-engine.mk
@@ -11,7 +11,7 @@ DOCKER_ENGINE_SITE = $(call
github,docker,docker-ce,$(DOCKER_ENGINE_VERSION))
 DOCKER_ENGINE_LICENSE = Apache-2.0
 DOCKER_ENGINE_LICENSE_FILES = LICENSE

-DOCKER_ENGINE_DEPENDENCIES = host-go host-pkgconf
+DOCKER_ENGINE_DEPENDENCIES = host-pkgconf

 DOCKER_ENGINE_GOPATH = "$(@D)/gopath"
 DOCKER_ENGINE_MAKE_ENV = $(HOST_GO_TARGET_ENV) \
@@ -138,4 +138,4 @@ define DOCKER_ENGINE_INSTALL_TARGET_CMDS
  )
 endef

-$(eval $(generic-package))
+$(eval $(golang-package))

While the package infra code itself works fine in this case because I
override almost everything in these packages, I am skeptical of how
useful it would be in the general case for the following reasons:

 - The go code is already pretty succinct, aside from having a
dependency on host-go which looks just fine to me.
 - Go packages often do not have their main package in the root of the
repo. You need to allow package developers to specify multiple targets
in subdirs, like cmd/hyperkube, cmd/kubelet, etc.
 - Typically executables exist under a cmd/${executable} path which is
nice and predictable. I would almost make the default build path
cmd/kubelet.
- Is  CGO_NO_EMULATION=1  necessary? I use it in docker-engine, not sure why.
- Please add a mechanism to specify Go build tags
- In terms of gopath, determining the path to the executable / the
location inside the gopath is scary to me. Can we please set it
explicitly in Go packages? Like
RUNC_GO_PATH=github.com/opencontainers/runc - please see the runc.mk
file and make a proposed revision to that as well as flannel.

Overall I think it works and may potentially be useful but at the
moment it doesn't do enough to merit adding an entire new package
infrastructure for it, if I'm going to override 99% of it anyway. All
it does right now for every go package I've tried it on is remove the
host-go dependency, which I almost like having because it's more
explicit. Otherwise in these cases I've had to hand override most of
the infra because Go packages do not conform to as typical of a format
as autotools or cmake.

Thanks!
Christian Stewart

[Buildroot] [RFC, v2, 2/4] package/pkg-golang: new package infrastructure

[Angelo Compagnucci]

Dear Christian Stewart,

2017-10-19 18:06 GMT+02:00 Christian Stewart <christian@paral.in>:
> Hi Angelo,
>
> Angelo Compagnucci wrote:
>> This patch adds a new infrastructure for golang based packages.
>
> I have tested this new infrastructure against the docker series of packages:
>
> https://github.com/paralin/buildroot/commits/docker-package-infra
>
>  - https://github.com/paralin/buildroot/commit/74a1dd8913409b65a230c7c15acfd2df8fdc3d50
>  - https://github.com/paralin/buildroot/commit/3873fdf90c4900fba2b27097eecdaadb73816ce3
>  - https://github.com/paralin/buildroot/commit/c2a73eef568f9c6f656cf3344e2f6005cec8f5ca
>
> In general though the changes seem to just be:
>
> diff --git a/package/docker-engine/docker-engine.mk
> b/package/docker-engine/docker-engine.mk
> index 8494efdce7..70ab4a531a 100644
> --- a/package/docker-engine/docker-engine.mk
> +++ b/package/docker-engine/docker-engine.mk
> @@ -11,7 +11,7 @@ DOCKER_ENGINE_SITE = $(call
> github,docker,docker-ce,$(DOCKER_ENGINE_VERSION))
>  DOCKER_ENGINE_LICENSE = Apache-2.0
>  DOCKER_ENGINE_LICENSE_FILES = LICENSE
>
> -DOCKER_ENGINE_DEPENDENCIES = host-go host-pkgconf
> +DOCKER_ENGINE_DEPENDENCIES = host-pkgconf
>
>  DOCKER_ENGINE_GOPATH = "$(@D)/gopath"
>  DOCKER_ENGINE_MAKE_ENV = $(HOST_GO_TARGET_ENV) \
> @@ -138,4 +138,4 @@ define DOCKER_ENGINE_INSTALL_TARGET_CMDS
>   )
>  endef
>
> -$(eval $(generic-package))
> +$(eval $(golang-package))
>
> While the package infra code itself works fine in this case because I
> override almost everything in these packages, I am skeptical of how
> useful it would be in the general case for the following reasons:
>
>  - The go code is already pretty succinct, aside from having a
> dependency on host-go which looks just fine to me.
>  - Go packages often do not have their main package in the root of the
> repo. You need to allow package developers to specify multiple targets
> in subdirs, like cmd/hyperkube, cmd/kubelet, etc.

Yes right. I actually changed the logic of the build phase to use the
go build/install to take advantage of the go building system.

>  - Typically executables exist under a cmd/${executable} path which is
> nice and predictable. I would almost make the default build path
> cmd/kubelet.

I think with should stick with standard go rules for packages. Usually
packages are installed in GOPATH/bin when crosscompilation is not
enabled or GOPATH/bin/$GOOS_$GOARCH/ when crosscompilation is enabled,
stating to the go manual.
I think that simply using go install should be sufficient. The only
problem remaining could be that a package that needs to install
multiple binaries in the host or target directory should implement a
host/target install step and can't use the default one.


> - Is  CGO_NO_EMULATION=1  necessary? I use it in docker-engine, not sure why.

Unfortunately I cannot find any information about this variable or
google nor the manual. I don't think it makes something.

> - Please add a mechanism to specify Go build tags

Will do.

> - In terms of gopath, determining the path to the executable / the
> location inside the gopath is scary to me. Can we please set it
> explicitly in Go packages? Like
> RUNC_GO_PATH=github.com/opencontainers/runc - please see the runc.mk
> file and make a proposed revision to that as well as flannel.

It is already provided by the GO_FOO_GO_SRC_PATH variable.
Btw I think that there is no problem in guessing the path cause all
the projects I worked with use the domain/vendor/software convention.


> Overall I think it works and may potentially be useful but at the
> moment it doesn't do enough to merit adding an entire new package
> infrastructure for it, if I'm going to override 99% of it anyway. All
> it does right now for every go package I've tried it on is remove the
> host-go dependency, which I almost like having because it's more
> explicit. Otherwise in these cases I've had to hand override most of
> the infra because Go packages do not conform to as typical of a format
> as autotools or cmake.

I will do a respin of the series with the runc package converted to
the golang infrastructure and you will find it much smaller!

>
> Thanks!
> Christian Stewart



-- 
Profile: http://it.linkedin.com/in/compagnucciangelo

[Buildroot] [RFC, v2, 2/4] package/pkg-golang: new package infrastructure

[Christian Stewart]

Hi Angelo,

On Mon, Oct 23, 2017 at 11:50 AM, Angelo Compagnucci
<angelo.compagnucci@gmail.com> wrote:
>> - Is  CGO_NO_EMULATION=1  necessary? I use it in docker-engine, not sure why.
>
> Unfortunately I cannot find any information about this variable or
> google nor the manual. I don't think it makes something.

https://github.com/LibreELEC/LibreELEC.tv/pull/1669/files#diff-3fa55ea381e3943a7d8440c8adbb6bb1R40

But, you're right, I can't find it anywhere else.

> It is already provided by the GO_FOO_GO_SRC_PATH variable.
> Btw I think that there is no problem in guessing the path cause all
> the projects I worked with use the domain/vendor/software convention.

It looked to me like it was being overwritten but I may be wrong.


> I will do a respin of the series with the runc package converted to
> the golang infrastructure and you will find it much smaller!

Looking forward to it!

Best,
Christian

[Buildroot] [RFC v2 2/4] package/pkg-golang: new package infrastructure

[Angelo Compagnucci]

Dear Thomas,

2017-10-21 21:55 GMT+02:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Hello,
>
> On Mon, 16 Oct 2017 19:08:46 +0200, Angelo Compagnucci wrote:
>> This patch adds a new infrastructure for golang based packages.
>>
>> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
>
> As Christian said, it seems that this infrastructure isn't "general"
> enough to really be useful. We would to see it applied to several
> packages and see that there is a real benefit to several packages to
> apply it.
>
> So I'd be happy to take a go package infrastructure, but it needs to be
> usable by more than one package in a useful way.
>
>> +# Target packages need the Go compiler on the host.
>> +$(2)_DEPENDENCIES += host-go
>> +
>> +#
>> +# go install command doesn't work well when cross compilation is enabled,
>> +# we set the executable output of the compilation to a specific location.
>> +# We set this variable here to be used by packages if needed.
>> +#
>> +$(2)_EXECUTABLE = $$(@D)/gopath/bin/$(1)
>
> This seems to make the assumption that a given package can only install
> a single executable, and that this executable is named after the
> package. It is true for flannel, but what about other Go packages ?

It will be addressed in the respin of the series.

>
>> +# Source files in Go should be uncompressed in a precise folder in the
>> +# hiearchy of GOPATH. It usually resolves around domain/vendor/software.
>> +#
>> +$(1)_src_path ?= $$(call domain,$($(2)_SITE))/$$(firstword $$(subst /, ,$$(call notdomain,$($(2)_SITE))))
>> +$(2)_SRC_PATH  = $$(@D)/gopath/src/$$($(1)_src_path)/$(1)
>
> We don't use lower-case variables in Buildroot, especially if there is
> a variable with the same name upper-case.

Fixed, thanks!

>
> Best regards,
>
> Thomas Petazzoni
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com



-- 
Profile: http://it.linkedin.com/in/compagnucciangelo

[Buildroot] [RFC v2 2/4] package/pkg-golang: new package infrastructure

[Thomas Petazzoni]

Hello,

On Mon, 16 Oct 2017 19:08:46 +0200, Angelo Compagnucci wrote:
> This patch adds a new infrastructure for golang based packages.
> 
> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>

As Christian said, it seems that this infrastructure isn't "general"
enough to really be useful. We would to see it applied to several
packages and see that there is a real benefit to several packages to
apply it.

So I'd be happy to take a go package infrastructure, but it needs to be
usable by more than one package in a useful way.

> +# Target packages need the Go compiler on the host.
> +$(2)_DEPENDENCIES += host-go
> +
> +#
> +# go install command doesn't work well when cross compilation is enabled,
> +# we set the executable output of the compilation to a specific location.
> +# We set this variable here to be used by packages if needed.
> +#
> +$(2)_EXECUTABLE = $$(@D)/gopath/bin/$(1)

This seems to make the assumption that a given package can only install
a single executable, and that this executable is named after the
package. It is true for flannel, but what about other Go packages ?

> +# Source files in Go should be uncompressed in a precise folder in the
> +# hiearchy of GOPATH. It usually resolves around domain/vendor/software.
> +#
> +$(1)_src_path ?= $$(call domain,$($(2)_SITE))/$$(firstword $$(subst /, ,$$(call notdomain,$($(2)_SITE))))
> +$(2)_SRC_PATH  = $$(@D)/gopath/src/$$($(1)_src_path)/$(1)

We don't use lower-case variables in Buildroot, especially if there is
a variable with the same name upper-case.

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFC v2 2/4] package/pkg-golang: new package infrastructure

[Angelo Compagnucci]

This patch adds a new infrastructure for golang based packages.

Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
---
 package/Makefile.in   |   1 +
 package/pkg-golang.mk | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)
 create mode 100644 package/pkg-golang.mk

diff --git a/package/Makefile.in b/package/Makefile.in
index a1a5316..60d98d0 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -429,3 +429,4 @@ include package/pkg-kconfig.mk
 include package/pkg-rebar.mk
 include package/pkg-kernel-module.mk
 include package/pkg-waf.mk
+include package/pkg-golang.mk
diff --git a/package/pkg-golang.mk b/package/pkg-golang.mk
new file mode 100644
index 0000000..c4e6410
--- /dev/null
+++ b/package/pkg-golang.mk
@@ -0,0 +1,121 @@
+################################################################################
+# Golang package infrastructure
+#
+# This file implements an infrastructure that eases development of
+# package .mk files for Go packages. It should be used for all
+# packages that are written in go.
+#
+# See the Buildroot documentation for details on the usage of this
+# infrastructure
+#
+#
+# In terms of implementation, this golang infrastructure requires
+# the .mk file to only specify metadata information about the
+# package: name, version, download URL, etc.
+#
+# We still allow the package .mk file to override what the different
+# steps are doing, if needed. For example, if <PKG>_BUILD_CMDS is
+# already defined, it is used as the list of commands to perform to
+# build the package, instead of the default golang behaviour. The
+# package can also define some post operation hooks.
+#
+################################################################################
+
+################################################################################
+# inner-golang-package -- defines how the configuration, compilation and
+# installation of a Go package should be done, implements a few hooks to
+# tune the build process for Go specifities and calls the generic package
+# infrastructure to generate the necessary make targets
+#
+#  argument 1 is the lowercase package name
+#  argument 2 is the uppercase package name, including a HOST_ prefix
+#             for host packages
+#  argument 3 is the uppercase package name, without the HOST_ prefix
+#             for host packages
+#  argument 4 is the type (target or host)
+################################################################################
+
+define inner-golang-package
+
+ifndef $(2)_MAKE_ENV
+define $(2)_MAKE_ENV
+	$$(HOST_GO_TARGET_ENV) \
+	GOPATH="$$(@D)/gopath" \
+	CGO_ENABLED=$$(HOST_GO_CGO_ENABLED)
+endef
+endif
+
+# Target packages need the Go compiler on the host.
+$(2)_DEPENDENCIES += host-go
+
+#
+# go install command doesn't work well when cross compilation is enabled,
+# we set the executable output of the compilation to a specific location.
+# We set this variable here to be used by packages if needed.
+#
+$(2)_EXECUTABLE = $$(@D)/gopath/bin/$(1)
+
+#
+# Source files in Go should be uncompressed in a precise folder in the
+# hiearchy of GOPATH. It usually resolves around domain/vendor/software.
+#
+$(1)_src_path ?= $$(call domain,$($(2)_SITE))/$$(firstword $$(subst /, ,$$(call notdomain,$($(2)_SITE))))
+$(2)_SRC_PATH  = $$(@D)/gopath/src/$$($(1)_src_path)/$(1)
+
+#
+# Configure step. Only define it if not already defined by the package
+# .mk file. And take care of the differences between host and target
+# packages.
+#
+ifndef $(2)_CONFIGURE_CMDS
+define $(2)_CONFIGURE_CMDS
+	mkdir -p $$(@D)/gopath/bin
+	mkdir -p $$(@D)/gopath/src/$$($(1)_src_path)
+	ln -sf $$(@D) $$($(2)_SRC_PATH)
+endef
+endif
+
+#
+# Build step. Only define it if not already defined by the package .mk file.
+# There is no differences between host and target packages.
+#
+ifndef $(2)_BUILD_CMDS
+define $(2)_BUILD_CMDS
+	cd $$($(2)_SRC_PATH) && $$($(2)_MAKE_ENV) $(HOST_DIR)/bin/go build \
+	-o $$($(2)_EXECUTABLE) -v $$($(2)_BUILD_OPTS)
+endef
+endif
+
+#
+# Host installation step. Only define it if not already defined by the
+# package .mk file.
+#
+ifndef $(2)_INSTALL_CMDS
+define $(2)_INSTALL_CMDS
+	$(INSTALL) -D -m 0755 $$($(2)_EXECUTABLE) $(HOST_DIR)/usr/bin/
+endef
+endif
+
+#
+# Target installation step. Only define it if not already defined by the
+# package .mk file.
+#
+ifndef $(2)_INSTALL_TARGET_CMDS
+define $(2)_INSTALL_TARGET_CMDS
+	$(INSTALL) -D -m 0755 $$($(2)_EXECUTABLE) $(TARGET_DIR)/usr/bin/
+endef
+endif
+
+# Call the generic package infrastructure to generate the necessary make
+# targets
+$(call inner-generic-package,$(1),$(2),$(3),$(4))
+
+endef # inner-golang-package
+
+################################################################################
+# golang-package -- the target generator macro for Go packages
+################################################################################
+
+golang-package = $(call inner-golang-package,$(pkgname),$(call UPPERCASE,$(pkgname)),$(call UPPERCASE,$(pkgname)),target)
+host-golang-package = $(call inner-golang-package,host-$(pkgname),$(call UPPERCASE,host-$(pkgname)),$(call UPPERCASE,$(pkgname)),host)
+
-- 
2.7.4