[pacman-dev] [PATCH] makepkg: refactor checking for write permissions into a utility function

[Allan McRae]

On 08/10/17 17:05, Eli Schwartz wrote:
> Additionally provide a separate error for the confusing if unlikely
> event that the user tries to use an existing file as a package output
> directory.
> 
> This also means we now consistently try to create any nonexistent *DEST
> directories as needed before aborting with E_FS_PERMISSIONS. Previously
> only $BUILDDIR received that kindness.
> 
> In the process, move the handling of $extra_environment[@] to where it
> always belonged, above $BUILDDIR.
> 

This is a separate change and should be a separate patch.
(Does it need to go even higher?)


> Fixes FS#43537
> 
> Signed-off-by: Eli Schwartz <eschwartz at archlinux.org>
> ---
> 
> Am I trying to do way, way too much here? mkdir will hopefully already
> provide a somewhat useful error message in the case of an existing file,
> as we don't quiet it.

(added after review below).
The more I look at this, the more I think the patch is doing too much.
Letting mkdir error and then giving the makepkg error afterward is
probably enough here.  The extra may be error prone.

Although mkdir gives a really bad error when a file exists that
conflicts with the path to be created...

> 
> And this will need to be rebased on top of escondida's error codes patch
> 
>  scripts/libmakepkg/util/util.sh.in | 19 +++++++++++++++++++
>  scripts/makepkg.sh.in              | 29 +++++++++++------------------
>  2 files changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in
> index d676249d..a8813338 100644
> --- a/scripts/libmakepkg/util/util.sh.in
> +++ b/scripts/libmakepkg/util/util.sh.in
> @@ -83,3 +83,22 @@ cd_safe() {
>  		exit 1
>  	fi
>  }
> +
> +# Try to create directory if one does not yet exist. Fails if the directory
> +# exists but has no write permissions, or if there is an existing file with
> +# the same name.
> +ensure_dir() {

ensure_writable_dir()

Every time I read "ensure_dir", I had to think what was ensured!

> +	local parent=$1 dirname=$1
> +
> +	until [[ -e $parent ]]; do
> +		parent="${parent%/*}"
> +	done
> +	if [[ -f $parent ]]; then

Is that a strong enough test? Isn't "! -d $parent" what is really
wanted?  This might be too strong (symlink to directory would fail).

> +		error "$(gettext "Cannot create needed directory %s because it is already a file.")" "$parent"
> +		return 1
> +	fi

I think we can change the error message to be more succinct:
"$(gettext "Cannot create %s due to conflicting file.")" "$dirname"


> +	if ( [[ ! -d $dirname ]] && ! mkdir -p "$dirname" ) || [[ ! -w $dirname ]]; then
> +		return 1
> +	fi
> +	return 0
> +}
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 6449accd..b25e4106 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -1359,32 +1359,25 @@ else
>  	unset ALL_OFF BOLD BLUE GREEN RED YELLOW
>  fi
>  
> +# override settings from extra variables on commandline, if any
> +if (( ${#extra_environment[*]} )); then
> +	export "${extra_environment[@]}"
> +fi
> +
>  
>  # override settings with an environment variable for batch processing
>  BUILDDIR=${_BUILDDIR:-$BUILDDIR}
>  BUILDDIR=${BUILDDIR:-$startdir} #default to $startdir if undefined
> -if [[ ! -d $BUILDDIR ]]; then
> -	if ! mkdir -p "$BUILDDIR"; then
> -		error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR"
> -		plain "$(gettext "Aborting...")"
> -		exit 1
> -	fi
> -	chmod a-s "$BUILDDIR"
> -fi
> -if [[ ! -w $BUILDDIR ]]; then
> +if ! ensure_dir "$BUILDDIR"; then
>  	error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR"
>  	plain "$(gettext "Aborting...")"
>  	exit 1
>  fi
> -
> -# override settings from extra variables on commandline, if any
> -if (( ${#extra_environment[*]} )); then
> -	export "${extra_environment[@]}"
> -fi
> +chmod a-s "$BUILDDIR"
>  
>  PKGDEST=${_PKGDEST:-$PKGDEST}
>  PKGDEST=${PKGDEST:-$startdir} #default to $startdir if undefined
> -if (( ! (NOBUILD || GENINTEG) )) && [[ ! -w $PKGDEST ]]; then
> +if (( ! (NOBUILD || GENINTEG) )) && ! ensure_dir "$PKGDEST"; then
>  	error "$(gettext "You do not have write permission to store packages in %s.")" "$PKGDEST"
>  	plain "$(gettext "Aborting...")"
>  	exit 1
> @@ -1392,7 +1385,7 @@ fi
>  
>  SRCDEST=${_SRCDEST:-$SRCDEST}
>  SRCDEST=${SRCDEST:-$startdir} #default to $startdir if undefined
> -if [[ ! -w $SRCDEST ]] ; then
> +if ! ensure_dir "$SRCDEST" ; then
>  	error "$(gettext "You do not have write permission to store downloads in %s.")" "$SRCDEST"
>  	plain "$(gettext "Aborting...")"
>  	exit 1
> @@ -1401,7 +1394,7 @@ fi
>  SRCPKGDEST=${_SRCPKGDEST:-$SRCPKGDEST}
>  SRCPKGDEST=${SRCPKGDEST:-$startdir} #default to $startdir if undefined
>  if (( SOURCEONLY )); then
> -	if [[ ! -w $SRCPKGDEST ]]; then
> +	if ! ensure_dir "$SRCPKGDEST"; then
>  		error "$(gettext "You do not have write permission to store source tarballs in %s.")" "$SRCPKGDEST"
>  		plain "$(gettext "Aborting...")"
>  		exit 1
> @@ -1414,7 +1407,7 @@ fi
>  
>  LOGDEST=${_LOGDEST:-$LOGDEST}
>  LOGDEST=${LOGDEST:-$startdir} #default to $startdir if undefined
> -if (( LOGGING )) && [[ ! -w $LOGDEST ]]; then
> +if (( LOGGING )) && ! ensure_dir "$LOGDEST"; then
>  	error "$(gettext "You do not have write permission to store logs in %s.")" "$LOGDEST"
>  	plain "$(gettext "Aborting...")"
>  	exit 1
>