diff mbox

[pacman-dev] Fix using run_pacman to invoke -Qi with sudo

Message ID 20180515151309.8370-1-eschwartz@archlinux.org
State Accepted, archived
Headers show

Commit Message

Eli Schwartz May 15, 2018, 3:13 p.m. UTC
In commit 5698d7b66daa2a0bc99cab7a989cef1c806c3bf6 a new non-root use of
pacman was added -- previously we used -T or -Qq, and run_pacman did not
know how to special-case -Qi to skip being prepended with sudo.

The result is:

  -> Generating .BUILDINFO file...
ERROR: ld.so: object 'libfakeroot.so' from LD_PRELOAD cannot be preloaded (cannot open shared object file): ignored.
[sudo] password for eschwartz:
  -> Adding changelog file...

Fix this by using a more generic glob since neither -Q nor -T will ever
need sudo or PACMAN_OPTS

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
---
 scripts/makepkg.sh.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Dave Reisner May 15, 2018, 3:46 p.m. UTC | #1
On Tue, May 15, 2018 at 11:13:09AM -0400, Eli Schwartz wrote:
> In commit 5698d7b66daa2a0bc99cab7a989cef1c806c3bf6 a new non-root use of
> pacman was added -- previously we used -T or -Qq, and run_pacman did not
> know how to special-case -Qi to skip being prepended with sudo.

Can we just be explicit about when we do and don't need elevated
privileges rather than trying to guess? Surely the caller knows the
requirements a priori.

> The result is:
> 
>   -> Generating .BUILDINFO file...
> ERROR: ld.so: object 'libfakeroot.so' from LD_PRELOAD cannot be preloaded (cannot open shared object file): ignored.
> [sudo] password for eschwartz:
>   -> Adding changelog file...
> 
> Fix this by using a more generic glob since neither -Q nor -T will ever
> need sudo or PACMAN_OPTS
> 
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
>  scripts/makepkg.sh.in | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 62b912e3..e9080a70 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -220,12 +220,12 @@ missing_source_file() {
>  
>  run_pacman() {
>  	local cmd
> -	if [[ $1 != -@(T|Qq) ]]; then
> +	if [[ $1 != -@(T|Q)*([[:alpha:]]) ]]; then
>  		cmd=("$PACMAN_PATH" "${PACMAN_OPTS[@]}" "$@")
>  	else
>  		cmd=("$PACMAN_PATH" "$@")
>  	fi
> -	if [[ $1 != -@(T|Qq|Q) ]]; then
> +	if [[ $1 != -@(T|Q)*([[:alpha:]]) ]]; then
>  		if type -p sudo >/dev/null; then
>  			cmd=(sudo "${cmd[@]}")
>  		else
> -- 
> 2.17.0
Allan McRae May 16, 2018, 4:41 a.m. UTC | #2
On 16/05/18 01:46, Dave Reisner wrote:
> On Tue, May 15, 2018 at 11:13:09AM -0400, Eli Schwartz wrote:
>> In commit 5698d7b66daa2a0bc99cab7a989cef1c806c3bf6 a new non-root use of
>> pacman was added -- previously we used -T or -Qq, and run_pacman did not
>> know how to special-case -Qi to skip being prepended with sudo.
> 
> Can we just be explicit about when we do and don't need elevated
> privileges rather than trying to guess? Surely the caller knows the
> requirements a priori.

Are you thinking two functions? One for root, one not?   Or a helper
function as_root() that does the su/sudo part?  Or a function parameter
for root usage?

Either way, I think the current patch is fine for 5.1.  Bigger change
can happen later.

A
Eli Schwartz May 16, 2018, 10:50 a.m. UTC | #3
On 05/16/2018 12:41 AM, Allan McRae wrote:
> On 16/05/18 01:46, Dave Reisner wrote:
>> On Tue, May 15, 2018 at 11:13:09AM -0400, Eli Schwartz wrote:
>>> In commit 5698d7b66daa2a0bc99cab7a989cef1c806c3bf6 a new non-root use of
>>> pacman was added -- previously we used -T or -Qq, and run_pacman did not
>>> know how to special-case -Qi to skip being prepended with sudo.
>>
>> Can we just be explicit about when we do and don't need elevated
>> privileges rather than trying to guess? Surely the caller knows the
>> requirements a priori.
> 
> Are you thinking two functions? One for root, one not?   Or a helper
> function as_root() that does the su/sudo part?  Or a function parameter
> for root usage?
> 
> Either way, I think the current patch is fine for 5.1.  Bigger change
> can happen later.

My patch keeps to the spirit of how things are currently done, but for
the future, we should probably consider why it's designed the way it is now.

The function does three things:

- resolve "pacman" to $PACMAN

- interpolate options like --noprogressbar, --noconfirm, and --color=

- heuristically guess whether to try elevating to root.

#3 is currently broken. #2 is interesting because we sometimes don't do
it. Is there a reason for this?

dreisner initially suggested something along the lines of
https://paste.xinu.at/ldn/ with two functions, though as I pointed out
then, the function should be renamed to not clash with "pacman" itself.

An as_root helper should not be necessary, we don't need sudo for
anything except pacman syncing deps or installing built artifacts, so
we'd still need to have a run_pacman for #'s 1 & 2, which is quite indirect.

I don't favor the parameter-based approach... seems awkward to call it.
diff mbox

Patch

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 62b912e3..e9080a70 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -220,12 +220,12 @@  missing_source_file() {
 
 run_pacman() {
 	local cmd
-	if [[ $1 != -@(T|Qq) ]]; then
+	if [[ $1 != -@(T|Q)*([[:alpha:]]) ]]; then
 		cmd=("$PACMAN_PATH" "${PACMAN_OPTS[@]}" "$@")
 	else
 		cmd=("$PACMAN_PATH" "$@")
 	fi
-	if [[ $1 != -@(T|Qq|Q) ]]; then
+	if [[ $1 != -@(T|Q)*([[:alpha:]]) ]]; then
 		if type -p sudo >/dev/null; then
 			cmd=(sudo "${cmd[@]}")
 		else