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 <[email protected]> > --- > > 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 >
