| Message ID | 20220720125707.1899437-1-festevam@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | package/crucible: Pass 'osusergo netgo' tags | expand |
Hello Fabio, Hello Andrea, On Wed, 20 Jul 2022 09:57:07 -0300 Fabio Estevam <festevam@gmail.com> wrote: > The 'osusergo netgo' tags need to be passed when CGO_ENABLED=1. > > Fixes: > > http://autobuild.buildroot.net/results/48a39341ca3dcb3675b2876e680718c3e43f55aa > http://autobuild.buildroot.net/results/897b4803f4f221c52899641a45708b49b7cb5f5c > http://autobuild.buildroot.net/results/3a75c6b73a1f583d6db666eacc45c4debd9218e9 > > Suggested-by: Andrea Barisani <andrea.barisani@withsecure.com> > Signed-off-by: Fabio Estevam <festevam@gmail.com> Could you provide more background about these osusergo and netgo tags? Indeed, they don't seem to be specific to the crucible package, but are apparently generic Go tags, at least according to https://www.arp242.net/static-go.html, and they seem to be needed when statically linking Go programs. If that is the case, then I don't think the solution is to change crucible.mk, but probably to have a more generic solution in package/pkg-golang.mk. Or is that package-specific in some way? Do we understand under which conditions these tags are needed? Thanks in advance for your feedback! Thomas
Hello, On Wed, 20 Jul 2022 17:05:42 +0000 "Barisani, Andrea" <andrea.barisani@withsecure.com> wrote: > These flags are not crucible specific but they are necessary when > compiling static Go binaries with CGO enabled (which crucible does > not require by the way) to ensure portability as cgo versions of net > and os/users are not portable. Thanks for your feedback, much appreciated! So, I see we're passing CGO_ENABLED=1 (when thread support is available) to all packages, and you're saying that this is incorrect as not all Go packages need CGO? Passing CGO_ENABLED=1/0 should be a per-package decision? However, when CGO is enabled and we need static linking, passing osusergo and netgo tags is needed, correct? Thomas
On 20/07/2022 22:24, Thomas Petazzoni via buildroot wrote: > Hello, > > On Wed, 20 Jul 2022 17:05:42 +0000 > "Barisani, Andrea" <andrea.barisani@withsecure.com> wrote: > >> These flags are not crucible specific but they are necessary when >> compiling static Go binaries with CGO enabled (which crucible does >> not require by the way) to ensure portability as cgo versions of net >> and os/users are not portable. > > Thanks for your feedback, much appreciated! > > So, I see we're passing CGO_ENABLED=1 (when thread support is AFAICS, CGO_ENABLED is only set in HOST_GO_MAKE_ENV, which is only used in the host-go build commands. So it's not really possible to set it per package. However, apparently it is possible to override it: DOCKER_CLI_GO_ENV = CGO_ENABLED=no the commit message that introduced that unfortunately doesn't explain why, I'm not even sure if it does anything. Regards, Arnout > available) to all packages, and you're saying that this is incorrect as > not all Go packages need CGO? Passing CGO_ENABLED=1/0 should be a > per-package decision? > > However, when CGO is enabled and we need static linking, passing > osusergo and netgo tags is needed, correct? > > Thomas
Hello, On Fri, 22 Jul 2022 23:04:57 +0200 Arnout Vandecappelle <arnout@mind.be> wrote: > AFAICS, CGO_ENABLED is only set in HOST_GO_MAKE_ENV, which is only used in the > host-go build commands. So it's not really possible to set it per package. > However, apparently it is possible to override it: I see this: HOST_GO_COMMON_ENV = \ GO111MODULE=on \ GOFLAGS=-mod=vendor \ GOROOT="$(HOST_GO_ROOT)" \ GOPATH="$(HOST_GO_GOPATH)" \ GOPROXY=off \ PATH=$(BR_PATH) \ GOBIN= \ CGO_ENABLED=$(HOST_GO_CGO_ENABLED) HOST_GO_TARGET_ENV = \ $(HOST_GO_COMMON_ENV) \ GOOS="linux" \ GOARCH=$(GO_GOARCH) \ GOCACHE="$(HOST_GO_TARGET_CACHE)" \ CC="$(TARGET_CC)" \ CXX="$(TARGET_CXX)" \ CGO_CFLAGS="$(TARGET_CFLAGS)" \ CGO_CXXFLAGS="$(TARGET_CXXFLAGS)" \ CGO_LDFLAGS="$(TARGET_LDFLAGS)" \ GOTOOLDIR="$(HOST_GO_TOOLDIR)" so HOST_GO_TARGET_ENV has CGO_ENABLED=$(HOST_GO_CGO_ENABLED) and then HOST_GO_TARGET_ENV gets used in: # Build package for target define $(2)_BUILD_CMDS $$(foreach d,$$($(2)_BUILD_TARGETS),\ cd $$(@D); \ $$(HOST_GO_TARGET_ENV) \ $$($(2)_GO_ENV) \ $$(GO_BIN) build -v $$($(2)_BUILD_OPTS) \ -o $$(@D)/bin/$$(or $$($(2)_BIN_NAME),$$(notdir $$(d))) \ $$($(2)_GOMOD)/$$(d) ) endef Am I looking wrong? Thomas
On 22/07/2022 23:15, Thomas Petazzoni wrote: > Hello, > > On Fri, 22 Jul 2022 23:04:57 +0200 > Arnout Vandecappelle <arnout@mind.be> wrote: > >> AFAICS, CGO_ENABLED is only set in HOST_GO_MAKE_ENV, which is only used in the >> host-go build commands. So it's not really possible to set it per package. >> However, apparently it is possible to override it: > > I see this: > > HOST_GO_COMMON_ENV = \ > GO111MODULE=on \ > GOFLAGS=-mod=vendor \ > GOROOT="$(HOST_GO_ROOT)" \ > GOPATH="$(HOST_GO_GOPATH)" \ > GOPROXY=off \ > PATH=$(BR_PATH) \ > GOBIN= \ > CGO_ENABLED=$(HOST_GO_CGO_ENABLED) > > HOST_GO_TARGET_ENV = \ > $(HOST_GO_COMMON_ENV) \ > GOOS="linux" \ > GOARCH=$(GO_GOARCH) \ > GOCACHE="$(HOST_GO_TARGET_CACHE)" \ > CC="$(TARGET_CC)" \ > CXX="$(TARGET_CXX)" \ > CGO_CFLAGS="$(TARGET_CFLAGS)" \ > CGO_CXXFLAGS="$(TARGET_CXXFLAGS)" \ > CGO_LDFLAGS="$(TARGET_LDFLAGS)" \ > GOTOOLDIR="$(HOST_GO_TOOLDIR)" > > so HOST_GO_TARGET_ENV has CGO_ENABLED=$(HOST_GO_CGO_ENABLED) > > and then HOST_GO_TARGET_ENV gets used in: > > # Build package for target > define $(2)_BUILD_CMDS > $$(foreach d,$$($(2)_BUILD_TARGETS),\ > cd $$(@D); \ > $$(HOST_GO_TARGET_ENV) \ > $$($(2)_GO_ENV) \ > $$(GO_BIN) build -v $$($(2)_BUILD_OPTS) \ > -o $$(@D)/bin/$$(or $$($(2)_BIN_NAME),$$(notdir $$(d))) \ > $$($(2)_GOMOD)/$$(d) > ) > endef > > Am I looking wrong? No, I'm looking wrong, of course. I didn't see the one in HOST_GO_COMMON_ENV because it comes before the definition of HOST_GO_CGO_ENABLED :-( So indeed, the question if we should have a per-package option to enable cgo is relevant. I'd actually expect that the package itself would specify whether it needs cgo or not. Can we test a build of a package that actually uses cgo to verify if we really need to set it in HOST_GO_COMMON_ENV? Regardless, we *will* need to set the 'osusergo netgo' tags for all cgo-using packages when they do use cgo - so either we still need a per-package "enable cgo" option, or we should just always add those tags if cgo is enabled and we're doing static linking. Is there any reason why this problem is only observed for the crucible package? Regards, Arnout > > Thomas
Hi all, On Fri, Jul 22, 2022 at 3:03 PM Arnout Vandecappelle <arnout@mind.be> wrote: > I'd actually expect that the package itself would specify whether it needs cgo > or not. Can we test a build of a package that actually uses cgo to verify if we > really need to set it in HOST_GO_COMMON_ENV? "CGO_ENABLED" informs the compiler that it's OK to use Cgo. It should still be set to 1 even if the package doesn't use cgo at all. > Regardless, we *will* need to set the 'osusergo netgo' tags for all cgo-using > packages when they do use cgo - so either we still need a per-package "enable > cgo" option, or we should just always add those tags if cgo is enabled and we're > doing static linking. > > Is there any reason why this problem is only observed for the crucible package? So, this doesn't work? FOO_GO_ENV += CGO_ENABLED=0 .. because the package needs tags enabled conditionally if cgo is available or not? I don't understand why this specific package can't just check HOST_GO_CGO_ENABLED and conditionally add the tags? Best, Christian Stewart
On 23/07/2022 02:41, Christian Stewart wrote: > Hi all, > > > On Fri, Jul 22, 2022 at 3:03 PM Arnout Vandecappelle <arnout@mind.be> wrote: >> I'd actually expect that the package itself would specify whether it needs cgo >> or not. Can we test a build of a package that actually uses cgo to verify if we >> really need to set it in HOST_GO_COMMON_ENV? > > "CGO_ENABLED" informs the compiler that it's OK to use Cgo. > > It should still be set to 1 even if the package doesn't use cgo at all. > >> Regardless, we *will* need to set the 'osusergo netgo' tags for all cgo-using >> packages when they do use cgo - so either we still need a per-package "enable >> cgo" option, or we should just always add those tags if cgo is enabled and we're >> doing static linking. >> >> Is there any reason why this problem is only observed for the crucible package? > > So, this doesn't work? > > FOO_GO_ENV += CGO_ENABLED=0 > > .. because the package needs tags enabled conditionally if cgo is > available or not? > > I don't understand why this specific package can't just check > HOST_GO_CGO_ENABLED and conditionally add the tags? Yes it can. However, Andrea wrote: > These flags are not crucible specific but they are necessary when > compiling static Go binaries with CGO enabled (which crucible does > not require by the way) to ensure portability as cgo versions of net > and os/users are not portable. and we are wondering why only crucible needs those tags, and other Go packages don't. And what is especially weird is the part "which crucible does not require" which makes it sound as if crucible isn't using cgo at all... Regards, Arnout > > Best, > Christian Stewart
On 23/07/2022 10:54, Arnout Vandecappelle wrote: > > > On 23/07/2022 02:41, Christian Stewart wrote: >> Hi all, >> >> >> On Fri, Jul 22, 2022 at 3:03 PM Arnout Vandecappelle <arnout@mind.be> wrote: >>> I'd actually expect that the package itself would specify whether it >>> needs cgo >>> or not. Can we test a build of a package that actually uses cgo to verify if we >>> really need to set it in HOST_GO_COMMON_ENV? >> >> "CGO_ENABLED" informs the compiler that it's OK to use Cgo. >> >> It should still be set to 1 even if the package doesn't use cgo at all. >> >>> Regardless, we *will* need to set the 'osusergo netgo' tags for all >>> cgo-using >>> packages when they do use cgo - so either we still need a per-package "enable >>> cgo" option, or we should just always add those tags if cgo is enabled and >>> we're >>> doing static linking. >>> >>> Is there any reason why this problem is only observed for the crucible >>> package? >> >> So, this doesn't work? >> >> FOO_GO_ENV += CGO_ENABLED=0 >> >> .. because the package needs tags enabled conditionally if cgo is >> available or not? >> >> I don't understand why this specific package can't just check >> HOST_GO_CGO_ENABLED and conditionally add the tags? > > Yes it can. However, Andrea wrote: > >> These flags are not crucible specific but they are necessary when >> compiling static Go binaries with CGO enabled (which crucible does >> not require by the way) to ensure portability as cgo versions of net >> and os/users are not portable. > > and we are wondering why only crucible needs those tags, and other Go packages > don't. And what is especially weird is the part "which crucible does not > require" which makes it sound as if crucible isn't using cgo at all... It turns out that other packages also do need it, e.g. containerd and delve. So I moved this to the pkg-golang infra [1], and marked this patch as Superseded. Regards, Arnout [1] https://patchwork.ozlabs.org/project/buildroot/patch/20220918122239.189147-1-arnout@mind.be/
Hi Arnout, On Sun, Sep 18, 2022 at 9:50 AM Arnout Vandecappelle <arnout@mind.be> wrote: > It turns out that other packages also do need it, e.g. containerd and delve. > > So I moved this to the pkg-golang infra [1], and marked this patch as Superseded. Thanks for taking care of this. Regards, Fabio Estevam
diff --git a/package/crucible/crucible.mk b/package/crucible/crucible.mk index fdad709dde..fdcda109c2 100644 --- a/package/crucible/crucible.mk +++ b/package/crucible/crucible.mk @@ -9,5 +9,6 @@ CRUCIBLE_SITE = $(call github,usbarmory,crucible,v$(CRUCIBLE_VERSION)) CRUCIBLE_LICENSE = GPL-3.0 CRUCIBLE_LICENSE_FILES = LICENSE CRUCIBLE_GOMOD = ./cmd/crucible +CRUCIBLE_TAGS += osusergo netgo $(eval $(golang-package))
The 'osusergo netgo' tags need to be passed when CGO_ENABLED=1. Fixes: http://autobuild.buildroot.net/results/48a39341ca3dcb3675b2876e680718c3e43f55aa http://autobuild.buildroot.net/results/897b4803f4f221c52899641a45708b49b7cb5f5c http://autobuild.buildroot.net/results/3a75c6b73a1f583d6db666eacc45c4debd9218e9 Suggested-by: Andrea Barisani <andrea.barisani@withsecure.com> Signed-off-by: Fabio Estevam <festevam@gmail.com> --- package/crucible/crucible.mk | 1 + 1 file changed, 1 insertion(+)