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

[Robin H. Johnson]

Stay tuned for v2 with major improvements based on talking to upstream.
- Dropped an entire distfile per golang module (down to 1-2 files per
  module in go.sum)
- Better Go 1.13 support (some semantics changed slightly from 1.12)
- Easier way to track & include licenses of all the modules

On Thu, Feb 13, 2020 at 05:57:57PM +0100, Michał Górny wrote:
> > +# EGO_SUM=(
> > +#	"github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ="
> > +#	"github.com/BurntSushi/toml v0.3.1/go.mod h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ="
> Is it expected that the two entries would have the same hash?
In this case, they SHOULD have been different, but it does happen in
reality for the /go.mod entries, here's an example:
github.com/stretchr/testify v1.2.1/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=

1.2.1->1.2.2 there was no change in the dependencies, so the file didn't change.

> > -EXPORT_FUNCTIONS src_unpack pkg_postinst
> > +EXPORT_FUNCTIONS src_unpack src_prepare pkg_postinst
> Exporting a new phase looks potentially dangerous.  Are you sure no
> ebuilds are broken by this?
I'm not sure, so I've rolled it into src_unpack for now.
>
> > +
> > +# @ECLASS-VARIABLE: EGO_SUM
> > +# @DESCRIPTION:
> > +# This variable duplicates the go.sum content from inside the target package.
> > +# Entries of the form <version>/go.mod should be excluded.
> ...but you've included one of them in the example on top of the eclass.
There was an ongoing discussion with upstream, where I was trying to
trim down the number of distfiles involved. Sadly generating the .mod
files turns out to have some non-trivial corner cases

I did manage to drop the .info files at least, so it's 1-2 distfiles per
dependency now.

>
> > +#
> > +# <module> <version> <hash>
>
> Now I'm confused.  Unless my eyes betray me, PATCH 2 has entries without
> hash.
>
> Also, the description fails to mention that you're supposed to quote
> each line.
Improved in documentation, and covered why the hash is optional right
now.

> > +# The format is described upstream here:
> > +# https://tip.golang.org/cmd/go/#hdr-Module_authentication_using_go_sum
...
> I think it would be valuable to include an example here as well.
Done.
> > +# proxy generally verifies modules via the Hash1 code.
> > +#
> > +# Note: Users in China may find some mirrors in the list blocked, and may wish
> > +# to an explicit entry to /etc/portage/mirrors pointing mirror://goproxy/ to
> > +# https://goproxy.cn/, or change this variable.
> > +# See https://arslan.io/2019/08/02/why-you-should-use-a-go-module-proxy/ for further details
> > +: "${GOMODULE_GOPROXY_BASEURI:=mirror://goproxy/}"
> 'Changing this variable' sounds like violating metadata immutability
> rule and running in trouble with the caches.
Covered who & why this should be set, esp wrt to immutability.

> > +
> > +# @FUNCTION: go-module_set_globals
> > +# @DESCRIPTION:
> > +# Convert the information in EGO_SUM for other usage in the ebuild.
> > +# - Populates EGO_SUM_SRC_URI that can be added to SRC_URI
> > +# - Exports _EGO_SUM_MAPPING which provides reverse mapping from distfile back
> > +#   to the relative part of SRC_URI, as needed for GOPROXY=file:///...
> > +go-module_set_globals() {
> > +	local line error_in_gosum errorlines errormsg exts
> > +	local newline=$'\n'
> > +	error_in_gosum=0
> > +	errorlines=( )
> > +	for line in "${EGO_SUM[@]}"; do
> > +		local module version modfile version_modfile hash1 x
> > +		read -r module version_modfile hash1 x <<< "${line}"
> > +		# Validate input
> > +		if [[ -n $hash1 ]] && [[ ${hash1:0:3} != "h1:" ]] ; then
>
> Please use ${foo} everywhere consistently, and put && inside [[ ]].
> Also, I dare say wildcard match is more readable than hardcoding string
> length, i.e.:
>
>   [[ -n ${hash1} && ${hash1} != h1:* ]]
...

> > +		# Split 'v0.3.0/go.mod' into 'v0.3.0' and '/go.mod'
> > +		version=${version_modfile%%/*}
> > +		modfile=${version_modfile#*/}
> > +		[[ "$modfile" == "${version_modfile}" ]] && modfile=
> Check the initial string, not the result of arbitrary manipulations
> on it.  This would wrongly evaluate true for 'v0.3.0/v0.3.0'.
Reworked this

> > +go-module_src_unpack() {
> > +	if [[ "${#EGO_VENDOR[@]}" -gt 0 ]]; then
> > +		_go-module_src_unpack_vendor
> > +	elif [[ "${#EGO_SUM[@]}" -gt 0 ]]; then
> > +		_go-module_src_unpack_gosum
> Does that mean those two are mutually exclusive?
Yes.

> > +# @FUNCTION: go-module_src_prepare
...
> Wouldn't it be better to append this to src_unpack?  Overriding
> src_prepare is generally problematic, and as I've said above, you're
> already risking by adding a new export.
Moved it.

> > +# @ECLASS-VARIABLE: GOMODULE_GOSUM_PATH
> > +# @DESCRIPTION:
> > +# Path to root go.sum of package. If your ebuild modifies S after inheriting
> > +# the eclass, you may need to update this variable.
> > +: "${GO_MODULE_GOSUM_PATH:=${S}/go.sum}"
> Wouldn't it be cleaner to have the path relative to ${S} by default?
Variable is no longer needed.

> > +# @FUNCTION: _go-module_src_unpack_gosum
...
> > +	declare -A _EGO_SUM_MAPPING_ASSOC
> Why not make it local?

Done.

> > +	# go.sum entries ending in /go.mod aren't strictly needed at this phase
...
> Why not create and restore a copy?  Or does go-get make other changes?
The minimize is dropped entirely per upstream discussions.

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