On 28/05/10 00:35, Cedric Staniewski 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<[email protected]>
---
Changes:
- renamed error_source_file_not_found to missing_source_exit
- removed differentiation in download_sources: now it prints "Found %s" instead
of "Found %s in build dir" and "Using cached copy of %s"
I have very minor comments below. Suggested naming comments are
debatable...
scripts/makepkg.sh.in | 99 ++++++++++++++++++++++++-------------------------
1 files changed, 48 insertions(+), 51 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 8c0da8b..00167f7 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -179,6 +179,33 @@ 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 a source entry
+#
+# This function accepts a source entry or the already extracted filename of a
+# source entry as input
+get_absolute_filename() {
get_filepath()?
+ local file="$(get_filename "$1")"
+
+ if [[ -f "$startdir/$file" ]] ; then
+ file="$startdir/$file"
+ else
I'd prefer and "elif" here as it seems cleaner.
+ if [[ -f "$SRCDEST/$file" ]] ; then
+ file="$SRCDEST/$file"
+ else
+ return 1
+ fi
+ fi
+
+ echo "$file"
+}
+
+# Print 'source not found' error message and exit makepkg
+missing_source_exit() {
I do not really think a three line function needs documented. Also, I
think "missing_source_file" sounds a better name especially when you
read it after a "||" as it is used.
+ error "$(gettext "Unable to find source file %s.")" "$(get_filename
"$1")"
+ plain "$(gettext "Aborting...")"
+ exit 1 # $E_MISSING_FILE
+}
+
# extract the filename from a source entry
get_filename() {
# if a filename is specified, use it
<snip>
@@ -645,25 +653,17 @@ extract_sources() {
msg "$(gettext "Extracting Sources...")"
local netfile
for netfile in "${sour...@]}"; do
- file=$(get_filename "$netfile")
+ absfile="$(get_absolute_filename "$netfile")" || missing_source_exit
"$netfile"
+ file=${absfile##*/}
if in_array "$file" ${noextra...@]}; 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")
I do not think that this change is not needed.
local ext=${file##*.}
local cmd=''
case "$file_type" in
@@ -693,10 +693,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=$?
or these two. We have set up symlinks in $srcdir so we should just use them
fi
if (( ret )); then
error "$(gettext "Failed to extract %s")" "$file"
@@ -1097,13 +1097,10 @@ create_srcpackage() {
local netfile
for netfile in "${sour...@]}"; 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") ||
missing_source_exit "$netfile"
+ msg2 "$(gettext "Adding %s...")" "${file##*/}"
+ ln -s "$file" "${srclinks}/${pkgbase}"
fi
done
While this works, I would prefer to keep the distinction between local
and downloaded files. So something like:
local file
for file in "${sour...@]}"; do
if [[ -f $file ]]; then
msg2 "$(gettext "Adding %s...")" "$file"
ln -s "${startdir}/$file" "${srclinks}/${pkgbase}"
elif (( SOURCEONLY == 2 )); then
local absfile=$(get_absolute_filename "$file") ||
missing_source_exit "$file"
msg2 "$(gettext "Adding %s...")" "${absfile##*/}"
ln -s "$absfile" "${srclinks}/${pkgbase}"
fi
done
Note the renaming of "netfile" as the name makes little sense anymore...
With those comments addressed, this patch is good to go.
I would like this in the 3.4 release if possible since it is a bug fix.
Given it has string changes, that would mean it would need resubmitted
soon. I apologize for rushing you after taking so long to comment, but
I can take care of the changes if you are busy at the moment.
Allan
