| Message ID | 20240412132659.1168317-4-thomas.perale@mind.be |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | support for a pre-built Go compiler | expand |
Thomas, All, Thanks for this series, it really looks good! :-) I anyway have a few comments and questions, please see below. On 2024-04-12 15:26 +0200, Thomas Perale via buildroot spake thusly: > Turns host-go into a virtual package, which has two providers: With this patch, there is a single provider, so the commit log is not correct. Instead, it should state something like: Turn host-go into a virtual package, which currently has a single provider, the existing host-go package. In later commits, we'll add a prebuilt, binary host go compiler as another provider. > - host-go-src, which builds host-go from source based on the same logic > that was previously used in package/go/go/go.mk, now moved to > package/go/go-src/go-src.mk to remove any ambiguity on the role of > the package. > > - host-go-bin, which will be introduced in the next commit and installs > a pre-built Go compiler. > > The usage of a virtual package enables support for pre-built Go s/usage/use/ > compiler, which reduces the build time for of host-go. > A similar solution is proposed for host-rust. > > By default, host-go is built from sources to keep the same behavior as > the former version. Since this commit only introduces a single provider, this comment does not make sense, and belongs to the commit that adds the prebuilt go. Not related to this patch, but still relevant to the discussion: it would be even better if we could also have the possibility that the stage-3 has the option of being a prebuilt go. Indeed, it would still be interesting to be able to build the final go from source, but use a prebuild bootstrap. So that would allow for three situations: - full source: stage-1, stage-2, stage-3, and final go, all built from sources; - prebuilt stage-3 (so not stage-1 and no stage-2), but final built from sources; - no bootstrap at all, and prebuilt final go. Of course, this is _not_ required for this series to progress. > Signed-off-by: Thomas Perale <thomas.perale@mind.be> > --- > diff --git a/package/go/Config.in.host b/package/go/Config.in.host > index 7edf45850d..a213ee94bf 100644 > --- a/package/go/Config.in.host > +++ b/package/go/Config.in.host > @@ -32,6 +32,38 @@ config BR2_PACKAGE_HOST_GO_HOST_ARCH_SUPPORTS > default y > depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_STAGE3_ARCH_SUPPORTS > > +config BR2_PACKAGE_HOST_GO So now, all go packages are broken, because none selects BR2_PACKAGE_HOST_GO, which is required for the virtual-paclage infra. To "fix" that issue, there are two options: - fix all package in this very patch, but it is a bit huge, or - introduce BR2_PACKAGE_HOST_GO first and have packages select it; basically that's your next patch, but it should come first: config BR2_PACKAGE_HOST_GO bool depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_STAGE3_ARCH_SUPPORTS config BR2_PACKAGE_BALENA_ENGINE depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_STAGE3_ARCH_SUPPORTS select BR2_PACKAGE_HOST_GO and then this patch comes in to introduce the host variant. This way, we have a series that is bisectable. Also, I see that you modeled the solution after the rust one, and this is good. However, I don't think there is a need for a prompt for BR2_PACKAGE_HOST_GO. It can very well be a prompt-less symbol, as packages need to select it anyway. Also, there is (I think) no reason to enable BR2_PACKAGE_HOST_GO when no package needs it. Post-build/fakeroot/image scripts should not need the go compiler either, so I don;t think there is an actual need for a prompt... [--SNIP--] > diff --git a/package/go/go/go.mk b/package/go/go-src/go-src.mk > similarity index 68% > rename from package/go/go/go.mk > rename to package/go/go-src/go-src.mk > index c3c40922f4..9d73c7c692 100644 > --- a/package/go/go/go.mk > +++ b/package/go/go-src/go-src.mk > @@ -1,21 +1,22 @@ > ################################################################################ > # > -# go > +# go-src I haven't reviewed the s/GO/GO_SRC/ transition; that's mostly mechanical, so I don;t expect too much surprise. Except one point:if any of those symbol is used elsewhere, they need to be fixed as well... But unlike rust, there does not seem like there are packages that build go stuff as part of another ecosystem (à-la python+rust). Hmm... HOST_GO_COMMON_ENV is used in the pkg-golang infra, but I could not spot anything else on a cursory look. "It should be fine!" Regards, Yann E. MORIN.
Hi all, On Fri, Apr 12, 2024 at 1:01 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > Thanks for this series, it really looks good! :-) I agree that it looks good & having the go-bootstrap below the go/ package is nice. Haven't fully reviewed it yet but will try it. > I anyway have a few comments and questions, please see below. > > Not related to this patch, but still relevant to the discussion: it > would be even better if we could also have the possibility that the > stage-3 has the option of being a prebuilt go. Using stage-3 as pre-built and host-go itself as compiled from source using the pre-built toolchain as the bootstrap one is ideal. This is so that we can control which version of Go is used for the build for reproducibility. Otherwise if the major version of Go installed on the host machine is different, it might produce different results, particularly when using cgo. Best regards, Christian Stewart
Yann, Christian, Thanks for your comments I will soon send a V2 of the patch. > Not related to this patch, but still relevant to the discussion: it > would be even better if we could also have the possibility that the > stage-3 has the option of being a prebuilt go. > > Indeed, it would still be interesting to be able to build the final go > from source, but use a prebuild bootstrap. This is a good suggestion, I will add this to my TODO list and introduce it in a future patch series. > Also, I see that you modeled the solution after the rust one, and this > is good. Do you think it's worth moving rust packages to a subdirectory structure in the future ? Also, another point of the buildroot TODO list is to add support for prebuilt nodejs. If I get time to work on that patch series it will probably follow the same subdir structure. Regards, PERALE Thomas On 4/12/24 10:00 PM, Yann E. MORIN wrote: > Thomas, All, > > Thanks for this series, it really looks good! :-) > > I anyway have a few comments and questions, please see below. > > On 2024-04-12 15:26 +0200, Thomas Perale via buildroot spake thusly: >> Turns host-go into a virtual package, which has two providers: > With this patch, there is a single provider, so the commit log is not > correct. Instead, it should state something like: > > Turn host-go into a virtual package, which currently has a single > provider, the existing host-go package. In later commits, we'll > add a prebuilt, binary host go compiler as another provider. > >> - host-go-src, which builds host-go from source based on the same logic >> that was previously used in package/go/go/go.mk, now moved to >> package/go/go-src/go-src.mk to remove any ambiguity on the role of >> the package. >> >> - host-go-bin, which will be introduced in the next commit and installs >> a pre-built Go compiler. >> >> The usage of a virtual package enables support for pre-built Go > s/usage/use/ > >> compiler, which reduces the build time for of host-go. >> A similar solution is proposed for host-rust. >> >> By default, host-go is built from sources to keep the same behavior as >> the former version. > Since this commit only introduces a single provider, this comment does > not make sense, and belongs to the commit that adds the prebuilt go. > > Not related to this patch, but still relevant to the discussion: it > would be even better if we could also have the possibility that the > stage-3 has the option of being a prebuilt go. > > Indeed, it would still be interesting to be able to build the final go > from source, but use a prebuild bootstrap. > > So that would allow for three situations: > > - full source: stage-1, stage-2, stage-3, and final go, all built from > sources; > > - prebuilt stage-3 (so not stage-1 and no stage-2), but final built > from sources; > > - no bootstrap at all, and prebuilt final go. > > Of course, this is _not_ required for this series to progress. > >> Signed-off-by: Thomas Perale <thomas.perale@mind.be> >> --- >> diff --git a/package/go/Config.in.host b/package/go/Config.in.host >> index 7edf45850d..a213ee94bf 100644 >> --- a/package/go/Config.in.host >> +++ b/package/go/Config.in.host >> @@ -32,6 +32,38 @@ config BR2_PACKAGE_HOST_GO_HOST_ARCH_SUPPORTS >> default y >> depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_STAGE3_ARCH_SUPPORTS >> >> +config BR2_PACKAGE_HOST_GO > So now, all go packages are broken, because none selects > BR2_PACKAGE_HOST_GO, which is required for the virtual-paclage infra. > > To "fix" that issue, there are two options: > > - fix all package in this very patch, but it is a bit huge, or > > - introduce BR2_PACKAGE_HOST_GO first and have packages select it; > basically that's your next patch, but it should come first: > > config BR2_PACKAGE_HOST_GO > bool > depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_STAGE3_ARCH_SUPPORTS > > config BR2_PACKAGE_BALENA_ENGINE > depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_STAGE3_ARCH_SUPPORTS > select BR2_PACKAGE_HOST_GO > > and then this patch comes in to introduce the host variant. > > This way, we have a series that is bisectable. > > Also, I see that you modeled the solution after the rust one, and this > is good. > > However, I don't think there is a need for a prompt for > BR2_PACKAGE_HOST_GO. It can very well be a prompt-less symbol, as > packages need to select it anyway. > > Also, there is (I think) no reason to enable BR2_PACKAGE_HOST_GO when no > package needs it. Post-build/fakeroot/image scripts should not need the > go compiler either, so I don;t think there is an actual need for a > prompt... > > [--SNIP--] >> diff --git a/package/go/go/go.mk b/package/go/go-src/go-src.mk >> similarity index 68% >> rename from package/go/go/go.mk >> rename to package/go/go-src/go-src.mk >> index c3c40922f4..9d73c7c692 100644 >> --- a/package/go/go/go.mk >> +++ b/package/go/go-src/go-src.mk >> @@ -1,21 +1,22 @@ >> ################################################################################ >> # >> -# go >> +# go-src > I haven't reviewed the s/GO/GO_SRC/ transition; that's mostly > mechanical, so I don;t expect too much surprise. > > Except one point:if any of those symbol is used elsewhere, they need to > be fixed as well... But unlike rust, there does not seem like there are > packages that build go stuff as part of another ecosystem (à-la > python+rust). > > Hmm... HOST_GO_COMMON_ENV is used in the pkg-golang infra, but I could > not spot anything else on a cursory look. "It should be fine!" > > Regards, > Yann E. MORIN. >
On 14/04/2024 08:28, Christian Stewart via buildroot wrote: > Hi all, > > On Fri, Apr 12, 2024 at 1:01 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: >> Thanks for this series, it really looks good! :-) > > I agree that it looks good & having the go-bootstrap below the go/ > package is nice. > > Haven't fully reviewed it yet but will try it. > >> I anyway have a few comments and questions, please see below. >> > >> Not related to this patch, but still relevant to the discussion: it >> would be even better if we could also have the possibility that the >> stage-3 has the option of being a prebuilt go. > > Using stage-3 as pre-built and host-go itself as compiled from source > using the pre-built toolchain as the bootstrap one is ideal. > > This is so that we can control which version of Go is used for the > build for reproducibility. > > Otherwise if the major version of Go installed on the host machine is > different, it might produce different results, particularly when using > cgo. The "system" go, i.e. the one installed on the build machine, is never used. This patch series gives the choice to either *download* the go binary, or build it from source. I don't think we want to have the option to use the system go in Buildroot. We do have that for things like cmake and gzip, but it's a bit painful to maintain. We also have it for python3, but only for a very small subset of packages (none of them Python packages). And since a prebuilt binary of go is anyway available for almost all platforms supported by Go, there is absolutely no reason not to just download it to be sure. Regards, Arnout > > Best regards, > Christian Stewart > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
Yann, On Sun, Apr 28, 2024, 10:29 AM Arnout Vandecappelle <arnout@mind.be> wrote: > > > On 14/04/2024 08:28, Christian Stewart via buildroot wrote: > > Hi all, > > > > On Fri, Apr 12, 2024 at 1:01 PM Yann E. MORIN <yann.morin.1998@free.fr> > wrote: > >> Thanks for this series, it really looks good! :-) > > > > I agree that it looks good & having the go-bootstrap below the go/ > > package is nice. > > > > Haven't fully reviewed it yet but will try it. > > > >> I anyway have a few comments and questions, please see below. > >> > > > >> Not related to this patch, but still relevant to the discussion: it > >> would be even better if we could also have the possibility that the > >> stage-3 has the option of being a prebuilt go. > > > > Using stage-3 as pre-built and host-go itself as compiled from source > > using the pre-built toolchain as the bootstrap one is ideal. > > > > This is so that we can control which version of Go is used for the > > build for reproducibility. > > > > Otherwise if the major version of Go installed on the host machine is > > different, it might produce different results, particularly when using > > cgo. > > The "system" go, i.e. the one installed on the build machine, is never > used. > This patch series gives the choice to either *download* the go binary, or > build > it from source. > > I don't think we want to have the option to use the system go in > Buildroot. We > do have that for things like cmake and gzip, but it's a bit painful to > maintain. > We also have it for python3, but only for a very small subset of packages > (none > of them Python packages). And since a prebuilt binary of go is anyway > available > for almost all platforms supported by Go, there is absolutely no reason > not to > just download it to be sure. > Using the host go compiler on the system to build host-go is absolutely viable (I already use it all the time in my fork). It's better than just downloading a pre-built go for multiple reasons: namely, when doing a full from-source build, and I almost always prefer building the compiler myself instead of trusting binaries downloaded over the internet. It's not hard to add, I already have a patch on the mailing list for it and it's 5 lines of changes. Thanks, Christian
diff --git a/.checkpackageignore b/.checkpackageignore index b3eab26071..69681bfeaa 100644 --- a/.checkpackageignore +++ b/.checkpackageignore @@ -465,7 +465,7 @@ package/glorytun/0002-aegis256.c-fix-aarch64-build-with-uclibc.patch lib_patch.U package/gnu-efi/0001-Make.defaults-don-t-override-ARCH-when-cross-compili.patch lib_patch.Upstream package/gnupg/0001-build-Always-use-EXTERN_UNLESS_MAIN_MODULE-pattern.patch lib_patch.Upstream package/gnuplot/0001-configure-add-without-demo-option.patch lib_patch.Upstream -package/go/go/0001-build.go-explicit-option-for-crosscompilation.patch lib_patch.Upstream +package/go/go-src/0001-build.go-explicit-option-for-crosscompilation.patch lib_patch.Upstream package/gob2/0001-dont-include-from-prefix.patch lib_patch.Upstream package/gobject-introspection/0001-disable-tests.patch lib_patch.Upstream package/gobject-introspection/0002-Add-rpath-links-to-ccompiler.patch lib_patch.Upstream diff --git a/package/go/Config.in.host b/package/go/Config.in.host index 7edf45850d..a213ee94bf 100644 --- a/package/go/Config.in.host +++ b/package/go/Config.in.host @@ -32,6 +32,38 @@ config BR2_PACKAGE_HOST_GO_HOST_ARCH_SUPPORTS default y depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_STAGE3_ARCH_SUPPORTS +config BR2_PACKAGE_HOST_GO + bool "host go compiler" + depends on BR2_PACKAGE_HOST_GO_HOST_ARCH_SUPPORTS + help + Compiler for the Go language + + https://go.dev + +if BR2_PACKAGE_HOST_GO + +choice + prompt "Go compiler variant" + default BR2_PACKAGE_HOST_GO_SRC + help + Select a Go compiler variant. + + Default to 'host-go-src'. + +config BR2_PACKAGE_HOST_GO_SRC + bool "host go (source)" + help + This package will build the go compiler for the host. + +endchoice + +config BR2_PACKAGE_PROVIDES_HOST_GO + string + # Default to host-go-src + default "host-go-src" if BR2_PACKAGE_HOST_GO_SRC + +endif + source "package/go/go-bootstrap-stage1/Config.in.host" source "package/go/go-bootstrap-stage2/Config.in.host" source "package/go/go-bootstrap-stage3/Config.in.host" diff --git a/package/go/go/0001-build.go-explicit-option-for-crosscompilation.patch b/package/go/go-src/0001-build.go-explicit-option-for-crosscompilation.patch similarity index 100% rename from package/go/go/0001-build.go-explicit-option-for-crosscompilation.patch rename to package/go/go-src/0001-build.go-explicit-option-for-crosscompilation.patch diff --git a/package/go/go/0002-cmd-dist-set-buildvcs-false-when-building-go-bootstr.patch b/package/go/go-src/0002-cmd-dist-set-buildvcs-false-when-building-go-bootstr.patch similarity index 100% rename from package/go/go/0002-cmd-dist-set-buildvcs-false-when-building-go-bootstr.patch rename to package/go/go-src/0002-cmd-dist-set-buildvcs-false-when-building-go-bootstr.patch diff --git a/package/go/go/go.hash b/package/go/go-src/go-src.hash similarity index 100% rename from package/go/go/go.hash rename to package/go/go-src/go-src.hash diff --git a/package/go/go/go.mk b/package/go/go-src/go-src.mk similarity index 68% rename from package/go/go/go.mk rename to package/go/go-src/go-src.mk index c3c40922f4..9d73c7c692 100644 --- a/package/go/go/go.mk +++ b/package/go/go-src/go-src.mk @@ -1,21 +1,22 @@ ################################################################################ # -# go +# go-src # ################################################################################ -GO_SITE = https://storage.googleapis.com/golang -GO_SOURCE = go$(GO_VERSION).src.tar.gz +GO_SRC_SITE = https://storage.googleapis.com/golang +GO_SRC_SOURCE = go$(GO_VERSION).src.tar.gz -GO_LICENSE = BSD-3-Clause -GO_LICENSE_FILES = LICENSE -GO_CPE_ID_VENDOR = golang +GO_SRC_LICENSE = BSD-3-Clause +GO_SRC_LICENSE_FILES = LICENSE +GO_SRC_CPE_ID_VENDOR = golang -HOST_GO_DEPENDENCIES = host-go-bootstrap-stage3 +HOST_GO_SRC_PROVIDES = host-go +HOST_GO_SRC_DEPENDENCIES = host-go-bootstrap-stage3 ifeq ($(BR2_PACKAGE_HOST_GO_TARGET_ARCH_SUPPORTS),y) -HOST_GO_CROSS_ENV = \ +HOST_GO_SRC_CROSS_ENV = \ CC_FOR_TARGET="$(TARGET_CC)" \ CXX_FOR_TARGET="$(TARGET_CXX)" \ GOOS="linux" \ @@ -28,7 +29,7 @@ endif # The go build system is not compatible with ccache, so use # HOSTCC_NOCCACHE. See https://github.com/golang/go/issues/11685. -HOST_GO_MAKE_ENV = \ +HOST_GO_SRC_MAKE_ENV = \ GO111MODULE=off \ GOCACHE=$(HOST_GO_HOST_CACHE) \ GOROOT_BOOTSTRAP=$(HOST_GO_BOOTSTRAP_STAGE3_ROOT) \ @@ -41,12 +42,12 @@ HOST_GO_MAKE_ENV = \ CGO_ENABLED=$(HOST_GO_CGO_ENABLED) \ $(HOST_GO_CROSS_ENV) -define HOST_GO_BUILD_CMDS +define HOST_GO_SRC_BUILD_CMDS cd $(@D)/src && \ - $(HOST_GO_MAKE_ENV) ./make.bash $(if $(VERBOSE),-v) + $(HOST_GO_SRC_MAKE_ENV) ./make.bash $(if $(VERBOSE),-v) endef -define HOST_GO_INSTALL_CMDS +define HOST_GO_SRC_INSTALL_CMDS $(GO_BINARIES_INSTALL) endef diff --git a/package/go/go.mk b/package/go/go.mk index 104a4a73fb..21cdb29f8f 100644 --- a/package/go/go.mk +++ b/package/go/go.mk @@ -129,4 +129,6 @@ define GO_BINARIES_INSTALL find $(HOST_GO_ROOT) -type f -exec touch -r $(@D)/bin/go {} \; endef +$(eval $(host-virtual-package)) + include $(sort $(wildcard package/go/*/*.mk))
Turns host-go into a virtual package, which has two providers: - host-go-src, which builds host-go from source based on the same logic that was previously used in package/go/go/go.mk, now moved to package/go/go-src/go-src.mk to remove any ambiguity on the role of the package. - host-go-bin, which will be introduced in the next commit and installs a pre-built Go compiler. The usage of a virtual package enables support for pre-built Go compiler, which reduces the build time for of host-go. A similar solution is proposed for host-rust. By default, host-go is built from sources to keep the same behavior as the former version. Signed-off-by: Thomas Perale <thomas.perale@mind.be> --- .checkpackageignore | 2 +- package/go/Config.in.host | 32 +++++++++++++++++++ ...explicit-option-for-crosscompilation.patch | 0 ...ldvcs-false-when-building-go-bootstr.patch | 0 package/go/{go/go.hash => go-src/go-src.hash} | 0 package/go/{go/go.mk => go-src/go-src.mk} | 25 ++++++++------- package/go/go.mk | 2 ++ 7 files changed, 48 insertions(+), 13 deletions(-) rename package/go/{go => go-src}/0001-build.go-explicit-option-for-crosscompilation.patch (100%) rename package/go/{go => go-src}/0002-cmd-dist-set-buildvcs-false-when-building-go-bootstr.patch (100%) rename package/go/{go/go.hash => go-src/go-src.hash} (100%) rename package/go/{go/go.mk => go-src/go-src.mk} (68%)