From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by finch.gentoo.org (Postfix) with ESMTPS id 8F95E138350 for ; Wed, 19 Feb 2020 07:36:32 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id BE880E0999; Wed, 19 Feb 2020 07:36:29 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 1A30DE0985 for ; Wed, 19 Feb 2020 07:36:29 +0000 (UTC) Received: from grubbs.orbis-terrarum.net (localhost [127.0.0.1]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by smtp.gentoo.org (Postfix) with ESMTPS id B543E34ED67 for ; Wed, 19 Feb 2020 07:36:27 +0000 (UTC) Received: (qmail 18785 invoked by uid 10000); 19 Feb 2020 07:36:27 -0000 Date: Wed, 19 Feb 2020 07:36:27 +0000 From: "Robin H. Johnson" To: gentoo-dev@lists.gentoo.org Subject: Re: [gentoo-dev] [PATCH v2 1/4] eclass/go-module: add support for building based on go.sum Message-ID: References: <20200217092232.9483-1-robbat2@gentoo.org> <20200219054645.GA18470@linux1.home> Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-dev@lists.gentoo.org Reply-to: gentoo-dev@lists.gentoo.org X-Auto-Response-Suppress: DR, RN, NRN, OOF, AutoReply MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="z20Z95FJiBb6FirT" Content-Disposition: inline In-Reply-To: <20200219054645.GA18470@linux1.home> X-Archives-Salt: 448e74b2-ba23-4598-ae48-2401b7d14400 X-Archives-Hash: c019fe8c3ac4992b29b6fdf216e7da83 --z20Z95FJiBb6FirT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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=3D"${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. >=20 > > +# go-module_set_globals > > +# > > +# SRC_URI=3D"https://github.com/example/${PN}/archive/v${PV}.tar.gz ->= ${P}.tar.gz > > +# ${EGO_SUM_SRC_URI}" > > +# > > +# LICENSE=3D"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. >=20 > > +# 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,=20 > > +# > 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 > > +# @EXAMPLE: > > +# # github.com/BurntSushi/toml is a build-time only dep > > +# # github.com/aybabtme/rgbterm is a runtime dep, annotated with licen= ses >=20 > 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 =2E.. > > +# 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:=3Dmirror://goproxy/}" >=20 > 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 sy= mlinks (fast!) > > +# - Modules listed in EGO_VENDOR are extracted to "${S}/vendor" (slow) > > +# > > +# This function does NOT unpack the base distfile of a Go-based packag= e. > > +# 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() { >=20 > 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? > > + 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. > > +_go-module_src_unpack_vendor() { > > + # shellcheck disable=3DSC2120 > > + 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. >> ... > If EGO_SUM is up to date, this could also mean that upstream forgot to > run go mod tidy and commit the go.mod/go.sum before the release, so it > could be a bug that needs to be reported. Yes. I found at least one package where the upstream had failed to run go mod tidy in the release: I already reported it, and they say they will include it in the next release, not doing a release just for it. --=20 Robin Hugh Johnson Gentoo Linux: Dev, Infra Lead, Foundation Treasurer E-Mail : robbat2@gentoo.org GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85 GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136 --z20Z95FJiBb6FirT Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Robbat2 @ Orbis-Terrarum Networks - The text below is a digital signature. If it doesn't make any sense to you, ignore it. iQKTBAABCgB9FiEEveu2pS8Vb98xaNkRGTlfI8WIJsQFAl5M5XdfFIAAAAAALgAo aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldEJE RUJCNkE1MkYxNTZGREYzMTY4RDkxMTE5Mzk1RjIzQzU4ODI2QzQACgkQGTlfI8WI JsSBvw/+IOq6dIo6aJ1Vmfp+b1cV2cQdigEIlTtvUnFTLbCni786G8pdSB+K3aeU MyyI6AK0kXr4qdg5FslhAg5XbNWFKLL2W1/tSx59VONPbQPSOY+2hEft3aZ53GSG Bz5fQNDv68+bMtnOcmkcqtamVpgGkw1Z9HYFkEa5d5RPdT5Gy04qmhSHHMGn6VzV Q6o9uKyN2hr1CfWivZQI9qCLGjyMrYrlX2GlCqwurmuH2ynqz+Wx1CiDAclmUZDi X9MMzKZgHzXuVaExO3NBLg4ff3/rc/IwLPt22jMSapr30UIZycsv1PxBrWodm6T3 RPX6impdSn4eg7TmIkPTM8JK7hCBRPNkCF+11QR1rHj/6kQhTCMkbBom/Q9inMiM fCI+HMmGG9lUwh02bjQUhQbqqYM8AsbULNvly91G2gwqZWbIH59HUxfHC/PwEwUL gioGNelLlO/R2NZGTDEpZuXuwNtZsFCL+t5cQkqRQu9XoQrNRw6JYaQYgwPXk/10 ELgh7CmoJerqP7RgH82KPeyhcyTuQ7ZKpE7agn1W7fzK6sLdlwqc/qUWFcOOVOMW NLjdPKAWeJmIhweNAhSwtITfqAFOoKLKFZzL/4HeHwaLJtuzEUtbo+N9P+feET05 esF/88FWDWC1L7NIlVcM2zWfiVxpv6w72txb70rdSdUbFLBckFg= =W6K7 -----END PGP SIGNATURE----- --z20Z95FJiBb6FirT--