[pacman-dev] Patch: Add install like support to makepkg for repo-add
Allan McRae
allan at archlinux.org
Tue Jun 28 23:40:28 EDT 2011
On 12/06/11 12:13, Simon Gomizelj wrote:
> Hi,
>
> First of all, I'd like to say that this is both my first patch ever to
> an upstream project and my first time posting on a development mailing
> list.
Great to see a new contributor. Hopefully the much delayed reply here
has not been too off-putting...
General comments about the patch. I do not think setting REPO_PATH and
REPO_DB in makepkg.conf is the way to go. I would prefer something like:
makepkg --repo-add /path/to/repo.db.tar.gz
That means I can manage multiple repos more easily on my machine. Note
I also went for "-" in the middle of --repo-add for consistency with
name of the script. I'm not sure about the need for the short "-a"
option, but I could be convinced.
>> From 56ef40a2dbe42fafd32a152d102962d52e33e6f4 Mon Sep 17 00:00:00 2001
> From: Simon Gomizelj<simongmzlj at gmail.com>
> Date: Sat, 11 Jun 2011 21:16:11 -0400
> Subject: [PATCH] Added repo-add support.
>
> Signed-off-by: Simon Gomizelj<simongmzlj at gmail.com>
> ---
> scripts/makepkg.sh.in | 84 +++++++++++++++++++++++++++++++++++++++++++++++--
> 1 files changed, 81 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index b3081fc..348cb78 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -61,6 +61,7 @@ INFAKEROOT=0
> GENINTEG=0
> SKIPINTEG=0
> INSTALL=0
> +REPOADD=0
> NOBUILD=0
> NODEPS=0
> NOEXTRACT=0
> @@ -401,6 +402,33 @@ run_pacman() {
> eval "$cmd"
> }
>
> +run_repocp() {
> + local cmd
> + printf -v cmd "%q " "cp" -t $REPO_PATH "$@"
> + warning "$cmd"
> + if (( ! ASROOT )); then
> + if type -p sudo>/dev/null; then
> + cmd="sudo $cmd"
> + else
> + cmd="su root -c '$cmd'"
> + fi
> + fi
> + eval "$cmd"
> +}
> +
> +run_repoadd() {
> + local cmd
> + printf -v cmd "%q " "$REPO_ADD" $REPO_ADD_OPTS "$@"
> + if (( ! ASROOT )); then
> + if type -p sudo>/dev/null; then
> + cmd="sudo $cmd"
> + else
> + cmd="su root -c '$cmd'"
> + fi
> + fi
> + eval "$cmd"
> +}
I think there is no need for these two functions. If a user is
managing a repo on their machine, we can just assume they have write
permissions for that repo. In fact, we can just test that early on and
error if the permissions do not allow this.
> check_deps() {
> (( $#> 0 )) || return 0
>
> @@ -1241,6 +1269,39 @@ install_package() {
> fi
> }
>
> +repoadd_package() {
> + (( ! REPOADD ))&& return
> +
> + if (( ! SPLITPKG )); then
> + msg "$(gettext "Adding package %s to repo %s with %s...")"
> "$pkgname" "$REPO_DB" "$REPO_ADD"
> + else
> + msg "$(gettext "Adding %s package group to repo with %s...")"
> "$pkgbase" "$REPO_DB" "$REPO_ADD"
> + fi
> +
> + local fullver pkg pkglist repolist
> + for pkg in ${pkgname[@]}; do
> + # TODO: this wasn't properly fixed in commit 2020e629
> + fullver=$(get_full_version $epoch $pkgver $pkgrel)
> + if [[ -f $PKGDEST/${pkg}-${fullver}-${CARCH}${PKGEXT} ]]; then
> + pkglist+=" $PKGDEST/${pkg}-${fullver}-${CARCH}${PKGEXT}"
> + repolist+=" $REPO_PATH/${pkg}-${fullver}-${CARCH}${PKGEXT}"
> + else
> + pkglist+=" $REPO_PATH/${pkg}-${fullver}-any${PKGEXT}"
That does not look right... but I think this will get cleaned up with
removing the sudo/su checks above.
> + repolist+=" $REPO_PATH/${pkg}-${fullver}-any${PKGEXT}"
> + fi
> + done
> +
> + if ! run_repocp $pkglist; then
> + warning "$(gettext "Failed to copy package(s) to %s." "$REPO_PATH")"
> + return 0
> + fi
> +
> + if ! run_repoadd $REPO_PATH/$REPO_DB $repolist; then
> + warning "$(gettext "Failed to add package(s) to repo %s." "$REPO_DB")"
> + return 0
> + fi
> +}
> +
> check_sanity() {
> # check for no-no's in the build script
> local i
> @@ -1609,6 +1670,7 @@ usage() {
> echo "$(gettext " -g, --geninteg Generate integrity checks for
> source files")"
> echo "$(gettext " -h, --help Show this help message and exit")"
> echo "$(gettext " -i, --install Install package after
> successful build")"
> + echo "$(gettext " -a, --repoadd Add the package to a repo
> after successful build")"
This should alphabetically ordered. Again, I would stick to just
--repo-add.
> echo "$(gettext " -L, --log Log package build process")"
> echo "$(gettext " -m, --nocolor Disable colorized output messages")"
> echo "$(gettext " -o, --nobuild Download and extract files only")"
> @@ -1659,10 +1721,10 @@ fi
> ARGLIST=("$@")
>
> # Parse Command Line Options.
> -OPT_SHORT="AcCdefFghiLmop:rRsV"
> +OPT_SHORT="AcCdefFghiaLmop:rRsV"
> OPT_LONG="allsource,asroot,ignorearch,check,clean,cleancache,nodeps"
> OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver"
> -OPT_LONG+=",install,key:,log,nocolor,nobuild,nocheck,nosign,pkg:,rmdeps"
> +OPT_LONG+=",install,repoadd,key:,log,nocolor,nobuild,nocheck,nosign,pkg:,rmdeps"
> OPT_LONG+=",repackage,skipinteg,sign,source,syncdeps,version,config:"
> # Pacman Options
> OPT_LONG+=",noconfirm,noprogressbar"
> @@ -1697,6 +1759,7 @@ while true; do
> -g|--geninteg) GENINTEG=1 ;;
> --holdver) HOLDVER=1 ;;
> -i|--install) INSTALL=1 ;;
> + -a|--repoadd) REPOADD=1 ;;
> --key) shift; GPGKEY=$1 ;;
> -L|--log) LOGGING=1 ;;
> -m|--nocolor) USE_COLOR='n' ;;
> @@ -1749,6 +1812,9 @@ fi
> # set pacman command if not already defined
> PACMAN=${PACMAN:-pacman}
>
> +# set repoadd command if not already defined
> +REPO_ADD=${REPO_ADD:-repo-add}
> +
> # check if messages are to be printed using color
> unset ALL_OFF BOLD BLUE GREEN RED YELLOW
> if [[ -t 2&& ! $USE_COLOR = "n"&& $(check_buildenv color) = "y" ]]; then
> @@ -1970,6 +2036,10 @@ if (( ! SPLITPKG )); then
> warning "$(gettext "A package has already been built,
> installing existing package...")"
> install_package
> exit $?
> + elif (( REPOADD )); then
elif? I'm sure we can both install and add to a repo.
> + warning "$(gettext "A package has already been built,
> adding existing package...")"
> + repoadd_package
> + exit $?
> else
> error "$(gettext "A package has already been built. (use
> -f to overwrite)")"
> exit 1
> @@ -1994,6 +2064,10 @@ else
> warning "$(gettext "The package group has already
> been built, installing existing packages...")"
> install_package
> exit $?
> + elif (( REPOADD )); then
and again
> + warning "$(gettext "The package group has already
> been built, adding existing packages...")"
> + repoadd_package
> + exit $?
> else
> error "$(gettext "The package group has already been
> built. (use -f to overwrite)")"
> exit 1
> @@ -2174,7 +2248,11 @@ fi
> fullver=$(get_full_version $epoch $pkgver $pkgrel)
> msg "$(gettext "Finished making: %s")" "$pkgbase $fullver ($(date))"
>
> -install_package
> +if (( INSTALL )); then
> + install_package
> +elif (( REPOADD )); then
> + repoadd_package
> +fi
Again, I do not understand the if/elif there at all...
Overall, this looks a good first patch to me. Follow up the comments
here and then we can get the documentation added and this looks a nice
feature to add.
Allan
More information about the pacman-dev
mailing list