Re: [gentoo-dev] new eclass: go-live.eclass for handling go live ebuilds

[Mike Frysinger]

On 04 Jun 2015 14:10, William Hubbs wrote:
> # @ECLASS: go-live.eclass

since we're going to have a common go eclass, and i don't think we'll want to 
call it "go.eclass", this too probably should not be go-xxx.  if we assume the 
base one will be "golang.eclass", then this should be golang-xxx.eclass.  i'd 
prefer golang-vcs.eclass myself as that's the naming we've been moving towards.

> # @MAINTAINER:
> # William Hubbs <williamh@g.o>
> # @BLURB: Eclass for fetching and unpacking go repositories.
> # @DESCRIPTION:
> # This eclass is written to ease the maintenance of live ebuilds
> # of software written in the Go programming language.

this should note the ebuild is responsible for depending on the right vcs 
packages.  e.g. if you use git, then you need to depend on git.

although ...

> # @ECLASS-VARIABLE: EGO_PN
> # @REQUIRED
> # @DESCRIPTION:
> # This is the import path for the go package. Please emerge dev-lang/go
> # and read "go help importpath" for syntax.
> #
> # Example:
> # @CODE
> # EGO_PN="github.com/user/project"
> # @CODE

can't we automate some of the common hosts ?  if it says github, we know it's 
going to be using at least git.

> 	local saved_umask
> 	if [[ ${EVCS_UMASK} ]]; then
> 		saved_umask=$(umask)
> 		umask "${EVCS_UMASK}" ||
> 			die "${ECLASS}: bad options to umask: ${EVCS_UMASK}"
> 	fi

use `eumask_push` instead

on a related note, i don't think we should encourage the implicit -n operator.  
it adds no overhead and really no code maintenance to specify it.  conversely,
i'm not sure it can be considered common usage.  and even if it is, i still 
don't see a good reason to not just use the explicit -n as it's clear to the 
reader what it's doing.

> 	if [[ ! -d ${EGO_STORE_DIR} ]]; then
> 		(
> 			addwrite /
> 			mkdir -p "${EGO_STORE_DIR}" || die
> 		) || die "${ECLASS}: unable to create ${EGO_STORE_DIR}"
> 	fi

the inner die is redundant

> 	if [[ ${saved_umask} ]]; then
> 		umask "${saved_umask}" ||
> 			die "${ECLASS}: unable to restore saved umask"
> 	fi

use `eumask_pop` instead

> 	[[ ${EGO_PN} ]] ||
> 		die "${ECLASS}: EGO_PN is not set"

prefer -z && myself
	[[ -z ${EGO_PN} ]] && die ...

> 	if [[ ${EVCS_OFFLINE} ]]; then
> 		export GOPATH="${S}:${GOPATH}"

what if GOPATH isn't set ?  should this always be appending a colon ?

> 	local saved_umask
> 	if [[ ${EVCS_UMASK} ]]; then
> 		saved_umask=$(umask)
> 		umask "${EVCS_UMASK}" ||
> 			die "${ECLASS}: Bad options to umask: ${EVCS_UMASK}"
> 	fi

use `eumask_push` instead

> 	go get -d -t -u -v -x "$EGO_PN"

needs braces around ${EGO_PN}, and shouldn't this be calling `die` ?

would also be useful to show the command you're running:
	set -- go get -d -t -u -v -x "${EGO_PN}"
	echo "$@"
	"$@" || die

> 	if [[ ${saved_umask} ]]; then
> 		umask "${saved_umask}" ||
> 			die "${ECLASS}: unable to restore saved umask"
> 	fi

use `eumask_pop` instead

> 	export GOPATH="${S}:${GOPATH}"

same questions here
-mike