Re: [gentoo-dev] [PATCH v2 1/4] eclass/go-module: add support for building based on go.sum

[William Hubbs]

On Wed, Feb 19, 2020 at 07:36:27AM +0000, Robin H. Johnson wrote:
> On Tue, Feb 18, 2020 at 11:46:45PM -0600, William Hubbs wrote:
> > > -# If it does not have a vendor directory, you should use the EGO_VENDOR
> > > +# Alternatively, older versions of this eclass used the EGO_VENDOR
> > >  # variable and the go-module_vendor_uris function as shown in the
> > >  # example below to handle dependencies.
> > I think we can remove the example with EGO_VENDOR and
> > go-module_vendor_uris; we really don't want people to continue following
> > that example.
> I tried to handle more cases here, but now I'm wondering if it would be
> cleaner just to put all of new way into a distinct eclass, and sunset
> the old eclass entirely. I found unforeseen interactions, see below.
>
> > > +# S="${WORKDIR}/${MY_P}"
> > The default setting of S should be fine for most ebuilds, so I don't
> > think we need this in the example.
> I'd copied it, but yes in this case.
>
> >
> > > +# go-module_set_globals
> > > +#
> > > +# SRC_URI="https://github.com/example/${PN}/archive/v${PV}.tar.gz -> ${P}.tar.gz
> > > +#		   ${EGO_SUM_SRC_URI}"
> > > +#
> > > +# LICENSE="some-license ${EGO_SUM_LICENSES}"
> > > +#
> > > +# src_unpack() {
> > > +# 	unpack ${P}.tar.gz
> > > +#	go-module_src_unpack
> > > +# }
> >  I don't think I would put an src_unpack() in the example.
> This is one of the unforeseen interactions.
> The go.sum unpack only applies special handling to distfiles that it
> knows about. It does NOT process any other distfiles at all.
>
> EAPI8 or future Portage improvements might have annotations to disable
> any automatic unpacking for specific distfiles, which would resolve this
> issue.
>
> Hence, you need to explicitly unpack any distfiles that are NOT part of
> the go.sum dependencies. There are some ebuilds that do unpack & rename
> in src_unpack already, so they need extra care as well.
>
> The EGO_VENDOR src_unpack unpacked EVERYTHING, so it didn't have this
> issue.

I will look at that in a bit and comment on it.

>
> >
> > > +# The extra metadata keys accepted at this time are:
> > > +# - license: for dependencies built into the final runtime, the value field is
> > > +#   a comma seperated list of Gentoo licenses to apply to the LICENSE variable,
> > > +#
> > There are two lines for each module in go.sum, the one with /go.mod as a
> > suffix to the version and the one without. We do not need both right?
> This is not entirely correct, and it's the warnings from golang upstream
> about some hidden complexity in the /go.mod that lead me to list both of
> them.
>
> If we intend to verify the h1: in future, then we need to list all
> /go.mod entries explicitly, so have somewhere to put the h1: hash.
> If you're verifying the h1: hash, you must verify it on the
> {version}.mod ALWAYS, and if the {version}.zip is present, then also on
> that file (otherwise it could sneak in some evil metadata via the
> {version}.mod).
>
> If we omit h1: entirely, then we can get away with listing ONE line in
> EGO_SUM for each dependency.
> - If it contains /go.mod, it will fetch ONLY the {version}.mod file.
> - If it does not contain /go.mod, it will fetch the {version}.mod file
>   AND the {version}.zip file
 
If Go itself does that verification during the build, do we need to do
it also?

> > > +# @EXAMPLE:
> > > +# # github.com/BurntSushi/toml is a build-time only dep
> > > +# # github.com/aybabtme/rgbterm is a runtime dep, annotated with licenses
> >
> > I'm not sure we can distinguish between buildtime only and runtime deps.
> The 'golicense' tool will take a Golang binary and print out all of the
> Golang modules that got linked into it. I consider those to be the
> runtime deps in this case.
>
> > > +# @ECLASS-VARIABLE: _GOMODULE_GOPROXY_BASEURI
> ...
> > > +# This variable should NOT be present in user-level configuration e.g.
> > > +# /etc/portage/make.conf, as it will violate metadata immutability!
> > > +: "${_GOMODULE_GOPROXY_BASEURI:=mirror://goproxy/}"
> >
> > If this isn't supposed to be in user-level configuration, where should
> > it be set?
> Maybe I'll just prefix it with 'readonly' and force the value for now.
>
> > >  # @FUNCTION: go-module_src_unpack
> > >  # @DESCRIPTION:
> > > +# Extract & configure Go modules for consumpations.
> > > +# - Modules listed in EGO_SUM are configured as a local GOPROXY via symlinks (fast!)
> > > +# - Modules listed in EGO_VENDOR are extracted to "${S}/vendor" (slow)
> > > +#
> > > +# This function does NOT unpack the base distfile of a Go-based package.
> > > +# While the entries in EGO_SUM will be listed in ${A}, they should NOT be
> > > +# unpacked, Go will directly consume the files, including zips.
> > > +go-module_src_unpack() {
> >
> > If possible, this function should unpack the base distfile. That would
> > keep us from having to write src_unpack for every go ebuild that uses
> > the eclass.
> That's fine until we get to multiple base distfiles and handling them.
> Maybe pass a flag to go-module_src_unpack to tell it not to unpack any
> distfile that it does not explicitly know about?

Maybe, but again I'll think about this more.

> > > +		die "Neither EGO_SUM nor EGO_VENDOR are set!"
> > This shouldn't die, having neither one set is valid.
> Yes, I caught this in later testing: a Golang package in the tree that
> inherit go-module, but didn't use EGO_VENDOR, EGO_SUM or have a vendor
> directory! This is another reason why I think bumping the eclass to a
> new name would be safer now.

Which ebuild was that? this is actually an invalid case for go-module.
A go module requires the presence of go.mod and optionally go.sum (if
there are no external deps) and optionally vendor/.

If you want an invalid case, you would have to be able to test for
something like:

[[ ! -f "${S}"/go.mod ]] && die "..."

>
> > > +_go-module_src_unpack_vendor() {
> > > +	# shellcheck disable=SC2120
> > > +	debug-print-function "${FUNCNAME}" "$@"
> > Maybe add an eqawarn here that EGO_VENDOR is deprecated to encourage
> > people to migrate their ebuilds.
> Omitting this based on my feelings about a new eclass now.

I'm still not convinced we need a whole new eclass.

William