Re: [gentoo-dev] [PATCH 1/1] go-module.eclass: new eclass for go modules

[Michał Górny]

On Tue, 2019-09-24 at 13:08 -0500, William Hubbs wrote:
> This eclass includes the basic settings and src_unpack/pkg_postinst
> functions for Go modules.
>
> Signed-off-by: William Hubbs <williamh@g.o>
> ---
>  eclass/go-module.eclass | 164 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 164 insertions(+)
>  create mode 100644 eclass/go-module.eclass
>
> diff --git a/eclass/go-module.eclass b/eclass/go-module.eclass
> new file mode 100644
> index 00000000000..fcfa5ba643c
> --- /dev/null
> +++ b/eclass/go-module.eclass
> @@ -0,0 +1,164 @@
> +# Copyright 2019 gentoo authors

Sentence case here.

> +# Distributed under the terms of the GNU General Public License v2
> +
> +# @ECLASS: go-module.eclass
> +# @MAINTAINER:
> +# William Hubbs <williamh@g.o>
> +# @SUPPORTED_EAPIS: 7
> +# @BLURB: basic eclass for building software written in the go
> +# programming language that uses go modules.

@BLURB must be one line.

> +# @DESCRIPTION:
> +# This eclass provides basic settings and functions
> +# needed by all software written in the go programming language that uses
> +# go modules.
> +#
> +# 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.

Hmm, don't we want to eventually have one eclass that fits all packages?
I mean, if you can automatically determine whether modules are present
via go.mod file, can't you just make one eclass that determines that
and passes correct flags for both cases?

> +#
> +# 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 does not have a vendor directory, you should use the EGO_VENDOR
> +# variable and the go-module_vendor_uris function as shown in the
> +# example below to handle dependencies.
> +#
> +# Since Go programs are statically linked, it is important that your ebuild's
> +# LICENSE= setting includes the licenses of all statically linked
> +# dependencies. So please make sure it is accurate.
> +#
> +# @EXAMPLE:
> +#
> +# @CODE
> +#
> +# inherit go-module
> +#
> +# EGO_VENDOR=(
> +#	"github.com/xenolf/lego 6cac0ea7d8b28c889f709ec7fa92e92b82f490dd"
> +# "golang.org/x/crypto 453249f01cfeb54c3d549ddb75ff152ca243f9d8 github.com/golang/crypto"
> +# )
> +#
> +# SRC_URI="https://github.com/example/${PN}/archive/v${PV}.tar.gz -> ${P}.tar.gz
> +# $(go-module_vendor_uris)"
> +#
> +# @CODE
> +
> +case ${EAPI:-0} in
> +	7) ;;
> +	*) die "${ECLASS} API in EAPI ${EAPI} not yet established."
> +esac
> +
> +if [[ -z ${_GO_MODULE} ]]; then
> +
> +_GO_MODULE_=1
> +
> +BDEPEND=">=dev-lang/go-1.12"
> +
> +# Force go to build in module mode.
> +# In this mode the GOPATH environment variable is ignored.
> +# this will become the default in the future.
> +export GO111MODULE=on
> +
> +# 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"
> +
> +# Do not complain about CFLAGS etc since go projects do not use them.
> +QA_FLAGS_IGNORED='.*'
> +
> +# Go packages should not be stripped with strip(1).
> +RESTRICT="strip"
> +
> +EXPORT_FUNCTIONS src_unpack pkg_postinst
> +
> +# @ECLASS-VARIABLE: EGO_VENDOR
> +# @DESCRIPTION:
> +# This variable contains a list of vendored packages.
> +# The items of this array are strings that contain the
> +# import path and the git commit hash for a vendored package.
> +# If the import path does not start with github.com, the third argument
> +# can be used to point to a github repository.
> +
> +# @FUNCTION: go-module_vendor_uris
> +# @DESCRIPTION:
> +# Convert the information in EGO_VENDOR to a format suitable for
> +# SRC_URI.
> +# A call to this function should be added to SRC_URI in your ebuild if
> +# the upstream package does not vendor dependencies.

I would go for 'does not include vendored dependencies'.  If you fetch
them for the package, I'd still call it vendoring.

> +go-module_vendor_uris() {
> +	local hash import line repo

You want to make '_' local as well.

> +	for line in "${EGO_VENDOR[@]}"; do
> +		read -r import hash repo _ <<< "${line}"

Don't you want to error out on extraneous args (i.e. when '_' is non-
empty)?

> +		if [[ -n ${repo} ]]; then

I told you already that you can simplify this by doing:

: "${repo:=${import}}"

and then always using the first echo.

> +			echo "https://${repo}/archive/${hash}.tar.gz -> ${repo//\//-}-${hash}.tar.gz"
> +		else
> +			echo "https://${import}/archive/${hash}.tar.gz -> ${import//\//-}-${hash}.tar.gz"
> +		fi
> +	done
> +}
> +
> +# @FUNCTION: go-module_src_unpack
> +# @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} "$@"
> +	local f hash import line repo tarball vendor_uri

Same, you're not making '_' local.

> +	vendor_uri="$(go-module_vendor_uris)"
> +	for f in $A; do
> +		[[ $vendor_uri == *"$f"* ]] && continue

That's a cheap, ugly hack that's going to randomly fail if $f occurs
somewhere in the URI.  Even if that's unlikely to happen, it's not how
you're supposed to do things.

You should really get all filenames here, and compare them separately. 
You can use has() to search a list for a matching filename.  But you
need to get a proper filename list first, either by processing
EGO_VENDOR again or searching through generated SRC_URI properly (taking
just filenames, skipping URIs and arrows).  The former will probably be
more readable.

> +		unpack $f
> +	done
> +
> +	for line in "${EGO_VENDOR[@]}"; do
> +		read -r import hash repo _ <<< "${line}"

Same, check for non-empty '_'.

> +		if [[ -n ${repo} ]]; then
> +			tarball=${repo//\//-}-${hash}.tar.gz

Same, you can simplify it to one branch.  Also, I suppose if you move
this part earlier, you can get the list of tarballs you need for $A
filtering.

> +		else
> +			tarball=${import//\//-}-${hash}.tar.gz
> +		fi
> +		einfo "Vendoring ${import} ${tarball}"

ebegin/eend maybe?

> +		rm -fr "${S}/vendor/${import}" || die
> +		mkdir -p "${S}/vendor/${import}" || die
> +		tar -C "${S}/vendor/${import}" -x --strip-components 1\

As I've pointed out in earlier iteration of your eclasses, you're
missing space before backslash.

> +			-f "${DISTDIR}/${tarball}" || die
> +	done
> +}
> +
> +# @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} "$@"
> +
> +	[[ "${PV}" == *9999* ]] ||
> +		die "${FUNCNAME} only allowed in live/9999 ebuilds"

We use PROPERTIES=live these days.

> +	[[ "${EBUILD_PHASE}" == unpack ]] ||
> +		die "${FUNCNAME} only allowed in src_unpack"
> +	[[ -d "${S}"/vendor ]] ||
> +		die "${FUNCNAME} only allowed when upstream isn't vendoring"
> +
> +	cd "${S}" || die

Isn't it potentially confusing that calling this function in src_unpack
silently changes working directory?  pushd/popd may be preferable.

> +	go mod vendor || die
> +}
> +
> +# @FUNCTION: go-module_pkg_postinst
> +# @DESCRIPTION:
> +# Display a warning about security updates for Go programs.
> +go-module_pkg_postinst() {
> +	debug-print-function ${FUNCNAME} "$@"
> +	ewarn "${PN} is written in the Go programming language."
> +	ewarn "Since this language is statically linked, security"
> +	ewarn "updates will be handled in individual packages and will be"
> +	ewarn "difficult for us to track as a distribution."
> +	ewarn "For this reason, please update any go packages asap when new"
> +	ewarn "versions enter the tree or go stable if you are running the"
> +	ewarn "stable tree."

This message really sounds like those packages should never become
stable.

> +}
> +
> +fi

-- 
Best regards,
Michał Górny