[pacman-dev] [PATCH] makepkg: refactor absolute filename detection
Move the absolute filename detection to a new function to reduce code duplication. This patch also fixes the --allsource option that did not include remote source files if they reside in $startdir instead of $SRCDEST. Signed-off-by: Cedric Staniewski <cedric@gmx.ca> --- Finally finished this quiet old patch[1]. However, I am not completely happy with it: 1) error_source_file_not_found is an ugly name; need to find a better/shorter one. It's not possible to correctly exit makepkg from within a subshell anymore which why these file=... || error_source_file_not_found checks are necessary. 2) The part in download_sources is rather ugly too. Do we really need to distinguish between source files from build dir and from cache? I think I've tested every changed function, but please do further testing. This patch is rebased on master and Dieter's patch[2]. [1] http://mailman.archlinux.org/pipermail/pacman-dev/2009-October/009755.html [2] http://mailman.archlinux.org/pipermail/pacman-dev/2010-May/010779.html scripts/makepkg.sh.in | 106 +++++++++++++++++++++++++----------------------- 1 files changed, 55 insertions(+), 51 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 9741879..96c5d12 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -179,6 +179,32 @@ trap 'trap_exit "$(gettext "An unknown error has occurred. Exiting...")"' ERR # 1) "filename::http://path/to/file" # 2) "http://path/to/file" +# return the absolute filename of an source entry +# +# this function accepts a source entry or the already extracted filename of a +# source entry as input +get_absolute_filename() { + local file="$(get_filename "$1")" + + if [[ -f "$startdir/$file" ]] ; then + file="$startdir/$file" + else + if [[ -f "$SRCDEST/$file" ]] ; then + file="$SRCDEST/$file" + else + return 1 + fi + fi + + echo "$file" +} + +error_source_file_not_found() { + error "$(gettext "Unable to find source file %s.")" "$file" + plain "$(gettext "Aborting...")" + exit 1 # $E_MISSING_FILE +} + # extract the filename from a source entry get_filename() { # if a filename is specified, use it @@ -458,20 +484,24 @@ download_sources() { local netfile for netfile in "${source[@]}"; do - local file=$(get_filename "$netfile") - local url=$(get_url "$netfile") - if [[ -f $startdir/$file ]]; then - msg2 "$(gettext "Found %s in build dir")" "$file" - rm -f "$srcdir/$file" - ln -s "$startdir/$file" "$srcdir/" - continue - elif [[ -f $SRCDEST/$file ]]; then - msg2 "$(gettext "Using cached copy of %s")" "$file" - rm -f "$srcdir/$file" - ln -s "$SRCDEST/$file" "$srcdir/" + local file + if file=$(get_absolute_filename "$netfile"); then + if [[ ${file%/*} = $startdir ]]; then + msg2 "$(gettext "Found %s in build dir")" "${file##*/}" + elif [[ ${file%/*} = $SRCDEST ]]; then + msg2 "$(gettext "Using cached copy of %s")" "${file##*/}" + else + # should never be reached + error 'Source file found but it neither exists in $startdir nor $SRCDEST.' + exit 1 + fi + ln -sf "$file" "$srcdir/" continue fi + file=$(get_filename "$netfile") + local url=$(get_url "$netfile") + # if we get here, check to make sure it was a URL, else fail if [[ $file = $url ]]; then error "$(gettext "%s was not found in the build directory and is not a URL.")" "$file" @@ -553,18 +583,7 @@ generate_checksums() { local netfile for netfile in "${source[@]}"; do - local file="$(get_filename "$netfile")" - - if [[ ! -f $file ]] ; then - if [[ ! -f $SRCDEST/$file ]] ; then - error "$(gettext "Unable to find source file %s to generate checksum.")" "$file" - plain "$(gettext "Aborting...")" - exit 1 - else - file="$SRCDEST/$file" - fi - fi - + local file="$(get_absolute_filename "$netfile")" || error_source_file_not_found local sum="$(openssl dgst -${integ} "$file")" sum=${sum##* } (( ct )) && echo -n "$indent" @@ -600,14 +619,10 @@ check_checksums() { file="$(get_filename "$file")" echo -n " $file ... " >&2 - if [[ ! -f $file ]] ; then - if [[ ! -f $SRCDEST/$file ]] ; then - echo "$(gettext "NOT FOUND")" >&2 - errors=1 - found=0 - else - file="$SRCDEST/$file" - fi + if ! file="$(get_absolute_filename "$file")"; then + echo "$(gettext "NOT FOUND")" >&2 + errors=1 + found=0 fi if (( $found )) ; then @@ -645,25 +660,17 @@ extract_sources() { msg "$(gettext "Extracting Sources...")" local netfile for netfile in "${source[@]}"; do - file=$(get_filename "$netfile") + absfile="$(get_absolute_filename "$netfile")" || error_source_file_not_found + file=${absfile##*/} if in_array "$file" ${noextract[@]}; then #skip source files in the noextract=() array # these are marked explicitly to NOT be extracted continue fi - if [[ ! -f $file ]] ; then - if [[ ! -f $SRCDEST/$file ]] ; then - error "$(gettext "Unable to find source file %s for extraction.")" "$file" - plain "$(gettext "Aborting...")" - exit 1 - else - file="$SRCDEST/$file" - fi - fi # fix flyspray #6246 - local file_type=$(file -bizL "$file") + local file_type=$(file -bizL "$absfile") local ext=${file##*.} local cmd='' case "$file_type" in @@ -693,10 +700,10 @@ extract_sources() { local ret=0 msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd" if [[ $cmd = bsdtar ]]; then - $cmd -xf "$file" || ret=$? + $cmd -xf "$absfile" || ret=$? else rm -f "${file%.*}" - $cmd -dcf "$file" > "${file%.*}" || ret=$? + $cmd -dcf "$absfile" > "${file%.*}" || ret=$? fi if (( ret )); then error "$(gettext "Failed to extract %s")" "$file" @@ -1091,13 +1098,10 @@ create_srcpackage() { local netfile for netfile in "${source[@]}"; do - local file=$(get_filename "$netfile") - if [[ -f $netfile ]]; then - msg2 "$(gettext "Adding %s...")" "$netfile" - ln -s "${startdir}/$netfile" "${srclinks}/${pkgbase}" - elif (( SOURCEONLY == 2 )) && [[ -f $SRCDEST/$file ]]; then - msg2 "$(gettext "Adding %s...")" "$file" - ln -s "$SRCDEST/$file" "${srclinks}/${pkgbase}/" + if [[ -f $netfile ]] || (( SOURCEONLY == 2 )); then + local file=$(get_absolute_filename "$netfile") || error_source_file_not_found + msg2 "$(gettext "Adding %s...")" "${file##*/}" + ln -s "$file" "${srclinks}/${pkgbase}" fi done -- 1.7.1
On Mon, May 17, 2010 at 7:01 AM, Cedric Staniewski <cedric@gmx.ca> wrote:
Move the absolute filename detection to a new function to reduce code duplication.
This patch also fixes the --allsource option that did not include remote source files if they reside in $startdir instead of $SRCDEST.
Signed-off-by: Cedric Staniewski <cedric@gmx.ca> ---
Finally finished this quiet old patch[1]. However, I am not completely happy with it:
1) error_source_file_not_found is an ugly name; need to find a better/shorter one. It's not possible to correctly exit makepkg from within a subshell anymore which why these file=... || error_source_file_not_found checks are necessary.
Yeah this is disgusting. :/ I am banging my head on this too trying to come up with something better and nothing is coming to mind...
2) The part in download_sources is rather ugly too. Do we really need to distinguish between source files from build dir and from cache?
No, I don't think so, especially since we don't really do that anywhere else.
I think I've tested every changed function, but please do further testing. This patch is rebased on master and Dieter's patch[2].
[1] http://mailman.archlinux.org/pipermail/pacman-dev/2009-October/009755.html [2] http://mailman.archlinux.org/pipermail/pacman-dev/2010-May/010779.html
scripts/makepkg.sh.in | 106 +++++++++++++++++++++++++----------------------- 1 files changed, 55 insertions(+), 51 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 9741879..96c5d12 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -179,6 +179,32 @@ trap 'trap_exit "$(gettext "An unknown error has occurred. Exiting...")"' ERR # 1) "filename::http://path/to/file" # 2) "http://path/to/file"
+# return the absolute filename of an source entry +# +# this function accepts a source entry or the already extracted filename of a +# source entry as input +get_absolute_filename() { + local file="$(get_filename "$1")" + + if [[ -f "$startdir/$file" ]] ; then + file="$startdir/$file" + else + if [[ -f "$SRCDEST/$file" ]] ; then + file="$SRCDEST/$file" + else + return 1 + fi + fi + + echo "$file" +} + +error_source_file_not_found() { + error "$(gettext "Unable to find source file %s.")" "$file" + plain "$(gettext "Aborting...")" + exit 1 # $E_MISSING_FILE +} + # extract the filename from a source entry get_filename() { # if a filename is specified, use it @@ -458,20 +484,24 @@ download_sources() {
local netfile for netfile in "${source[@]}"; do - local file=$(get_filename "$netfile") - local url=$(get_url "$netfile") - if [[ -f $startdir/$file ]]; then - msg2 "$(gettext "Found %s in build dir")" "$file" - rm -f "$srcdir/$file" - ln -s "$startdir/$file" "$srcdir/" - continue - elif [[ -f $SRCDEST/$file ]]; then - msg2 "$(gettext "Using cached copy of %s")" "$file" - rm -f "$srcdir/$file" - ln -s "$SRCDEST/$file" "$srcdir/" + local file + if file=$(get_absolute_filename "$netfile"); then + if [[ ${file%/*} = $startdir ]]; then + msg2 "$(gettext "Found %s in build dir")" "${file##*/}" + elif [[ ${file%/*} = $SRCDEST ]]; then + msg2 "$(gettext "Using cached copy of %s")" "${file##*/}" + else + # should never be reached + error 'Source file found but it neither exists in $startdir nor $SRCDEST.' + exit 1 + fi + ln -sf "$file" "$srcdir/" continue fi
+ file=$(get_filename "$netfile") + local url=$(get_url "$netfile") + # if we get here, check to make sure it was a URL, else fail if [[ $file = $url ]]; then error "$(gettext "%s was not found in the build directory and is not a URL.")" "$file" @@ -553,18 +583,7 @@ generate_checksums() {
local netfile for netfile in "${source[@]}"; do - local file="$(get_filename "$netfile")" - - if [[ ! -f $file ]] ; then - if [[ ! -f $SRCDEST/$file ]] ; then - error "$(gettext "Unable to find source file %s to generate checksum.")" "$file" - plain "$(gettext "Aborting...")" - exit 1 - else - file="$SRCDEST/$file" - fi - fi - + local file="$(get_absolute_filename "$netfile")" || error_source_file_not_found local sum="$(openssl dgst -${integ} "$file")" sum=${sum##* } (( ct )) && echo -n "$indent" @@ -600,14 +619,10 @@ check_checksums() { file="$(get_filename "$file")" echo -n " $file ... " >&2
- if [[ ! -f $file ]] ; then - if [[ ! -f $SRCDEST/$file ]] ; then - echo "$(gettext "NOT FOUND")" >&2 - errors=1 - found=0 - else - file="$SRCDEST/$file" - fi + if ! file="$(get_absolute_filename "$file")"; then + echo "$(gettext "NOT FOUND")" >&2 + errors=1 + found=0 fi
if (( $found )) ; then @@ -645,25 +660,17 @@ extract_sources() { msg "$(gettext "Extracting Sources...")" local netfile for netfile in "${source[@]}"; do - file=$(get_filename "$netfile") + absfile="$(get_absolute_filename "$netfile")" || error_source_file_not_found + file=${absfile##*/} if in_array "$file" ${noextract[@]}; then #skip source files in the noextract=() array # these are marked explicitly to NOT be extracted continue fi
- if [[ ! -f $file ]] ; then - if [[ ! -f $SRCDEST/$file ]] ; then - error "$(gettext "Unable to find source file %s for extraction.")" "$file" - plain "$(gettext "Aborting...")" - exit 1 - else - file="$SRCDEST/$file" - fi - fi
# fix flyspray #6246 - local file_type=$(file -bizL "$file") + local file_type=$(file -bizL "$absfile") local ext=${file##*.} local cmd='' case "$file_type" in @@ -693,10 +700,10 @@ extract_sources() { local ret=0 msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd" if [[ $cmd = bsdtar ]]; then - $cmd -xf "$file" || ret=$? + $cmd -xf "$absfile" || ret=$? else rm -f "${file%.*}" - $cmd -dcf "$file" > "${file%.*}" || ret=$? + $cmd -dcf "$absfile" > "${file%.*}" || ret=$? fi if (( ret )); then error "$(gettext "Failed to extract %s")" "$file" @@ -1091,13 +1098,10 @@ create_srcpackage() {
local netfile for netfile in "${source[@]}"; do - local file=$(get_filename "$netfile") - if [[ -f $netfile ]]; then - msg2 "$(gettext "Adding %s...")" "$netfile" - ln -s "${startdir}/$netfile" "${srclinks}/${pkgbase}" - elif (( SOURCEONLY == 2 )) && [[ -f $SRCDEST/$file ]]; then - msg2 "$(gettext "Adding %s...")" "$file" - ln -s "$SRCDEST/$file" "${srclinks}/${pkgbase}/" + if [[ -f $netfile ]] || (( SOURCEONLY == 2 )); then + local file=$(get_absolute_filename "$netfile") || error_source_file_not_found + msg2 "$(gettext "Adding %s...")" "${file##*/}" + ln -s "$file" "${srclinks}/${pkgbase}" fi done
-- 1.7.1
Excerpts from Dan McGee's message of 2010-05-24 22:53:28 -0400:
On Mon, May 17, 2010 at 7:01 AM, Cedric Staniewski <cedric@gmx.ca> wrote:
1) error_source_file_not_found is an ugly name; need to find a better/shorter one. It's not possible to correctly exit makepkg from within a subshell anymore which why these file=... || error_source_file_not_found checks are necessary.
Yeah this is disgusting. :/
I am banging my head on this too trying to come up with something better and nothing is coming to mind...
How about "missing_source_exit"? -- David Campbell
On 25.05.2010 05:37, David Campbell wrote:
How about "missing_source_exit"?
Thanks, that's better.
participants (3)
-
Cedric Staniewski
-
Dan McGee
-
David Campbell