On Wed, Feb 19, 2020 at 07:36:27AM +0000, Robin H. Johnson wrote: > 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.
I will look at that in a bit and comment on it. > > > > > > +# 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 If Go itself does that verification during the build, do we need to do it also? > > > +# @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? Maybe, but again I'll think about this more. > > > + 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. Which ebuild was that? this is actually an invalid case for go-module. A go module requires the presence of go.mod and optionally go.sum (if there are no external deps) and optionally vendor/. If you want an invalid case, you would have to be able to test for something like: [[ ! -f "${S}"/go.mod ]] && die "..." > > > > +_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. I'm still not convinced we need a whole new eclass. William
signature.asc
Description: Digital signature