Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass

["Michał Górny" <mgorny@gentoo.org>]

On Sun, 2023-07-30 at 22:01 +0200, Maciej Barć wrote:
> > > +# @ECLASS_VARIABLE: NUGETS
> > > +# @DEFAULT_UNSET
> > > +# @PRE_INHERIT
> > > +# @DESCRIPTION:
> > > +# String containing all NuGet packages that need to be downloaded.
> > > +# Used by "nuget_uris".
> > > +#
> > > +# Example:
> > > +# @CODE
> > > +# NUGETS="
> > > +#	ImGui.NET-1.87.2
> > > +#	Config.Net-4.19.0
> > > +# "
> > > +#
> > > +# inherit dotnet-pkg
> > > +#
> > > +# ...
> > > +#
> > > +# SRC_URI+=" $(nuget_uris) "
> > > +# @CODE
> > > +
> > > +# @FUNCTION: nuget_uris
> > > +# @USAGE: <nuget...>
> > > +# @DESCRIPTION:
> > > +# Generates the URIs to put in SRC_URI to help fetch dependencies.
> > > +# If no arguments provided, uses the "NUGETS" variable.
> > 
> > Sounds like you're copying the old, horribly slow cargo.eclass API.
> > Don't do that, unless you have to.  Prefer variable-setting approach
> > used by modern cargo.eclass.
> > 
> 
> Please elaborate.
> 
> Maybe such mechanisms could be extracted into standalone eclass? I do 
> not know cargo, rust nor how it is used in Gentoo.
> 

Calling subshells in global scope is a very bad idea, and it is
completely unnecessary in the most common case when NUGETS are defined
prior to inherit and URIs are inserted into top-level SRC_URI.

Just look at cargo.eclass.

> > > +# @FUNCTION: nuget_link
> > > +# @USAGE: <nuget-path>
> > > +# @DESCRIPTION:
> > > +# Link a specified NuGet package at "nuget-path" to the "NUGET_PACKAGES"
> > > +# directory.
> > > +#
> > > +# Example:
> > > +# @CODE
> > > +# nuget_link "${DISTDIR}"/pkg.0.nupkg
> > > +# @CODE
> > > +#
> > > +# This function is used inside "dotnet-pkg_src_unpack"
> > > +# from the "dotnet-pkg" eclass.
> > > +nuget_link() {
> > > +	[[ ! "${1}" ]] && die "${FUNCNAME}: no nuget path given"
> > > +
> > > +	mkdir -p "${NUGET_PACKAGES}" || die
> > > +
> > > +	local nuget_name="${1##*/}"
> > > +
> > > +	if [[ -f "${NUGET_PACKAGES}"/${nuget_name} ]] ; then
> > > +		ewarn "${FUNCNAME}: ${nuget_name} already exists"
> > 
> > What does that mean?  What is the user supposed to do about it?  Is it
> > normal?  If it's normal, then why are you warning about it?  If it isn't
> > normal, then why isn't this fatal?
> 
> This can happen if NuGets copied from SYSTEM_NUGETS are overwritten by 
> ones specified in ebuild.
> 
> This might be desired by ebuild author.
> 
> We need to have this logged, I think ewarn is appropriate.

ewarn is a warning for the end user.  The end user doesn't benefit from
being warned for "ebuild is doing something that might be wrong, or
might be perfectly fine, ignore this".

-- 
Best regards,
Michał Górny