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

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 ---

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