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

[Michał Górny]

On Sun, 2020-02-09 at 12:31 -0800, Robin H. Johnson wrote:
> EGO_SUM mode now supplements the existing EGO_VENDOR mode.
>
> EGO_SUM should be populated by the maintainer, directly from the go.sum
> file of the root package. See eclass and conversion example
> (dev-go/go-tour & app-admin/kube-bench) for further details.
>
> The go-module_set_globals function performs validation of
> inputs and does die on fatal errors.
>
> Signed-off-by: Robin H. Johnson <robbat2@g.o>
> ---
>  eclass/go-module.eclass    | 328 +++++++++++++++++++++++++++++++++++--
>  profiles/thirdpartymirrors |   1 +
>  2 files changed, 311 insertions(+), 18 deletions(-)
>
> diff --git eclass/go-module.eclass eclass/go-module.eclass
> index d5de5f60ccdf..b8a635d52de7 100644
> --- eclass/go-module.eclass
> +++ eclass/go-module.eclass
> @@ -4,22 +4,46 @@
>  # @ECLASS: go-module.eclass
>  # @MAINTAINER:
>  # William Hubbs <williamh@g.o>
> +# @AUTHOR:
> +# William Hubbs <williamh@g.o>
> +# Robin H. Johnson <robbat2@g.o>
>  # @SUPPORTED_EAPIS: 7
>  # @BLURB: basic eclass for building software written as go modules
>  # @DESCRIPTION:
> -# This eclass provides basic settings and functions
> -# needed by all software written in the go programming language that uses
> -# go modules.
> +# This eclass provides basic settings and functions needed by all software
> +# written in the go programming language that uses go modules.
> +#
> +# You might know the software you are packaging uses modules because
> +# it has files named go.sum and go.mod in its top-level source directory.
> +# If it does not have these files, try use the golang-* eclasses FIRST!
> +# There ARE legacy Golang packages that use external modules with none of
> +# go.mod, go.sum, vendor/ that can use this eclass regardless.
> +#
> +# Guidelines for usage:
> +# "go.mod" && "go.sum" && "vendor/":
> +# - pre-vendored package. Do NOT set EGO_SUM or EGO_VENDOR.
> +#
> +# "go.mod" && "go.sum":
> +# - Populate EGO_SUM with entries from go.sum
> +# - Do NOT include any lines that contain <version>/go.mod
> +#
> +# "go.mod" only:
> +# - Populate EGO_VENDOR
>  #
> -# You will know the software you are packaging uses modules because
> -# it will have files named go.sum and go.mod in its top-level source
> -# directory. If it does not have these files, use the golang-* eclasses.
> +# None of the above:
> +# - Did you try golang-* eclasses first? Upstream has undeclared dependencies
> +#   (perhaps really old source). You can use either EGO_SUM or EGO_VENDOR.
> +
>  #
> -# If it has these files and a directory named vendor in its top-level
> -# source directory, you only need to inherit the eclass since upstream
> -# is vendoring the dependencies.
> +# If it has these files AND a directory named "vendor" in its top-level source
> +# directory, you only need to inherit the eclass since upstream has already
> +# vendored the dependencies.
> +
> +# If it does not have a vendor directory, you should use the EGO_SUM
> +# variable and the go-module_gosum_uris function as shown in the
> +# example below to handle dependencies.
>  #
> -# 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.
>  #
> @@ -28,6 +52,21 @@
>  # dependencies. So please make sure it is accurate.
>  #
>  # @EXAMPLE:
> +# @CODE
> +#
> +# inherit go-module
> +#
> +# 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?

> +# )
> +# S="${WORKDIR}/${MY_P}"
> +# go-module_set_globals
> +#
> +# SRC_URI="https://github.com/example/${PN}/archive/v${PV}.tar.gz -> ${P}.tar.gz
> +# ${EGO_SUM_SRC_URI}"
> +#
> +# @CODE
>  #
>  # @CODE
>  #
> @@ -35,7 +74,7 @@
>  #
>  # EGO_VENDOR=(
>  #	"github.com/xenolf/lego 6cac0ea7d8b28c889f709ec7fa92e92b82f490dd"
> -# "golang.org/x/crypto 453249f01cfeb54c3d549ddb75ff152ca243f9d8 github.com/golang/crypto"
> +#	"golang.org/x/crypto 453249f01cfeb54c3d549ddb75ff152ca243f9d8 github.com/golang/crypto"
>  # )
>  #
>  # SRC_URI="https://github.com/example/${PN}/archive/v${PV}.tar.gz -> ${P}.tar.gz
> @@ -64,10 +103,12 @@ export GO111MODULE=on
>  export GOCACHE="${T}/go-build"
>
>  # The following go flags should be used for all builds.
> -# -mod=vendor stopps downloading of dependencies from the internet.
>  # -v prints the names of packages as they are compiled
>  # -x prints commands as they are executed
> -export GOFLAGS="-mod=vendor -v -x"
> +# -mod=vendor use the vendor directory instead of downloading dependencies
> +# -mod=readonly do not update go.mod/go.sum but fail if updates are needed
> +export GOFLAGS="-v -x -mod=readonly"
> +[[ ${#EGO_VENDOR[@]} -gt 0 ]] && GOFLAGS+=" -mod=vendor"
>
>  # Do not complain about CFLAGS etc since go projects do not use them.
>  QA_FLAGS_IGNORED='.*'
> @@ -75,7 +116,23 @@ QA_FLAGS_IGNORED='.*'
>  # Go packages should not be stripped with strip(1).
>  RESTRICT="strip"
>
> -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?

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

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

> +#
> +# The format is described upstream here:
> +# https://tip.golang.org/cmd/go/#hdr-Module_authentication_using_go_sum
> +#
> +# <hash> is the Hash1 structure used by upstream Go
> +# Note that Hash1 is MORE stable than Gentoo distfile hashing, and upstream
> +# warns that it's conceptually possible for the Hash1 value to remain stable
> +# while the upstream zipfiles change. E.g. it does NOT capture mtime changes in
> +# files within a zipfile.

I think it would be valuable to include an example here as well.

>
>  # @ECLASS-VARIABLE: EGO_VENDOR
>  # @DESCRIPTION:
> @@ -106,13 +163,202 @@ go-module_vendor_uris() {
>  	done
>  }
>
> +# @ECLASS-VARIABLE: GOMODULE_GOPROXY_BASEURI
> +# @DESCRIPTION:
> +# Golagg module proxy service to fetch module files from. Note that the module

Typo: golagg -> golang.

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

> +
> +# @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:* ]]

> +			error_in_gosum=1
> +			errorlines+=( "Unknown hash: ${line}" )
> +		elif [[ -n $x ]]; then
> +			error_in_gosum=1
> +			errorlines+=( "Trailing data: ${line}" )
> +		fi
> +
> +		# 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'.

> +
> +		# The trailing part should be either empty or '/go.mod'
> +		# There is a chance that upstream Go might add something else here in
> +		# future, and we should be prepared to capture it.
> +		exts=()
> +		errormsg=''
> +		case "$modfile" in
> +			'') exts=( mod info zip ) ;;
> +			'go.mod'|'/go.mod') exts=( mod info ) ;;
> +			#'go.mod'|'/go.mod') errormsg="Prohibited file: You must exclude /go.mod lines from EGO_SUM! " ;;

Why is it commented out?

> +			*) errormsg="Unknown modfile: line='${line}', modfile='${modfile}'" ;;
> +		esac
> +
> +		# If it was a bad entry, restart the loop
> +		if [[ -n $errormsg ]]; then
> +			error_in_gosum=1
> +			errorlines+=( "${errormsg} line='${line}', modfile='${modfile}'" )
> +			continue
> +		fi
> +
> +		# Directory structure for Go proxy hosts:
> +		# - def encode(s):
> +		#     return re.sub('([A-Z]{1})', r'!\1', s).lower()
> +		#
> +		# Sed variant:
> +		# This uses GNU Sed extension \l to downcase the match
> +		#_dir=$(echo "${module}" |sed 's,[A-Z],!\l&,g')
> +		#
> +		# Bash variant:
> +		re='(.*)([A-Z])(.*)'
> +		input=${module}
> +		while [[ $input =~ $re ]]; do
> +			lower='!'"${BASH_REMATCH[2],}"
> +			input="${BASH_REMATCH[1]}${lower}${BASH_REMATCH[3]}"
> +		done
> +		_dir=$input
> +		unset lower input re
> +
> +		for _ext in "${exts[@]}" ; do
> +			# Relative URI within a GOPROXY for a file
> +			_reluri="${_dir}/@v/${version}.${_ext}"
> +			# SRC_URI: LHS entry
> +			_uri="${GOMODULE_GOPROXY_BASEURI}/${_reluri}"
> +			# SRC_URI: RHS entry, encode any slash in the path as %2F in the filename
> +			_distfile="${_reluri//\//%2F}"
> +
> +			EGO_SUM_SRC_URI+=" ${_uri} -> ${_distfile}${newline}"
> +			_EGO_SUM_MAPPING+=" ${_distfile}:${_reluri}${newline}"
> +		done
> +	done
> +
> +	if [[ $error_in_gosum != 0 ]]; then
> +		eerror "Trailing information in EGO_SUM in ${P}.ebuild"
> +		for line in "${errorlines[@]}" ; do
> +			eerror "${line}"
> +		done
> +		die "Invalid EGO_SUM format"
> +	fi
> +
> +	# Ensure these variables not not changed past this point
> +	readonly EGO_SUM
> +	readonly EGO_SUM_SRC_URI
> +	readonly _EGO_SUM_MAPPING
> +
> +	# Set the guard that we are safe
> +	_GO_MODULE_SET_GLOBALS_CALLED=1
> +}
> +
> +
>  # @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 [[ "${#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?

> +	else
> +		die "Neither EGO_SUM nor EGO_VENDOR are set!"
> +	fi
> +}
> +
> +# @FUNCTION: go-module_src_prepare
> +# @DESCRIPTION:
> +# Prepare for building. Presently only needed for EGO_SUM variant.
> +go-module_src_prepare() {
> +	# shellcheck disable=SC2120
> +	debug-print-function "${FUNCNAME}" "$@"
> +
> +	if [[ "${#EGO_SUM[@]}" -gt 0 ]]; then
> +		_go-module_src_prepare_gosum
> +	fi

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.

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

> +
> +# @FUNCTION: _go-module_src_unpack_gosum
> +# @DESCRIPTION:
> +# Populate a GOPROXY directory hierarchy with distfiles from EGO_SUM
> +#
> +# Exports GOPROXY environment variable so that Go calls will source the
> +# directory correctly.
> +_go-module_src_unpack_gosum() {
> +	# shellcheck disable=SC2120
> +	debug-print-function "${FUNCNAME}" "$@"
> +
> +	if [[ ! ${_GO_MODULE_SET_GLOBALS_CALLED} ]]; then
> +		die "go-module_set_globals must be called in global scope"
> +	fi
> +
> +	local goproxy_dir="${T}/goproxy"
> +	local goproxy_mod_dir
> +	mkdir -p "${goproxy_dir}"
> +	# Convert the list format to an associative array to avoid O(N*M)
> +	# performance when populating the GOPROXY directory structure.
> +	declare -A _EGO_SUM_MAPPING_ASSOC

Why not make it local?

> +	for s in ${_EGO_SUM_MAPPING}; do
> +		a=${s//:*}
> +		b=${s//*:}
> +		_EGO_SUM_MAPPING_ASSOC["$a"]=$b
> +	done
> +
> +	# For each Golang module distfile, look up where it's supposed to go, and
> +	# symlink into place.
> +	for _A in ${A}; do

This one looks like local candidate as well.

> +		goproxy_mod_path="${_EGO_SUM_MAPPING_ASSOC["${_A}"]}"
> +		if [[ -n "${goproxy_mod_path}" ]]; then
> +			einfo "Populating goproxy for $goproxy_mod_path"
> +			# Build symlink hierarchy
> +			goproxy_mod_dir=$( dirname "${goproxy_dir}"/"${goproxy_mod_path}" )
> +			mkdir -p "${goproxy_mod_dir}"

|| die

> +			ln -sf "${DISTDIR}"/"${_A}" "${goproxy_dir}/${goproxy_mod_path}" || die "Failed to ln"
> +		fi
> +	done
> +	export GOPROXY="file://${goproxy_dir}"
> +	unset _EGO_SUM_MAPPING_ASSOC
> +}
> +
> +# @FUNCTION: _go-module_src_unpack_vendor
> +# @DESCRIPTION:
>  # Extract all archives in ${a} which are not nentioned in ${EGO_VENDOR}
>  # to their usual locations then extract all archives mentioned in
>  # ${EGO_VENDOR} to ${S}/vendor.
> -go-module_src_unpack() {
> -	debug-print-function ${FUNCNAME} "$@"
> +_go-module_src_unpack_vendor() {
> +	# shellcheck disable=SC2120
> +	debug-print-function "${FUNCNAME}" "$@"
>  	local f hash import line repo tarball vendor_tarballs x
>  	vendor_tarballs=()
>  	for line in "${EGO_VENDOR[@]}"; do
> @@ -145,13 +391,59 @@ go-module_src_unpack() {
>  	done
>  }
>
> +# @FUNCTION: _go-module_src_prepare_gosum
> +# @DESCRIPTION:
> +# Validate the Go modules declared by EGO_SUM are sufficent to cover building
> +# the package, without actually building it yet.
> +_go-module_src_prepare_gosum() {
> +	# shellcheck disable=SC2120
> +	debug-print-function "${FUNCNAME}" "$@"
> +
> +	if [[ ! ${_GO_MODULE_SET_GLOBALS_CALLED} ]]; then
> +		die "go-module_set_globals must be called in global scope"
> +	fi
> +
> +	# go.sum entries ending in /go.mod aren't strictly needed at this phase
> +	if [[ ! -e "${GO_MODULE_GOSUM_PATH}" ]]; then
> +		die "Could not find package root go.sum, please update GO_MODULE_GOSUM_PATH"
> +	fi
> +	go-module_minimize_gosum "${GO_MODULE_GOSUM_PATH}"
> +
> +	# Verify that all needed modules are present.
> +	GO111MODULE=on \
> +		go get -v -d -mod readonly || die "Some module is missing, update EGO_SUM"
> +
> +	# Need to re-minimize because go-get expands it again

Why not create and restore a copy?  Or does go-get make other changes?

> +	go-module_minimize_gosum "${GO_MODULE_GOSUM_PATH}"
> +}
> +
> +# @FUNCTION: go-module_minimize_gosum
> +# @DESCRIPTION:
> +# Remove all /go.mod entries from go.sum files
> +# In most cases, if go.sum only has a /go.mod entry without a corresponding
> +# direct entry, this is a sign of a weak dependency that is NOT required for
> +# building the package.
> +go-module_minimize_gosum() {
> +	local gosumfile=${1}
> +	if test ! -e "${gosumfile}".orig; then

Use [[ ... ]].  Be consistent.  This is an eclass, not a throwaway
script.

> +		cp -f "${gosumfile}"{,.orig} || die
> +	fi
> +	awk -e '$2 ~ /\/go.mod$/{next} {print}' \
> +		<"${gosumfile}".orig \
> +		>"${gosumfile}" || die
> +	if grep -sq /go.mod "${gosumfile}"; then
> +		die "sed failed to remove all module go.mod entries from go.sum"

Err, but the rule for grep is inconsistent with the rule for awk.  It's
going to fail when 'go.mod' (i.e. go<ANYCHAR>mod) happens anywhere
on the line.

> +	fi
> +}
> +
>  # @FUNCTION: go-module_live_vendor
>  # @DESCRIPTION:
>  # This function is used in live ebuilds to vendor the dependencies when
>  # upstream doesn't vendor them.
>  go-module_live_vendor() {
> -	debug-print-function ${FUNCNAME} "$@"
> +	debug-print-function "${FUNCNAME}" "$@"
>
> +	# shellcheck disable=SC2086
>  	has live ${PROPERTIES} ||
>  		die "${FUNCNAME} only allowed in live ebuilds"
>  	[[ "${EBUILD_PHASE}" == unpack ]] ||
> @@ -168,7 +460,7 @@ go-module_live_vendor() {
>  # @DESCRIPTION:
>  # Display a warning about security updates for Go programs.
>  go-module_pkg_postinst() {
> -	debug-print-function ${FUNCNAME} "$@"
> +	debug-print-function "${FUNCNAME}" "$@"
>  	[[ -n ${REPLACING_VERSIONS} ]] && return 0
>  	ewarn "${PN} is written in the Go programming language."
>  	ewarn "Since this language is statically linked, security"
> diff --git profiles/thirdpartymirrors profiles/thirdpartymirrors
> index ad4c4b972146..d60f166e07c9 100644
> --- profiles/thirdpartymirrors
> +++ profiles/thirdpartymirrors
> @@ -25,3 +25,4 @@ sourceforge	https://download.sourceforge.net
>  sourceforge.jp	http://iij.dl.sourceforge.jp https://osdn.dl.sourceforge.jp https://jaist.dl.sourceforge.jp
>  ubuntu		http://mirror.internode.on.net/pub/ubuntu/ubuntu/ https://mirror.tcc.wa.edu.au/ubuntu/ http://ubuntu.uni-klu.ac.at/ubuntu/ http://mirror.dhakacom.com/ubuntu-archive/ http://ubuntu.c3sl.ufpr.br/ubuntu/ http://ubuntu.uni-sofia.bg/ubuntu/ http://hr.archive.ubuntu.com/ubuntu/ http://cz.archive.ubuntu.com/ubuntu/ https://mirror.dkm.cz/ubuntu http://ftp.cvut.cz/ubuntu/ http://ftp.stw-bonn.de/ubuntu/ https://ftp-stud.hs-esslingen.de/ubuntu/ https://mirror.netcologne.de/ubuntu/ https://mirror.unej.ac.id/ubuntu/ http://kr.archive.ubuntu.com/ubuntu/ https://mirror.nforce.com/pub/linux/ubuntu/ http://mirror.amsiohosting.net/archive.ubuntu.com/ http://nl3.archive.ubuntu.com/ubuntu/ https://mirror.timeweb.ru/ubuntu/ http://ubuntu.mirror.su.se/ubuntu/ https://ftp.yzu.edu.tw/ubuntu/ https://mirror.aptus.co.tz/pub/ubuntuarchive/ https://ubuntu.volia.net/ubuntu-archive/ https://mirror.sax.uk.as61049.net/ubuntu/ https://mirror.pnl.gov/ubuntu/ http://mirror.cc.columbia.edu/pub/linux/ubuntu/archive/ https://mirrors.namecheap.com/ubuntu/
>  vdr-developerorg http://projects.vdr-developer.org/attachments/download
> +goproxy	https://proxy.golang.org/ https://goproxy.io/ https://gocenter.io/

-- 
Best regards,
Michał Górny