[PATCH 1/2] makepkg: move pacman calls to a function

[Cedric Staniewski]

Signed-off-by: Cedric Staniewski <cedric@...>
---
I'm not sure about that part

pmout=$(run_pacman -T "$@")

for two reasons. First, we have to work around sudo in run_pacman, and
second, most of the pacman wrappers does not support -T or do not return
the same codes as pacman does. For this second reason, the following patch
would be (currently) useless for the majority of pacman wrapper users.


 scripts/makepkg.sh.in |   39 +++++++++++++++------------------------
 1 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index fbcdc70..cfc5736 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -328,10 +328,21 @@ download_file() {
  fi
 }
 
+run_pacman() {
+ local ret=0
+ if (( ! ASROOT )) && [[ $1 != -T ]]; then
+ sudo pacman $PACMAN_OPTS "$@" || ret=$?
+ else
+ pacman $PACMAN_OPTS "$@" || ret=$?
+ fi
+ return $ret
+}
+
 check_deps() {
  [ $# -gt 0 ] || return
 
- pmout=$(pacman $PACMAN_OPTS -T "$@")
+ local ret=0
+ pmout=$(run_pacman -T "$@")
  ret=$?
  if [ $ret -eq 127 ]; then #unresolved deps
  echo "$pmout"
@@ -356,15 +367,8 @@ handle_deps() {
  if [ "$DEP_BIN" -eq 1 ]; then
  # install missing deps from binary packages (using pacman -S)
  msg "$(gettext "Installing missing dependencies...")"
- local ret=0
-
- if [ "$ASROOT" -eq 0 ]; then
- sudo pacman $PACMAN_OPTS -S --asdeps $deplist || ret=$?
- else
- pacman $PACMAN_OPTS -S --asdeps $deplist || ret=$?
- fi
 
- if [ $ret -ne 0 ]; then
+ if ! run_pacman -S --asdeps $deplist; then
  error "$(gettext "Pacman failed to install missing dependencies.")"
  exit 1 # TODO: error code
  fi
@@ -422,15 +426,9 @@ remove_deps() {
  done
 
  msg "Removing installed dependencies..."
- local ret=0
- if [ "$ASROOT" -eq 0 ]; then
- sudo pacman $PACMAN_OPTS -Rns $deplist || ret=$?
- else
- pacman $PACMAN_OPTS -Rns $deplist || ret=$?
- fi
 
  # Fixes FS#10039 - exit cleanly as package has built successfully
- if [ $ret -ne 0 ]; then
+ if ! run_pacman -Rns $deplist; then
  warning "$(gettext "Failed to remove installed dependencies.")"
  return 0
  fi
@@ -1113,14 +1111,7 @@ install_package() {
  fi
  done
 
- local ret=0
- if [ "$ASROOT" -eq 0 ]; then
- sudo pacman $PACMAN_OPTS -U ${pkglist} || ret=$?
- else
- pacman $PACMAN_OPTS -U ${pkglist} || ret=$?
- fi
-
- if [ $ret -ne 0 ]; then
+ if ! run_pacman -U $pkglist; then
  warning "$(gettext "Failed to install built package(s).")"
  return 0
  fi
--
1.6.5.2

[Cedric Staniewski]

Implements FS#13028.

Signed-off-by: Cedric Staniewski <cedric@...>
---
 doc/makepkg.conf.5.txt |    6 ++++++
 scripts/makepkg.sh.in  |   20 ++++++++++++--------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/doc/makepkg.conf.5.txt b/doc/makepkg.conf.5.txt
index 704dccc..617ec84 100644
--- a/doc/makepkg.conf.5.txt
+++ b/doc/makepkg.conf.5.txt
@@ -178,6 +178,12 @@ Options
 *PKGEXT*, *SRCEXT*::
  Do not touch these unless you know what you are doing.
 
+*PACMAN*::
+ The command which will be used to check for missing dependencies and to
+ install and remove packages. Pacman's -U, -T, -S and -Rns operations
+ must be supported by this command. If no command is specified, makepkg
+ will fall back to `pacman'.
+
 
 See Also
 --------
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index cfc5736..cf2b001 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -77,6 +77,7 @@ SPLITPKG=0
 # when dealing with svn/cvs/etc PKGBUILDs.
 FORCE_VER=""
 
+PACMAN=
 PACMAN_OPTS=
 
 ### SUBROUTINES ###
@@ -331,9 +332,9 @@ download_file() {
 run_pacman() {
  local ret=0
  if (( ! ASROOT )) && [[ $1 != -T ]]; then
- sudo pacman $PACMAN_OPTS "$@" || ret=$?
+ sudo $PACMAN $PACMAN_OPTS "$@" || ret=$?
  else
- pacman $PACMAN_OPTS "$@" || ret=$?
+ $PACMAN $PACMAN_OPTS "$@" || ret=$?
  fi
  return $ret
 }
@@ -347,7 +348,7 @@ check_deps() {
  if [ $ret -eq 127 ]; then #unresolved deps
  echo "$pmout"
  elif [ $ret -ne 0 ]; then
- error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout"
+ error "$(gettext "%s returned a fatal error (%i): %s")" "$PACMAN" "$ret" "$pmout"
  exit 1
  fi
 }
@@ -369,7 +370,7 @@ handle_deps() {
  msg "$(gettext "Installing missing dependencies...")"
 
  if ! run_pacman -S --asdeps $deplist; then
- error "$(gettext "Pacman failed to install missing dependencies.")"
+ error "$(gettext "%s failed to install missing dependencies.")" "$PACMAN"
  exit 1 # TODO: error code
  fi
  fi
@@ -1097,9 +1098,9 @@ install_package() {
  [ "$INSTALL" -eq 0 ] && return
 
  if [ "$SPLITPKG" -eq 0 ]; then
- msg "$(gettext "Installing package ${pkgname} with pacman -U...")"
+ msg "$(gettext "Installing package %s with %s -U...")" "$pkgname" "$PACMAN"
  else
- msg "$(gettext "Installing ${pkgbase} package group with pacman -U...")"
+ msg "$(gettext "Installing %s package group with %s -U...")" "$pkgbase" "$PACMAN"
  fi
 
  local pkglist
@@ -1550,6 +1551,9 @@ if [ -r ~/.makepkg.conf ]; then
  source ~/.makepkg.conf
 fi
 
+# set pacman command if not defined in config files
+PACMAN=${PACMAN:-pacman}
+
 # check if messages are to be printed using color
 unset ALL_OFF BOLD BLUE GREEN RED YELLOW
 if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then
@@ -1796,7 +1800,7 @@ if [ "$NODEPS" -eq 1 -o "$NOBUILD" -eq 1 -o "$REPKG" -eq 1 ]; then
  if [ "$NODEPS" -eq 1 ]; then
  warning "$(gettext "Skipping dependency checks.")"
  fi
-elif [ $(type -p pacman) ]; then
+elif [ $(type -p "$PACMAN") ]; then
  unset pkgdeps # Set by resolve_deps() and used by remove_deps()
  deperr=0
 
@@ -1811,7 +1815,7 @@ elif [ $(type -p pacman) ]; then
  exit 1
  fi
 else
- warning "$(gettext "pacman was not found in PATH; skipping dependency checks.")"
+ warning "$(gettext "%s was not found in PATH; skipping dependency checks.")" "$PACMAN"
 fi
 
 # ensure we have a sane umask set
--
1.6.5.2

[Xavier Chantry-2]

On Fri, Nov 6, 2009 at 8:47 PM, Cedric Staniewski <cedric@...> wrote:

> Signed-off-by: Cedric Staniewski <cedric@...>
> ---
> I'm not sure about that part
>
> pmout=$(run_pacman -T "$@")
>
> for two reasons. First, we have to work around sudo in run_pacman, and
> second, most of the pacman wrappers does not support -T or do not return
> the same codes as pacman does. For this second reason, the following patch
> would be (currently) useless for the majority of pacman wrapper users.
>
>

This comment makes more sense after reading your second patch :)
where you make run_pacman use a specified $PACMAN binary

I think its fine to keep calling pacman directly for -T operation, and
allow a wrapper for the others.

[Cedric Staniewski]

Xavier wrote:

> On Fri, Nov 6, 2009 at 8:47 PM, Cedric Staniewski <cedric@...> wrote:
>> Signed-off-by: Cedric Staniewski <cedric@...>
>> ---
>> I'm not sure about that part
>>
>> pmout=$(run_pacman -T "$@")
>>
>> for two reasons. First, we have to work around sudo in run_pacman, and
>> second, most of the pacman wrappers does not support -T or do not return
>> the same codes as pacman does. For this second reason, the following patch
>> would be (currently) useless for the majority of pacman wrapper users.
>>
>>
>
> This comment makes more sense after reading your second patch :)
> where you make run_pacman use a specified $PACMAN binary
>
> I think its fine to keep calling pacman directly for -T operation, and
> allow a wrapper for the others.

I think it mostly depends on if the second patch gets applied or not. In any case, I will send patches without run_pacman -T.

[Cedric Staniewski]

Signed-off-by: Cedric Staniewski <cedric@...>
---
 scripts/makepkg.sh.in |   37 ++++++++++++++-----------------------
 1 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index fbcdc70..125b4e0 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -328,9 +328,20 @@ download_file() {
  fi
 }
 
+run_pacman() {
+ local ret=0
+ if (( ! ASROOT )); then
+ sudo pacman $PACMAN_OPTS "$@" || ret=$?
+ else
+ pacman $PACMAN_OPTS "$@" || ret=$?
+ fi
+ return $ret
+}
+
 check_deps() {
  [ $# -gt 0 ] || return
 
+ local ret=0
  pmout=$(pacman $PACMAN_OPTS -T "$@")
  ret=$?
  if [ $ret -eq 127 ]; then #unresolved deps
@@ -356,15 +367,8 @@ handle_deps() {
  if [ "$DEP_BIN" -eq 1 ]; then
  # install missing deps from binary packages (using pacman -S)
  msg "$(gettext "Installing missing dependencies...")"
- local ret=0
-
- if [ "$ASROOT" -eq 0 ]; then
- sudo pacman $PACMAN_OPTS -S --asdeps $deplist || ret=$?
- else
- pacman $PACMAN_OPTS -S --asdeps $deplist || ret=$?
- fi
 
- if [ $ret -ne 0 ]; then
+ if ! run_pacman -S --asdeps $deplist; then
  error "$(gettext "Pacman failed to install missing dependencies.")"
  exit 1 # TODO: error code
  fi
@@ -422,15 +426,9 @@ remove_deps() {
  done
 
  msg "Removing installed dependencies..."
- local ret=0
- if [ "$ASROOT" -eq 0 ]; then
- sudo pacman $PACMAN_OPTS -Rns $deplist || ret=$?
- else
- pacman $PACMAN_OPTS -Rns $deplist || ret=$?
- fi
 
  # Fixes FS#10039 - exit cleanly as package has built successfully
- if [ $ret -ne 0 ]; then
+ if ! run_pacman -Rns $deplist; then
  warning "$(gettext "Failed to remove installed dependencies.")"
  return 0
  fi
@@ -1113,14 +1111,7 @@ install_package() {
  fi
  done
 
- local ret=0
- if [ "$ASROOT" -eq 0 ]; then
- sudo pacman $PACMAN_OPTS -U ${pkglist} || ret=$?
- else
- pacman $PACMAN_OPTS -U ${pkglist} || ret=$?
- fi
-
- if [ $ret -ne 0 ]; then
+ if ! run_pacman -U $pkglist; then
  warning "$(gettext "Failed to install built package(s).")"
  return 0
  fi
--
1.6.5.2

[Cedric Staniewski]

Implements FS#13028.

Signed-off-by: Cedric Staniewski <cedric@...>
---
 doc/makepkg.conf.5.txt |    6 ++++++
 scripts/makepkg.sh.in  |   20 ++++++++++++--------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/doc/makepkg.conf.5.txt b/doc/makepkg.conf.5.txt
index 704dccc..617ec84 100644
--- a/doc/makepkg.conf.5.txt
+++ b/doc/makepkg.conf.5.txt
@@ -178,6 +178,12 @@ Options
 *PKGEXT*, *SRCEXT*::
  Do not touch these unless you know what you are doing.
 
+*PACMAN*::
+ The command which will be used to check for missing dependencies and to
+ install and remove packages. Pacman's -U, -T, -S and -Rns operations
+ must be supported by this command. If no command is specified, makepkg
+ will fall back to `pacman'.
+
 
 See Also
 --------
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 125b4e0..9463601 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -77,6 +77,7 @@ SPLITPKG=0
 # when dealing with svn/cvs/etc PKGBUILDs.
 FORCE_VER=""
 
+PACMAN=
 PACMAN_OPTS=
 
 ### SUBROUTINES ###
@@ -331,9 +332,9 @@ download_file() {
 run_pacman() {
  local ret=0
  if (( ! ASROOT )); then
- sudo pacman $PACMAN_OPTS "$@" || ret=$?
+ sudo $PACMAN $PACMAN_OPTS "$@" || ret=$?
  else
- pacman $PACMAN_OPTS "$@" || ret=$?
+ $PACMAN $PACMAN_OPTS "$@" || ret=$?
  fi
  return $ret
 }
@@ -347,7 +348,7 @@ check_deps() {
  if [ $ret -eq 127 ]; then #unresolved deps
  echo "$pmout"
  elif [ $ret -ne 0 ]; then
- error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout"
+ error "$(gettext "%s returned a fatal error (%i): %s")" "$PACMAN" "$ret" "$pmout"
  exit 1
  fi
 }
@@ -369,7 +370,7 @@ handle_deps() {
  msg "$(gettext "Installing missing dependencies...")"
 
  if ! run_pacman -S --asdeps $deplist; then
- error "$(gettext "Pacman failed to install missing dependencies.")"
+ error "$(gettext "%s failed to install missing dependencies.")" "$PACMAN"
  exit 1 # TODO: error code
  fi
  fi
@@ -1097,9 +1098,9 @@ install_package() {
  [ "$INSTALL" -eq 0 ] && return
 
  if [ "$SPLITPKG" -eq 0 ]; then
- msg "$(gettext "Installing package ${pkgname} with pacman -U...")"
+ msg "$(gettext "Installing package %s with %s -U...")" "$pkgname" "$PACMAN"
  else
- msg "$(gettext "Installing ${pkgbase} package group with pacman -U...")"
+ msg "$(gettext "Installing %s package group with %s -U...")" "$pkgbase" "$PACMAN"
  fi
 
  local pkglist
@@ -1550,6 +1551,9 @@ if [ -r ~/.makepkg.conf ]; then
  source ~/.makepkg.conf
 fi
 
+# set pacman command if not defined in config files
+PACMAN=${PACMAN:-pacman}
+
 # check if messages are to be printed using color
 unset ALL_OFF BOLD BLUE GREEN RED YELLOW
 if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then
@@ -1796,7 +1800,7 @@ if [ "$NODEPS" -eq 1 -o "$NOBUILD" -eq 1 -o "$REPKG" -eq 1 ]; then
  if [ "$NODEPS" -eq 1 ]; then
  warning "$(gettext "Skipping dependency checks.")"
  fi
-elif [ $(type -p pacman) ]; then
+elif [ $(type -p "$PACMAN") ]; then
  unset pkgdeps # Set by resolve_deps() and used by remove_deps()
  deperr=0
 
@@ -1811,7 +1815,7 @@ elif [ $(type -p pacman) ]; then
  exit 1
  fi
 else
- warning "$(gettext "pacman was not found in PATH; skipping dependency checks.")"
+ warning "$(gettext "%s was not found in PATH; skipping dependency checks.")" "$PACMAN"
 fi
 
 # ensure we have a sane umask set
--
1.6.5.2

[Cedric Staniewski]

Implements FS#13028.

Signed-off-by: Cedric Staniewski <cedric@...>
---
Oops... forgot to adjust the documentation. -T support is not required
anymore for the pacman wrappers.

 doc/makepkg.conf.5.txt |    5 +++++
 scripts/makepkg.sh.in  |   20 ++++++++++++--------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/doc/makepkg.conf.5.txt b/doc/makepkg.conf.5.txt
index 704dccc..c23aaf9 100644
--- a/doc/makepkg.conf.5.txt
+++ b/doc/makepkg.conf.5.txt
@@ -178,6 +178,11 @@ Options
 *PKGEXT*, *SRCEXT*::
  Do not touch these unless you know what you are doing.
 
+*PACMAN*::
+ The command which will be used to install and remove packages. Pacman's
+ -U, -S and -Rns operations must be supported by this command. If no
+ command is specified, makepkg will fall back to `pacman'.
+
 
 See Also
 --------
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 125b4e0..9463601 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -77,6 +77,7 @@ SPLITPKG=0
 # when dealing with svn/cvs/etc PKGBUILDs.
 FORCE_VER=""
 
+PACMAN=
 PACMAN_OPTS=
 
 ### SUBROUTINES ###
@@ -331,9 +332,9 @@ download_file() {
 run_pacman() {
  local ret=0
  if (( ! ASROOT )); then
- sudo pacman $PACMAN_OPTS "$@" || ret=$?
+ sudo $PACMAN $PACMAN_OPTS "$@" || ret=$?
  else
- pacman $PACMAN_OPTS "$@" || ret=$?
+ $PACMAN $PACMAN_OPTS "$@" || ret=$?
  fi
  return $ret
 }
@@ -347,7 +348,7 @@ check_deps() {
  if [ $ret -eq 127 ]; then #unresolved deps
  echo "$pmout"
  elif [ $ret -ne 0 ]; then
- error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout"
+ error "$(gettext "%s returned a fatal error (%i): %s")" "$PACMAN" "$ret" "$pmout"
  exit 1
  fi
 }
@@ -369,7 +370,7 @@ handle_deps() {
  msg "$(gettext "Installing missing dependencies...")"
 
  if ! run_pacman -S --asdeps $deplist; then
- error "$(gettext "Pacman failed to install missing dependencies.")"
+ error "$(gettext "%s failed to install missing dependencies.")" "$PACMAN"
  exit 1 # TODO: error code
  fi
  fi
@@ -1097,9 +1098,9 @@ install_package() {
  [ "$INSTALL" -eq 0 ] && return
 
  if [ "$SPLITPKG" -eq 0 ]; then
- msg "$(gettext "Installing package ${pkgname} with pacman -U...")"
+ msg "$(gettext "Installing package %s with %s -U...")" "$pkgname" "$PACMAN"
  else
- msg "$(gettext "Installing ${pkgbase} package group with pacman -U...")"
+ msg "$(gettext "Installing %s package group with %s -U...")" "$pkgbase" "$PACMAN"
  fi
 
  local pkglist
@@ -1550,6 +1551,9 @@ if [ -r ~/.makepkg.conf ]; then
  source ~/.makepkg.conf
 fi
 
+# set pacman command if not defined in config files
+PACMAN=${PACMAN:-pacman}
+
 # check if messages are to be printed using color
 unset ALL_OFF BOLD BLUE GREEN RED YELLOW
 if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then
@@ -1796,7 +1800,7 @@ if [ "$NODEPS" -eq 1 -o "$NOBUILD" -eq 1 -o "$REPKG" -eq 1 ]; then
  if [ "$NODEPS" -eq 1 ]; then
  warning "$(gettext "Skipping dependency checks.")"
  fi
-elif [ $(type -p pacman) ]; then
+elif [ $(type -p "$PACMAN") ]; then
  unset pkgdeps # Set by resolve_deps() and used by remove_deps()
  deperr=0
 
@@ -1811,7 +1815,7 @@ elif [ $(type -p pacman) ]; then
  exit 1
  fi
 else
- warning "$(gettext "pacman was not found in PATH; skipping dependency checks.")"
+ warning "$(gettext "%s was not found in PATH; skipping dependency checks.")" "$PACMAN"
 fi
 
 # ensure we have a sane umask set
--
1.6.5.2

[Dan McGee]

On Fri, Nov 6, 2009 at 2:51 PM, Cedric Staniewski <cedric@...> wrote:
> Implements FS#13028.
>
> Signed-off-by: Cedric Staniewski <cedric@...>
> ---
> Oops... forgot to adjust the documentation. -T support is not required
> anymore for the pacman wrappers.

I think I'd rather see this as an environment variable- wouldn't that
make more sense, as we wouldn't require it to be specified by the vast
majority of people that just want to use the default?

PACMAN=${PACMAN:-pacman} or whatever it is.

-Dan

[Dan McGee]

On Fri, Nov 6, 2009 at 2:20 PM, Cedric Staniewski <cedric@...> wrote:
> Signed-off-by: Cedric Staniewski <cedric@...>
> ---

Seems pretty reasonable to me; Allan, is this OK?

-Dan

[Cedric Staniewski]

Dan McGee wrote:

> On Fri, Nov 6, 2009 at 2:51 PM, Cedric Staniewski <cedric@...> wrote:
>> Implements FS#13028.
>>
>> Signed-off-by: Cedric Staniewski <cedric@...>
>> ---
>> Oops... forgot to adjust the documentation. -T support is not required
>> anymore for the pacman wrappers.
>
> I think I'd rather see this as an environment variable- wouldn't that
> make more sense, as we wouldn't require it to be specified by the vast
> majority of people that just want to use the default?
>
> PACMAN=${PACMAN:-pacman} or whatever it is.
>
> -Dan

Sorry Dan, but I do not get your point. This patch makes it possible to specify a pacman command in one of the makepkg.conf files, but it is not mandatory because of the reason you mentioned. I already use your line in my patch:

+# set pacman command if not defined in config files
+PACMAN=${PACMAN:-pacman}
+

It is, however, not possible to provide a pacman command via environment variable, because I think it makes more sense to store it in a config file.
So do you mean it should be possible to use an environment variable as well?

[Allan McRae-3]

Dan McGee wrote:
> On Fri, Nov 6, 2009 at 2:20 PM, Cedric Staniewski <cedric@...> wrote:
>> Signed-off-by: Cedric Staniewski <cedric@...>
>> ---
>
> Seems pretty reasonable to me; Allan, is this OK?
>

I have been thinking about this and its companion patch.  I like the
refactoring of the pacman call into the function but dislike not
replacing the "pacman -T" call with it.

If there is a config option for setting the "pacman" binary, and I have
program that replaces pacman (e.g. the one based on the python alpm
wrapper should work), then I should not need pacman on my system at all.

So I prefer the original version where the "pacman -T" call was replaced
too.

Allan

[Allan McRae-3]

Cedric Staniewski wrote:

> Dan McGee wrote:
>> On Fri, Nov 6, 2009 at 2:51 PM, Cedric Staniewski <cedric@...> wrote:
>>> Implements FS#13028.
>>>
>>> Signed-off-by: Cedric Staniewski <cedric@...>
>>> ---
>>> Oops... forgot to adjust the documentation. -T support is not required
>>> anymore for the pacman wrappers.
>> I think I'd rather see this as an environment variable- wouldn't that
>> make more sense, as we wouldn't require it to be specified by the vast
>> majority of people that just want to use the default?
>>
>> PACMAN=${PACMAN:-pacman} or whatever it is.
>>
>> -Dan
>
> Sorry Dan, but I do not get your point. This patch makes it possible to specify a pacman command in one of the makepkg.conf files, but it is not mandatory because of the reason you mentioned. I already use your line in my patch:
>
> +# set pacman command if not defined in config files
> +PACMAN=${PACMAN:-pacman}
> +
>
> It is, however, not possible to provide a pacman command via environment variable, because I think it makes more sense to store it in a config file.
> So do you mean it should be possible to use an environment variable as well?

I think Dan wants it to only be available through and environmental
variable and not in the makepkg.conf.  That should already be possible
with your patch (as you point out, the line Dan suggested is already there).

I have no real preference but am leaning towards it not being included
in pacman.conf given the usage is expected to be low.  If we are wrong
and it becomes very popular to use some wrapper, then we can always add
the config option later.

Allan

[Dan McGee]

On Tue, Nov 10, 2009 at 4:36 PM, Allan McRae <allan@...> wrote:

> Cedric Staniewski wrote:
>>
>> Dan McGee wrote:
>>>
>>> On Fri, Nov 6, 2009 at 2:51 PM, Cedric Staniewski <cedric@...> wrote:
>>>>
>>>> Implements FS#13028.
>>>>
>>>> Signed-off-by: Cedric Staniewski <cedric@...>
>>>> ---
>>>> Oops... forgot to adjust the documentation. -T support is not required
>>>> anymore for the pacman wrappers.
>>>
>>> I think I'd rather see this as an environment variable- wouldn't that
>>> make more sense, as we wouldn't require it to be specified by the vast
>>> majority of people that just want to use the default?
>>>
>>> PACMAN=${PACMAN:-pacman} or whatever it is.
>>>
>>> -Dan
>>
>> Sorry Dan, but I do not get your point. This patch makes it possible to
>> specify a pacman command in one of the makepkg.conf files, but it is not
>> mandatory because of the reason you mentioned. I already use your line in my
>> patch:
>>
>> +# set pacman command if not defined in config files
>> +PACMAN=${PACMAN:-pacman}
>> +
>>
>> It is, however, not possible to provide a pacman command via environment
>> variable, because I think it makes more sense to store it in a config file.
>> So do you mean it should be possible to use an environment variable as
>> well?
>
> I think Dan wants it to only be available through and environmental variable
> and not in the makepkg.conf.  That should already be possible with your
> patch (as you point out, the line Dan suggested is already there).
>
> I have no real preference but am leaning towards it not being included in
> pacman.conf given the usage is expected to be low.  If we are wrong and it
> becomes very popular to use some wrapper, then we can always add the config
> option later.

My suggestion was as Allan says; only do it in the environment. Of
course, since makepkg.conf is just sourced anyway, then to each his
own, but I'd rather not advertise it there at all.

Why? Things like $BROWSER, $EDITOR, etc. are pretty standard and
accepted variables. There is no reason that on an Arch system, $PACMAN
shouldn't join that list. This would address other things, such as
people always wanting to have it run with a certain command line flag,
or perhaps always use a pacman.static binary, or anything like that.
Maybe I'm making orange juice out of bananas, but yeah.

-Dan

[Cedric Staniewski]

Dan McGee wrote:

> On Tue, Nov 10, 2009 at 4:36 PM, Allan McRae <allan@...> wrote:
>> Cedric Staniewski wrote:
>>> Dan McGee wrote:
>>>> On Fri, Nov 6, 2009 at 2:51 PM, Cedric Staniewski <cedric@...> wrote:
>>>>> Implements FS#13028.
>>>>>
>>>>> Signed-off-by: Cedric Staniewski <cedric@...>
>>>>> ---
>>>>> Oops... forgot to adjust the documentation. -T support is not required
>>>>> anymore for the pacman wrappers.
>>>> I think I'd rather see this as an environment variable- wouldn't that
>>>> make more sense, as we wouldn't require it to be specified by the vast
>>>> majority of people that just want to use the default?
>>>>
>>>> PACMAN=${PACMAN:-pacman} or whatever it is.
>>>>
>>>> -Dan
>>> Sorry Dan, but I do not get your point. This patch makes it possible to
>>> specify a pacman command in one of the makepkg.conf files, but it is not
>>> mandatory because of the reason you mentioned. I already use your line in my
>>> patch:
>>>
>>> +# set pacman command if not defined in config files
>>> +PACMAN=${PACMAN:-pacman}
>>> +
>>>
>>> It is, however, not possible to provide a pacman command via environment
>>> variable, because I think it makes more sense to store it in a config file.
>>> So do you mean it should be possible to use an environment variable as
>>> well?
>> I think Dan wants it to only be available through and environmental variable
>> and not in the makepkg.conf.  That should already be possible with your
>> patch (as you point out, the line Dan suggested is already there).
>>
>> I have no real preference but am leaning towards it not being included in
>> pacman.conf given the usage is expected to be low.  If we are wrong and it
>> becomes very popular to use some wrapper, then we can always add the config
>> option later.
>
> My suggestion was as Allan says; only do it in the environment. Of
> course, since makepkg.conf is just sourced anyway, then to each his
> own, but I'd rather not advertise it there at all.
>
> Why? Things like $BROWSER, $EDITOR, etc. are pretty standard and
> accepted variables. There is no reason that on an Arch system, $PACMAN
> shouldn't join that list. This would address other things, such as
> people always wanting to have it run with a certain command line flag,
> or perhaps always use a pacman.static binary, or anything like that.
> Maybe I'm making orange juice out of bananas, but yeah.
>
> -Dan
>

I'm fine with your approach and it would definitely have some
advantages, however I prefer documenting it somewhere. My worry is that
we run into side-effects when someone has set this variable to something
weird for some obscure reasons, but perhaps I am a bit too pessimistic here.

[Cedric Staniewski]

Allan McRae wrote:

> Dan McGee wrote:
>> On Fri, Nov 6, 2009 at 2:20 PM, Cedric Staniewski <cedric@...> wrote:
>>> Signed-off-by: Cedric Staniewski <cedric@...>
>>> ---
>>
>> Seems pretty reasonable to me; Allan, is this OK?
>>
>
> I have been thinking about this and its companion patch.  I like the
> refactoring of the pacman call into the function but dislike not
> replacing the "pacman -T" call with it.
>
> If there is a config option for setting the "pacman" binary, and I have
> program that replaces pacman (e.g. the one based on the python alpm
> wrapper should work), then I should not need pacman on my system at all.
>
> So I prefer the original version where the "pacman -T" call was replaced
> too.
>

And leave it to the pacman wrapper authors to fix their programs? Sounds
good. :)
I also prefer the original patch, mainly because it seems 'cleaner' to
me, but being able to replace pacman completely on a system is a valid
reason, too.

[Xavier Chantry-2]

On Thu, Nov 12, 2009 at 2:08 PM, Cedric Staniewski <cedric@...> wrote:

>>
>> I have been thinking about this and its companion patch.  I like the
>> refactoring of the pacman call into the function but dislike not
>> replacing the "pacman -T" call with it.
>>
>> If there is a config option for setting the "pacman" binary, and I have
>> program that replaces pacman (e.g. the one based on the python alpm
>> wrapper should work), then I should not need pacman on my system at all.
>>
>> So I prefer the original version where the "pacman -T" call was replaced
>> too.
>>
>
> And leave it to the pacman wrapper authors to fix their programs? Sounds
> good. :)
> I also prefer the original patch, mainly because it seems 'cleaner' to
> me, but being able to replace pacman completely on a system is a valid
> reason, too.
>
>

Well, I am still not convinced.
Why would any wrapper have to care about pacman -T ?
This is a hidden / undocumented / internal argument just for the usage
of makepkg.

In the best case, a wrapper will just forward it correctly. In the
worst case, it will break it.

[Dan McGee]

On Thu, Nov 12, 2009 at 7:48 AM, Xavier <shiningxc@...> wrote:

> On Thu, Nov 12, 2009 at 2:08 PM, Cedric Staniewski <cedric@...> wrote:
>>>
>>> I have been thinking about this and its companion patch.  I like the
>>> refactoring of the pacman call into the function but dislike not
>>> replacing the "pacman -T" call with it.
>>>
>>> If there is a config option for setting the "pacman" binary, and I have
>>> program that replaces pacman (e.g. the one based on the python alpm
>>> wrapper should work), then I should not need pacman on my system at all.
>>>
>>> So I prefer the original version where the "pacman -T" call was replaced
>>> too.
>>>
>>
>> And leave it to the pacman wrapper authors to fix their programs? Sounds
>> good. :)
>> I also prefer the original patch, mainly because it seems 'cleaner' to
>> me, but being able to replace pacman completely on a system is a valid
>> reason, too.
>>
>>
>
> Well, I am still not convinced.
> Why would any wrapper have to care about pacman -T ?
> This is a hidden / undocumented / internal argument just for the usage
> of makepkg.

Doesn't look undocumented to me:
       -T, --deptest
           Check dependencies; this is useful in scripts such as makepkg to
           check installed packages. This operation will check each dependency
           specified and return a list of those which are not currently
           satisfied on the system. This operation accepts no other options.
           Example usage: pacman -T qt "bash>=3.2".


> In the best case, a wrapper will just forward it correctly. In the
> worst case, it will break it.

[Xavier Chantry-2]

On Thu, Nov 12, 2009 at 2:51 PM, Dan McGee <dpmcgee@...> wrote:

>
> Doesn't look undocumented to me:
>       -T, --deptest
>           Check dependencies; this is useful in scripts such as makepkg to
>           check installed packages. This operation will check each dependency
>           specified and return a list of those which are not currently
>           satisfied on the system. This operation accepts no other options.
>           Example usage: pacman -T qt "bash>=3.2".
>
>

Ahah ok. Well I am still not sure what a wrapper could add to that
functionality, besides breaking it. But if everyone is against me,
then I will shut up :) It is not a big deal.

[Cedric Staniewski]

Xavier wrote:

> On Thu, Nov 12, 2009 at 2:08 PM, Cedric Staniewski <cedric@...> wrote:
>>> I have been thinking about this and its companion patch.  I like the
>>> refactoring of the pacman call into the function but dislike not
>>> replacing the "pacman -T" call with it.
>>>
>>> If there is a config option for setting the "pacman" binary, and I have
>>> program that replaces pacman (e.g. the one based on the python alpm
>>> wrapper should work), then I should not need pacman on my system at all.
>>>
>>> So I prefer the original version where the "pacman -T" call was replaced
>>> too.
>>>
>> And leave it to the pacman wrapper authors to fix their programs? Sounds
>> good. :)
>> I also prefer the original patch, mainly because it seems 'cleaner' to
>> me, but being able to replace pacman completely on a system is a valid
>> reason, too.
>>
>>
>
> Well, I am still not convinced.
> Why would any wrapper have to care about pacman -T ?
> This is a hidden / undocumented / internal argument just for the usage
> of makepkg.
>
> In the best case, a wrapper will just forward it correctly. In the
> worst case, it will break it.

Since pacman 3.3 it is not that hidden anymore[1].

[1]
http://projects.archlinux.org/pacman.git/commit/?id=9af9c0f328094228fa363d842ddc9b2a605f0d22

[Cedric Staniewski]

Xavier wrote:

> On Thu, Nov 12, 2009 at 2:51 PM, Dan McGee <dpmcgee@...> wrote:
>> Doesn't look undocumented to me:
>>       -T, --deptest
>>           Check dependencies; this is useful in scripts such as makepkg to
>>           check installed packages. This operation will check each dependency
>>           specified and return a list of those which are not currently
>>           satisfied on the system. This operation accepts no other options.
>>           Example usage: pacman -T qt "bash>=3.2".
>>
>>
>
> Ahah ok. Well I am still not sure what a wrapper could add to that
> functionality, besides breaking it. But if everyone is against me,
> then I will shut up :) It is not a big deal.

It would be possible to get rid of -T and depend on vercmp for the
installed dependency check. I am not sure if this is any better though
(probably rather worse).