Re: [Buildroot] [PATCH v3 1/3] package/go-bootstrap: split into two stages: go1.4 and go1.19.5

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

On Thu, 23 Mar 2023 19:45:04 -0700
Christian Stewart <christian@paral.in> wrote:

> > This doesn't make much sense. Why would the toolchain package be needed
> > for HOSTCC_NOCCACHE ? We have several packages that use HOSTCC_NOCCACHE
> > before the toolchain is ready.  
> 
> I suppose HOSTCC_NOCCACHE uses the gcc installed outside of buildroot,
> so it should be fine to remove this dependency.

Yes, the native toolchain must always be provided by the system, it is
not provided by Buildroot. The "toolchain" package only provides the
cross-compilation toolchain, it does nothing about the native toolchain.

> > > +     # Set all file timestamps to prevent the go compiler from rebuilding any
> > > +     # built in packages when programs are built.
> > > +     find $(HOST_GO_BOOTSTRAP_STAGE2_ROOT) -type f -exec touch -r $(@D)/bin/go {} \;  
> >
> > So we have to do this for bootstrap-stage2 but not bootstrap-stage1 ?  
> 
> It's not strictly necessary for either one. I removed it for the next
> revision of the patch.

But your comment above seemed to indicate that it was necessary.

> > > -     depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_ARCH_SUPPORTS
> > > +     depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_STAGE2_ARCH_SUPPORTS
> > > +     # See https://go.dev/doc/install/source#environment
> > > +     # See src/go/build/syslist.go for the list of supported architectures  
> >
> > This comment looks good, but is unrelated. Separate patch?  
> 
> Is it really necessary to put in a separate patch? It's a minor comment change.
> 
> Feels appropriate to bundle it here with other related changes.

Well, it always makes the review a bit more complicated, as we wonder
why it's there. If you really want to bundle unrelated changes like
this, they should at least be mentioned in the commit log so that the
reviewer knows what's going on.


> > > -# The go build system is not compatible with ccache, so use
> > > -# HOSTCC_NOCCACHE.  See https://github.com/golang/go/issues/11685.
> > > +# The go build system is not compatable with ccache, so use HOSTCC_NOCCACHE.
> > > +# See https://github.com/golang/go/issues/11685.  
> >
> > Why is this comment being changed, with a typo added?  
> 
> Fixed the typo. The comment is being changed because it can fit on a
> single line in 75 characters.
> 
> It just looks cleaner.
> 
> I can revert this if necessary.

Same as above: it's creating some unrelated "noise" in the patch, which
makes things odd when doing a review.

> 
> > >  HOST_GO_MAKE_ENV = \
> > >       GO111MODULE=off \
> > >       GOCACHE=$(HOST_GO_HOST_CACHE) \
> > > -     GOROOT_BOOTSTRAP=$(HOST_GO_BOOTSTRAP_ROOT) \
> > > +     GOROOT_BOOTSTRAP=$(HOST_GO_BOOTSTRAP_STAGE2_ROOT) \
> > >       GOROOT_FINAL=$(HOST_GO_ROOT) \
> > >       GOROOT="$(@D)" \
> > >       GOBIN="$(@D)/bin" \
> > > @@ -154,7 +154,6 @@ define HOST_GO_INSTALL_CMDS
> > >       cp -a $(@D)/pkg/include $(@D)/pkg/linux_* $(HOST_GO_ROOT)/pkg/
> > >       cp -a $(@D)/pkg/tool $(HOST_GO_ROOT)/pkg/
> > >
> > > -     # There is a known issue which requires the go sources to be installed  
> >
> > Why is this comment being removed?  
> 
> It's not an issue, per se. The nature of the Go compiler is that it
> needs the sources to work.
> That bug report has been closed for years now and is actually
> unrelated to needing sources anyway.

So perhaps rephrase to explain "Yes the Go compiler needs the sources
to work", rather than just dropping the comment? But here again, it's
unrelated to the patch we're discussing so => separate patch.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot