diff mbox

kbuild: deb-pkg improve maintainer address generation

Message ID 20180503114627.30183-1-riku.voipio@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Riku Voipio May 3, 2018, 11:46 a.m. UTC 
From: Riku Voipio <riku.voipio@linaro.org>

There is multiple issues with the genaration of maintainer string

It uses DEBEMAIL and EMAIL enviroment variables, which may contain angle brackets,
creating invalid maintainer strings. The documented KBUILD_BUILD_USER and
KBUILD_BUILD_HOST variables are not used. Undocumented and uncommon NAME
variable is used. Refactor the Maintainer string to:

- use EMAIL or DEBEMAIL directly if they are in form "name <user@host>"
- use KBUILD_BUILD_USER and KBUILD_BUILD_HOST if set before falling
  back to autodetection
- no longer use NAME variable or the useless Anonymous string

The logic is switched from multiline if/then/fi statements to compact
shell variable substition commands.

Reported-by: Mathieu Malaterre <malat@debian.org>
Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 scripts/package/mkdebian | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

Comments

Mathieu Malaterre May 3, 2018, 5:05 p.m. UTC | #1 
Hi,

On Thu, May 3, 2018 at 1:46 PM,  <riku.voipio@linaro.org> wrote:
> From: Riku Voipio <riku.voipio@linaro.org>
>
> There is multiple issues with the genaration of maintainer string
>
> It uses DEBEMAIL and EMAIL enviroment variables, which may contain angle brackets,
> creating invalid maintainer strings. The documented KBUILD_BUILD_USER and
> KBUILD_BUILD_HOST variables are not used. Undocumented and uncommon NAME
> variable is used. Refactor the Maintainer string to:
>
> - use EMAIL or DEBEMAIL directly if they are in form "name <user@host>"
> - use KBUILD_BUILD_USER and KBUILD_BUILD_HOST if set before falling
>   back to autodetection
> - no longer use NAME variable or the useless Anonymous string
>
> The logic is switched from multiline if/then/fi statements to compact
> shell variable substition commands.

Much more elegant indeed.

Acked-by: Mathieu Malaterre <malat@debian.org>

> Reported-by: Mathieu Malaterre <malat@debian.org>
> Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
> ---
>  scripts/package/mkdebian | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/package/mkdebian b/scripts/package/mkdebian
> index 6adb3a16ba3b..a70f20eb877a 100755
> --- a/scripts/package/mkdebian
> +++ b/scripts/package/mkdebian
> @@ -71,22 +71,23 @@ if [ "$ARCH" = "um" ] ; then
>         packagename=user-mode-linux-$version
>  fi
>
> -# Try to determine maintainer and email values
> -if [ -n "$DEBEMAIL" ]; then
> -       email=$DEBEMAIL
> -elif [ -n "$EMAIL" ]; then
> -       email=$EMAIL
> -else
> -       email=$(id -nu)@$(hostname -f 2>/dev/null || hostname)
> -fi
> -if [ -n "$DEBFULLNAME" ]; then
> -       name=$DEBFULLNAME
> -elif [ -n "$NAME" ]; then
> -       name=$NAME
> +email=${DEBEMAIL-$EMAIL}
> +
> +# use email string directly if it contains <email>
> +if echo $email|grep -q '<.*>';
> +then
> +       maintainer=$email
>  else
> -       name="Anonymous"
> +       # or construct the maintainer string
> +       user=${KBUILD_BUILD_USER-$(id -nu)}
> +       name=${DEBFULLNAME-$user}
> +       if [ -n "$email" ]; then
> +               maintainer="$name <$email>"
> +       else
> +               buildhost=${KBUILD_BUILD_HOST-$(hostname -f 2>/dev/null || hostname)}
> +               maintainer="$name <$user@$buildhost>"
> +       fi
>  fi
> -maintainer="$name <$email>"
>
>  # Try to determine distribution
>  if [ -n "$KDEB_CHANGELOG_DIST" ]; then
> --
> 2.14.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Masahiro Yamada May 5, 2018, 9:45 a.m. UTC | #2 
Hi Riku,


2018-05-03 20:46 GMT+09:00  <riku.voipio@linaro.org>:
> From: Riku Voipio <riku.voipio@linaro.org>
>
> There is multiple issues with the genaration of maintainer string
>
> It uses DEBEMAIL and EMAIL enviroment variables, which may contain angle brackets,
> creating invalid maintainer strings. The documented KBUILD_BUILD_USER and
> KBUILD_BUILD_HOST variables are not used. Undocumented and uncommon NAME
> variable is used. Refactor the Maintainer string to:
>
> - use EMAIL or DEBEMAIL directly if they are in form "name <user@host>"
> - use KBUILD_BUILD_USER and KBUILD_BUILD_HOST if set before falling
>   back to autodetection
> - no longer use NAME variable or the useless Anonymous string
>
> The logic is switched from multiline if/then/fi statements to compact
> shell variable substition commands.
>
> Reported-by: Mathieu Malaterre <malat@debian.org>
> Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
> ---


Almost looks good to me.

Just small nits.


>  scripts/package/mkdebian | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/package/mkdebian b/scripts/package/mkdebian
> index 6adb3a16ba3b..a70f20eb877a 100755
> --- a/scripts/package/mkdebian
> +++ b/scripts/package/mkdebian
> @@ -71,22 +71,23 @@ if [ "$ARCH" = "um" ] ; then
>         packagename=user-mode-linux-$version
>  fi
>
> -# Try to determine maintainer and email values
> -if [ -n "$DEBEMAIL" ]; then
> -       email=$DEBEMAIL
> -elif [ -n "$EMAIL" ]; then
> -       email=$EMAIL
> -else
> -       email=$(id -nu)@$(hostname -f 2>/dev/null || hostname)
> -fi
> -if [ -n "$DEBFULLNAME" ]; then
> -       name=$DEBFULLNAME
> -elif [ -n "$NAME" ]; then
> -       name=$NAME
> +email=${DEBEMAIL-$EMAIL}
> +
> +# use email string directly if it contains <email>
> +if echo $email|grep -q '<.*>';
> +then




May I ask two coding style changes?

  - Please add spaces around the pipe operator '|'
  - Move 'then' to the end of the previous line, for style consistency

i.e. like follows:

if echo $email | grep -q '<.*>'; then






> +       maintainer=$email
>  else
> -       name="Anonymous"
> +       # or construct the maintainer string
> +       user=${KBUILD_BUILD_USER-$(id -nu)}
> +       name=${DEBFULLNAME-$user}
> +       if [ -n "$email" ]; then
> +               maintainer="$name <$email>"
> +       else
> +               buildhost=${KBUILD_BUILD_HOST-$(hostname -f 2>/dev/null || hostname)}
> +               maintainer="$name <$user@$buildhost>"
> +       fi
>  fi


I think the else-block can be a bit shorter
if you write like follows:


    # or construct the maintainer string
    user=${KBUILD_BUILD_USER-$(id -nu)}
    name=${DEBFULLNAME-$user}
    if [ -z "$email" ]; then
            buildhost=${KBUILD_BUILD_HOST-$(hostname -f 2>/dev/null ||
hostname)}
            email=$user@$buildhost
    fi
    maintainer="$name <$email>"
diff mbox

Patch

diff --git a/scripts/package/mkdebian b/scripts/package/mkdebian
index 6adb3a16ba3b..a70f20eb877a 100755
--- a/scripts/package/mkdebian
+++ b/scripts/package/mkdebian
@@ -71,22 +71,23 @@  if [ "$ARCH" = "um" ] ; then
 	packagename=user-mode-linux-$version
 fi
 
-# Try to determine maintainer and email values
-if [ -n "$DEBEMAIL" ]; then
-       email=$DEBEMAIL
-elif [ -n "$EMAIL" ]; then
-       email=$EMAIL
-else
-       email=$(id -nu)@$(hostname -f 2>/dev/null || hostname)
-fi
-if [ -n "$DEBFULLNAME" ]; then
-       name=$DEBFULLNAME
-elif [ -n "$NAME" ]; then
-       name=$NAME
+email=${DEBEMAIL-$EMAIL}
+
+# use email string directly if it contains <email>
+if echo $email|grep -q '<.*>';
+then
+	maintainer=$email
 else
-       name="Anonymous"
+	# or construct the maintainer string
+	user=${KBUILD_BUILD_USER-$(id -nu)}
+	name=${DEBFULLNAME-$user}
+	if [ -n "$email" ]; then
+		maintainer="$name <$email>"
+	else
+		buildhost=${KBUILD_BUILD_HOST-$(hostname -f 2>/dev/null || hostname)}
+		maintainer="$name <$user@$buildhost>"
+	fi
 fi
-maintainer="$name <$email>"
 
 # Try to determine distribution
 if [ -n "$KDEB_CHANGELOG_DIST" ]; then