[Buildroot] [RFC v4 1/4] package/pkg-golang: new package infrastructure
Angelo Compagnucci
angelo.compagnucci at gmail.com
Wed Oct 25 08:53:19 UTC 2017
Dear Arnout Vandecappelle,
2017-10-25 0:39 GMT+02:00 Arnout Vandecappelle <arnout at mind.be>:
> Hi Angelo,
>
> Incomplete review but at least something you can work with :-)
>
> On 24-10-17 23:14, Angelo Compagnucci wrote:
>> This patch adds a new infrastructure for golang based packages.
>>
>> Signed-off-by: Angelo Compagnucci <angelo.compagnucci at gmail.com>
> [snip]> +define inner-golang-package
>> +
>> +$(2)_GOPATH ?= _gopath
>
> Is there ever a need for a package to override this? docker-containerd sets it
> to 'vendor' but I don't think there's a good reason for that.
Yes, there's a reason for that. I converted also the docker-containerd
package and in the following respin of the patch you can see why it is
needed.
>> +
>> +ifndef $(2)_MAKE_ENV
>> +define $(2)_MAKE_ENV
>> + $$(HOST_GO_TARGET_ENV) \
>> + GOPATH="$$(@D)/$$($(2)_GOPATH)" \
>> + CGO_ENABLED=$$(HOST_GO_CGO_ENABLED)
>> +endef
>> +endif
>
> Don't do it like this, do it like in autotools: these options are always
> provided in the build environment, and $(2)_MAKE_ENV (default empty but no need
> to set it here) comes after it.
>
> Also, _MAKE_ENV is not a good name since you're not calling make. Just _ENV or
> _BUILD_ENV or _GO_ENV is better.
Will do.
>
>> +
>> +ifdef $(2)_GO_LDFLAGS
>> + $(2)_BUILD_OPTS += -ldflags "$$($(2)_GO_LDFLAGS)"
>> +endif
>> +
>> +ifeq ($(BR2_STATIC_LIBS),y)
>> + $(2)_BUILD_OPTS += -extldflags '-static'
>> +endif
>
> This is NOT what is currently done for flannel and runc, they put this in the
> -ldflags argument.
Good catch!
>
>> +
>> +ifdef $(2)_GO_TAGS
>> + $(2)_BUILD_OPTS += -tags "$$($(2)_GO_TAGS)"
>> +endif
>
> I guess it would make sense to also have
>
> $(2)_GO_TAGS ?= $$($(3)_GO_TAGS)
>
> (i.e. inherit the target tags for the host) though I'm not sure that is always
> going to be relevant.
>
>> +
>> +# Target packages need the Go compiler on the host.
>> +$(2)_DEPENDENCIES += host-go
>> +
>> +#
>> +# The go build/install command installs the binaries inside
>> +# gopath/bin/linux_GOARCH/ when cross compilation is enabled.
>> +# We set this variable here to be used by packages if needed.
>> +#
>> +$(2)_BINDIR = $$(@D)/$$($(2)_GOPATH)/bin/linux_$$(GO_GOARCH)
>
> What about host packages?
>
>> +
>> +#
>> +# Source files in Go should be uncompressed in a precise folder in the
>> +# hierarchy of GOPATH. It usually resolves around domain/vendor/software.
>> +#
>> +$(2)_GO_SRC_PATH ?= $$(call domain,$($(2)_SITE))/$$(firstword $$(subst /, ,$$(call notdomain,$($(2)_SITE))))
>> +$(2)_SRC_PATH = $$(@D)/$$($(2)_GOPATH)/src/$$($(2)_GO_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
>
> I don't see any differences?
>
>> +# packages.
>> +#
>> +ifndef $(2)_CONFIGURE_CMDS
>> +define $(2)_CONFIGURE_CMDS
>> + mkdir -p $$(@D)/$$($(2)_GOPATH)/bin
>> + mkdir -p $$(@D)/$$($(2)_GOPATH)/src/$$($(2)_GO_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.
>
> Really? How is that possible that exactly the same commands sometimes build for
> the host and sometimes for the target?
>
> Since we currently don't have host-go packages, I think it's better to remove
> support for host-go packages entirely. We can add it when there is an example.
Yes, reasoning on that I think you are right. I'll remove the host part.
>
>> +# We use the install command instead of build command here because the
>> +# install command also moves the package binaries in gopath/bin/linux_GOARCH/.
>> +# Using the install command also leverages the go build infrastructure
>> +# for building and installing multiple binaries.
>> +#
>> +ifndef $(2)_BUILD_CMDS
>> +define $(2)_BUILD_CMDS
>> + cd $$($(2)_SRC_PATH) && \
>> + $$($(2)_MAKE_ENV) $(HOST_DIR)/bin/go install \
>> + -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)_BINDIR)/$(1) $(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)_BINDIR)/$(1) $(TARGET_DIR)/usr/bin/
>
> There is no guarantee at all that the binary name is the same as the package
> name, so better let the package override it.
Nope. Go mandates[1] that if a package contains only a binary, that
binary will be named as the package unless you overwrite this behavior
with the -o option at build time.
If a package contains multiples main.go files, each binary will be
named with name of the subfolder containing it and -o option is of
source disabled.
> Also, it would be convenient to
> support building multiple binaries like is done now for docker-containerd and
> docker-engine. But I'm all for delaying that to a future patch, let's get this
> thing in first.
Honestly, it's not so common in the go world to have multiple binaries
in the same source tree. I think we should treat this as a corner case
and let the developer doing what it's best. In the follow up of the
series I added also docker-containerd to show how a non standard
package could be implemented.
>
>
> Regards,
> Arnout
>
>
>> +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)
>> +
>>
>
> --
> Arnout Vandecappelle arnout at mind be
> Senior Embedded Software Architect +32-16-286500
> Essensium/Mind http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
[1] https://golang.org/cmd/go/#hdr-Compile_packages_and_dependencies
--
Profile: http://it.linkedin.com/in/compagnucciangelo
More information about the buildroot
mailing list