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

[Robin H. Johnson]

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.

>
> > +# 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

> > +# @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?

> > +		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.

> > +_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.

>> ...
> If EGO_SUM is up to date, this could also mean that upstream forgot to
> run go mod tidy and commit the go.mod/go.sum before the release, so it
> could be a bug that needs to be reported.
Yes. I found at least one package where the upstream had failed to run
go mod tidy in the release: I already reported it, and they say they
will include it in the next release, not doing a release just for it.

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@g.o
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136