Re: [gentoo-dev] [PATCH] eclass: add rust-toolchain.eclass

[Michał Górny]

On Mon, 2018-10-15 at 21:41 +0200, Dirkjan Ochtman wrote:
> Signed-off-by: Dirkjan Ochtman <djc@g.o>
> ---
>  eclass/rust-toolchain.eclass | 120 +++++++++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
>  create mode 100644 eclass/rust-toolchain.eclass

I'm sorry that I haven't managed to find time to review this in 3 days.

>
> diff --git a/eclass/rust-toolchain.eclass b/eclass/rust-toolchain.eclass
> new file mode 100644
> index 00000000000..d09db264fc3
> --- /dev/null
> +++ b/eclass/rust-toolchain.eclass
> @@ -0,0 +1,120 @@
> +# Copyright 1999-2018 Gentoo Foundation
> +# Distributed under the terms of the GNU General Public License v2
> +
> +# @ECLASS: rust-toolchain.eclass
> +# @MAINTAINER:
> +# Rust Project <rust@g.o>
> +# @SUPPORTED_EAPIS: 6

This doesn't match the conditional below...

> +# @BLURB: helps map gentoo arches to rust ABIs
> +# @DESCRIPTION:
> +# This eclass contains a src_unpack default phase function, and
> +# helper functions, to aid in proper rust-ABI handling for various
> +# gentoo arches.
> +
> +case ${EAPI} in
> +	6) : ;;
> +	7) : ;;

...which permits 6 and 7.

> +	*) die "EAPI=${EAPI:-0} is not supported" ;;
> +esac
> +
> +inherit multilib-build
> +
> +# @ECLASS-VARIABLE: RUST_TOOLCHAIN_BASEURL
> +# @DESCRIPTION:
> +# This variable specifies the base URL used by the
> +# rust_arch_uri and rust_all_arch_uris functions when
> +# generating the URI output list.
> +: ${RUST_TOOLCHAIN_BASEURL:=https://static.rust-lang.org/dist/}
> +
> +# @FUNCTION: rust_abi
> +# @USAGE: [CHOST-value]
> +# @DESCRIPTION:
> +# Outputs the Rust ABI name from a CHOST value, uses CHOST in the
> +# environment if none is specified.
> +
> +rust_abi() {
> +  local CTARGET=${1:-${CHOST}}

You're mixing tab and space indent.

> +  case ${CTARGET%%*-} in
> +    aarch64*)     echo aarch64-unknown-linux-gnu;;
> +    mips64*)      echo mips64-unknown-linux-gnuabi64;;
> +    powerpc64le*) echo powerpc64le-unknown-linux-gnu;;
> +    powerpc64*)   echo powerpc64-unknown-linux-gnu;;
> +    x86_64*)      echo x86_64-unknown-linux-gnu;;
> +    armv6j*s*)    echo arm-unknown-linux-gnueabi;;

Substring match for a single letter is a horrible idea.  It even fails
with one of standard CHOSTs we have: armv6j-unknown-linux-musleabihf.

> +    armv6j*h*)    echo arm-unknown-linux-gnueabihf;;
> +    armv7a*h*)    echo armv7-unknown-linux-gnueabihf;;
> +    i?86*)        echo i686-unknown-linux-gnu;;
> +    mipsel*)      echo mipsel-unknown-linux-gnu;;
> +    mips*)        echo mips-unknown-linux-gnu;;
> +    powerpc*)     echo powerpc-unknown-linux-gnu;;
> +    s390x*)       echo s390x-unknown-linux-gnu;;
> +    *)            echo ${CTARGET};;
> +  esac
> +}
> +
> +# @FUNCTION: rust_all_abis
> +# @DESCRIPTION:
> +# Outputs a list of all the enabled Rust ABIs
> +rust_all_abis() {
> +  if use multilib; then

USE=multilib is deprecated.  Any new packages should be using ABI_*
flags directly.

> +    local abi
> +    local ALL_ABIS=()
> +    for abi in $(multilib_get_enabled_abis); do

In fact, to what purpose are you inheriting multilib-build if you
completely ignore the new API and instead use obsolete multilib.eclass
functions directly?

> +      ALL_ABIS+=( $(rust_abi $(get_abi_CHOST ${abi})) )
> +    done
> +    local abi_list
> +    IFS=, eval 'abi_list=${ALL_ABIS[*]}'

The 'eval' here is absolutely unnecessary:

  local IFS=,
  echo "${ALL_ABIS[*]}"

> +    echo ${abi_list}
> +  else
> +    rust_abi
> +  fi
> +}
> +
> +# @FUNCTION: rust_arch_uri
> +# @USAGE: <rust-ABI> <base-uri> [alt-distfile-basename]
> +# @DESCRIPTION:
> +# Output the URI for use in SRC_URI, combining $RUST_TOOLCHAIN_BASEURL
> +# and the URI suffix provided in ARG2 with the rust ABI in ARG1, and
> +# optionally renaming to the distfile basename specified in ARG3.

Why are you using ARGn when you explicitly named the parameters above?

> +#
> +# @EXAMPLE:
> +# SRC_URI="amd64? (
> +#    $(rust_arch_uri x86_64-unknown-linux-gnu rustc-${STAGE0_VERSION})
> +# )"
> +#
> +rust_arch_uri() {
> +  if [ -n "$3" ]; then

Always use [[ ... ]] in bash.

> +    echo "${RUST_TOOLCHAIN_BASEURL}${2}-${1}.tar.gz -> ${3}-${1}.tar.gz"
> +  else
> +    echo "${RUST_TOOLCHAIN_BASEURL}${2}-${1}.tar.gz"

Why not make this common, and just append "-> ..." when [[ -n ${3} ]]?

> +  fi
> +}
> +
> +# @FUNCTION: rust_all_arch_uris
> +# @USAGE <base-uri> [alt-distfile-basename]
> +# @DESCRIPTION:
> +# Outputs the URIs for SRC_URI to help fetch dependencies, using a base URI
> +# provided as an argument.  Optionally allows for distfile renaming via a specified
> +# basename.
> +#
> +# @EXAMPLE:
> +# SRC_URI="$(rust_all_arch_uris rustc-${STAGE0_VERSION})"
> +#
> +rust_all_arch_uris()
> +{
> +  local uris=""
> +  uris+="amd64? ( $(rust_arch_uri x86_64-unknown-linux-gnu       "$@") ) "
> +  uris+="arm?   ( $(rust_arch_uri arm-unknown-linux-gnueabi      "$@")
> +                  $(rust_arch_uri arm-unknown-linux-gnueabihf    "$@")
> +                  $(rust_arch_uri armv7-unknown-linux-gnueabihf  "$@") ) "
> +  uris+="arm64? ( $(rust_arch_uri aarch64-unknown-linux-gnu      "$@") ) "
> +  uris+="mips?  ( $(rust_arch_uri mips-unknown-linux-gnu         "$@")
> +                  $(rust_arch_uri mipsel-unknown-linux-gnu       "$@")
> +                  $(rust_arch_uri mips64-unknown-linux-gnuabi64  "$@") ) "
> +  uris+="ppc?   ( $(rust_arch_uri powerpc-unknown-linux-gnu      "$@") ) "
> +  uris+="ppc64? ( $(rust_arch_uri powerpc64-unknown-linux-gnu    "$@")
> +                  $(rust_arch_uri powerpc64le-unknown-linux-gnu  "$@") ) "
> +  uris+="s390?  ( $(rust_arch_uri s390x-unknown-linux-gnu        "$@") ) "
> +  uris+="x86?   ( $(rust_arch_uri i686-unknown-linux-gnu         "$@") ) "
> +  echo "${uris}"
> +}

How are you going to deal with future versions having more/less
variants?

-- 
Best regards,
Michał Górny