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

2018-05-15 Thread Allan McRae 
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

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

2018-05-15 Thread Dave Reisner 
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 
> ---
>  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

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

2018-05-15 Thread Eli Schwartz 
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 
---
 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

Re: [pacman-dev] [PATCH 2/2] Append '-$arch' to 'installed' array in .BUILDINFO

2018-05-15 Thread Robin Broda 
On 05/15/2018 04:46 PM, Dave Reisner wrote:
> On Sun, Mar 18, 2018 at 11:40:53AM +1000, Allan McRae wrote:
>>
>> What makes this call to pacman not need to use run_pacman like the others?
>>
> 
> Answer: run_pacman calls sudo, which means that a bare 'makepkg' will
> require elevated privileges.
> 

Looking at 5698d7b66daa2a0bc99cab7a989cef1c806c3bf6 (1), `run_pacman` was
already used for a `pacman -Q` prior to this patch, and looking at
makepkg.sh.in l221 (2) it appears like run_pacman is currently whitelisting
a handful of options only.

This match should probably be improved

(1) 
https://git.archlinux.org/pacman.git/commit/?id=5698d7b66daa2a0bc99cab7a989cef1c806c3bf6
(2) https://git.archlinux.org/pacman.git/tree/scripts/makepkg.sh.in?id=HEAD#n221

-- 
Rob (coderobe)

O< ascii ribbon campaign - stop html mail - www.asciiribbon.org

Re: [pacman-dev] [PATCH 2/2] Append '-$arch' to 'installed' array in .BUILDINFO

2018-05-15 Thread Dave Reisner 
On Sun, Mar 18, 2018 at 11:40:53AM +1000, Allan McRae wrote:
> On 18/03/18 07:24, Robin Broda wrote:
> > This patch incurs a **severe** performance degradation when generating
> > the .BUILDINFO file, likely due to frequent usage of `pacman -Qi`
> > and `grep -E`. I haven't found a faster way to gather this information.
> > 
> > Signed-off-by: Robin Broda 
> 
> I will comment on the utility of this patch in another email.   This is
> just pointing out that it is broken...
> 
> > ---
> >  doc/BUILDINFO.5.txt   |  2 +-
> >  scripts/makepkg.sh.in | 12 ++--
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/doc/BUILDINFO.5.txt b/doc/BUILDINFO.5.txt
> > index 4734301e..2c74f9ff 100644
> > --- a/doc/BUILDINFO.5.txt
> > +++ b/doc/BUILDINFO.5.txt
> > @@ -61,7 +61,7 @@ BUILDINFO file format.
> >  
> >  *installed (array)*::
> > The installed packages at build time including the version information 
> > of
> > -   the package. Formatted as "$pkgname-$pkgver-$pkgrel".
> > +   the package. Formatted as "$pkgname-$pkgver-$pkgrel-$pkgarch".
> >  
> >  See Also
> >  
> > diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> > index ece53dca..10303417 100644
> > --- a/scripts/makepkg.sh.in
> > +++ b/scripts/makepkg.sh.in
> > @@ -694,8 +694,16 @@ write_buildinfo() {
> > write_kv_pair "buildenv" "${BUILDENV[@]}"
> > write_kv_pair "options" "${OPTIONS[@]}"
> >  
> > -   local pkglist=($(run_pacman -Q | sed "s# #-#"))
> > -   write_kv_pair "installed" "${pkglist[@]}"
> > +   local pkglist=($(run_pacman -Qq))
> > +   local installed=()
> > +   for pkg in "${pkglist[@]}"
> > +   do
> > +   pkginfo="$(pacman -Qi "${pkg}")"
> 
> What makes this call to pacman not need to use run_pacman like the others?
> 

Answer: run_pacman calls sudo, which means that a bare 'makepkg' will
require elevated privileges.

> > +   pkgver="$(grep -E '^Version' <<< "${pkginfo}" | tr -d ' ' | cut 
> > -d':' -f2-)"
> > +   pkgarch="$(grep -E '^Architecture' <<< "${pkginfo}" | tr -d ' ' 
> > | cut -d':' -f2-)"
> 
> Not every system runs in English...
> 
> > +   installed+=("${pkg}-${pkgver}-${pkgarch}")
> > +   done
> > +   write_kv_pair "installed" "${installed[@]}"
> >  }
> >  
> >  # build a sorted NUL-separated list of the full contents of the current
> >