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

Message ID 20180317212401.27429-3-robin@broda.me
State Changes Requested, archived
Headers show
Series Add Architecture information to .BUILDINFO | expand

Commit Message

Robin Broda March 17, 2018, 9:24 p.m. UTC
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 <robin@broda.me>
---
 doc/BUILDINFO.5.txt   |  2 +-
 scripts/makepkg.sh.in | 12 ++++++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

Eli Schwartz March 18, 2018, 1:19 a.m. UTC | #1
On 03/17/2018 05:24 PM, 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.

I'm not sure we can, without adding a dependency on e.g. expac. Which
would admittedly be a lot cleaner and we could also remove the existing
sed command.

The downside is depending on a pretty specific tool. The plus side is
ensuring that everyone has a pretty cool tool installed by default...

pacman itself does not really give output meant for parsing.

...

The alternative, reproduction-wise, is of course to brute force by
trying to access both "x86_64" and "any" (it would suffice to check the
return code of curl -IfLls -o /dev/null $url). I don't think this is a
terrible option, unless Allan is willing to accept expac-based patches...
Allan McRae March 18, 2018, 1:40 a.m. UTC | #2
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 <robin@broda.me>

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?

> +		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
>
Dave Reisner May 15, 2018, 2:46 p.m. UTC | #3
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 <robin@broda.me>
> 
> 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
> >
Robin Broda May 15, 2018, 2:58 p.m. UTC | #4
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

Patch

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