[pacman-dev] makepkg: drop duplicate reporting of missing dependencies

Message ID 20200206124839.252508-1-dreisner@archlinux.org
State Accepted, archived
Headers show
Series [pacman-dev] makepkg: drop duplicate reporting of missing dependencies | expand

Commit Message

Dave Reisner Feb. 6, 2020, 12:48 p.m. UTC
When pacman fails to satisfy deps, we might see output like the
following:

==> Making package: spiderfoot 3.0-1 (Thu 06 Feb 2020 12:45:10 PM CET)
==> Checking runtime dependencies...
==> Installing missing dependencies...
error: target not found: python-pygexf
==> ERROR: 'pacman' failed to install missing dependencies.
==> Missing dependencies:
  -> python-dnspython
  -> python-exifread
  -> python-cherrypy
  -> python-beautifulsoup4
  -> python-netaddr
  -> python-pysocks
  -> python-ipwhois
  -> python-ipaddress
  -> python-phonenumbers
  -> python-pypdf2
  -> python-stem
  -> python-whois
  -> python-future
  -> python-pyopenssl
  -> python-docx
  -> python-pptx
  -> python-networkx
  -> python-cryptography
  -> python-secure
  -> python-pygexf
  -> python-adblockparser
==> Checking buildtime dependencies...
==> ERROR: Could not resolve all dependencies.

This is misleading -- the only truly missing package is python-pygexf,
but we fail to remove sync-able deps from our deplist and report
everything as if it were missing. Simply drop this extra reporting
because pacman already tells us exactly what couldn't be resolved.
---
I thought about trying to make this accurate and diff the lists --
something like:

  mapfile -t deplist < <(printf '%s\n' "${deplist}" | grep -vxFf <(run_pacman -Ssq))

but I'm not convinced this is really the right thing to do...

 scripts/makepkg.sh.in | 6 ------
 1 file changed, 6 deletions(-)

Comments

Allan McRae Feb. 12, 2020, 9:04 a.m. UTC | #1
On 6/2/20 10:48 pm, Dave Reisner wrote:
> When pacman fails to satisfy deps, we might see output like the
> following:
> 
> ==> Making package: spiderfoot 3.0-1 (Thu 06 Feb 2020 12:45:10 PM CET)
> ==> Checking runtime dependencies...
> ==> Installing missing dependencies...
> error: target not found: python-pygexf
> ==> ERROR: 'pacman' failed to install missing dependencies.
> ==> Missing dependencies:
>   -> python-dnspython
>   -> python-exifread
>   -> python-cherrypy
>   -> python-beautifulsoup4
>   -> python-netaddr
>   -> python-pysocks
>   -> python-ipwhois
>   -> python-ipaddress
>   -> python-phonenumbers
>   -> python-pypdf2
>   -> python-stem
>   -> python-whois
>   -> python-future
>   -> python-pyopenssl
>   -> python-docx
>   -> python-pptx
>   -> python-networkx
>   -> python-cryptography
>   -> python-secure
>   -> python-pygexf
>   -> python-adblockparser
> ==> Checking buildtime dependencies...
> ==> ERROR: Could not resolve all dependencies.
> 
> This is misleading -- the only truly missing package is python-pygexf,
> but we fail to remove sync-able deps from our deplist and report
> everything as if it were missing. Simply drop this extra reporting
> because pacman already tells us exactly what couldn't be resolved.
> ---
> I thought about trying to make this accurate and diff the lists --
> something like:
> 
>   mapfile -t deplist < <(printf '%s\n' "${deplist}" | grep -vxFf <(run_pacman -Ssq))
> 
> but I'm not convinced this is really the right thing to do...

I'm OK with the error reported as is after this patch.  No need for more
complications.

A
Allan McRae March 17, 2020, 4:09 a.m. UTC | #2
On 6/2/20 10:48 pm, Dave Reisner wrote:
> When pacman fails to satisfy deps, we might see output like the
> following:
> 
> ==> Making package: spiderfoot 3.0-1 (Thu 06 Feb 2020 12:45:10 PM CET)
> ==> Checking runtime dependencies...
> ==> Installing missing dependencies...
> error: target not found: python-pygexf
> ==> ERROR: 'pacman' failed to install missing dependencies.
> ==> Missing dependencies:
>   -> python-dnspython
>   -> python-exifread
>   -> python-cherrypy
>   -> python-beautifulsoup4
>   -> python-netaddr
>   -> python-pysocks
>   -> python-ipwhois
>   -> python-ipaddress
>   -> python-phonenumbers
>   -> python-pypdf2
>   -> python-stem
>   -> python-whois
>   -> python-future
>   -> python-pyopenssl
>   -> python-docx
>   -> python-pptx
>   -> python-networkx
>   -> python-cryptography
>   -> python-secure
>   -> python-pygexf
>   -> python-adblockparser
> ==> Checking buildtime dependencies...
> ==> ERROR: Could not resolve all dependencies.
> 
> This is misleading -- the only truly missing package is python-pygexf,
> but we fail to remove sync-able deps from our deplist and report
> everything as if it were missing. Simply drop this extra reporting
> because pacman already tells us exactly what couldn't be resolved.
> ---

It seems this went too far:

Before:

$ makepkg
==> Making package: pacman 5.2.1-4 (Tue 17 Mar 2020 14:05:51)
==> Checking runtime dependencies...
==> Missing dependencies:
  -> pacman-mirrorlist
==> Checking buildtime dependencies...
==> ERROR: Could not resolve all dependencies.

After:

$ makepkg
==> Making package: pacman 5.2.1-4 (Tue 17 Mar 2020 14:01:13)
==> Checking runtime dependencies...
==> Checking buildtime dependencies...
==> ERROR: Could not resolve all dependencies.

I have reverted on my patchqueue branch for the time being, but won't
push to master.  I need more time to look at this...

Allan


> I thought about trying to make this accurate and diff the lists --
> something like:
> 
>   mapfile -t deplist < <(printf '%s\n' "${deplist}" | grep -vxFf <(run_pacman -Ssq))
> 
> but I'm not convinced this is really the right thing to do...
> 
>  scripts/makepkg.sh.in | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 7fa791e1..bfbf165b 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -316,12 +316,6 @@ resolve_deps() {
>  		[[ -z $deplist ]] && return $R_DEPS_SATISFIED
>  	fi
>  
> -	msg "$(gettext "Missing dependencies:")"
> -	local dep
> -	for dep in ${deplist[@]}; do
> -		msg2 "$dep"
> -	done
> -
>  	return $R_DEPS_MISSING
>  }
>  
>
Eli Schwartz March 17, 2020, 4:37 a.m. UTC | #3
On 3/17/20 12:09 AM, Allan McRae wrote:
> On 6/2/20 10:48 pm, Dave Reisner wrote:
>> When pacman fails to satisfy deps, we might see output like the
>> following:
>>
>> ==> Making package: spiderfoot 3.0-1 (Thu 06 Feb 2020 12:45:10 PM CET)
>> ==> Checking runtime dependencies...
>> ==> Installing missing dependencies...
>> error: target not found: python-pygexf
>> ==> ERROR: 'pacman' failed to install missing dependencies.
>> ==> Missing dependencies:
>>   -> python-dnspython
>>   -> python-exifread
>>   -> python-cherrypy
>>   -> python-beautifulsoup4
>>   -> python-netaddr
>>   -> python-pysocks
>>   -> python-ipwhois
>>   -> python-ipaddress
>>   -> python-phonenumbers
>>   -> python-pypdf2
>>   -> python-stem
>>   -> python-whois
>>   -> python-future
>>   -> python-pyopenssl
>>   -> python-docx
>>   -> python-pptx
>>   -> python-networkx
>>   -> python-cryptography
>>   -> python-secure
>>   -> python-pygexf
>>   -> python-adblockparser
>> ==> Checking buildtime dependencies...
>> ==> ERROR: Could not resolve all dependencies.
>>
>> This is misleading -- the only truly missing package is python-pygexf,
>> but we fail to remove sync-able deps from our deplist and report
>> everything as if it were missing. Simply drop this extra reporting
>> because pacman already tells us exactly what couldn't be resolved.
>> ---
> 
> It seems this went too far:
> 
> Before:
> 
> $ makepkg
> ==> Making package: pacman 5.2.1-4 (Tue 17 Mar 2020 14:05:51)
> ==> Checking runtime dependencies...
> ==> Missing dependencies:
>   -> pacman-mirrorlist
> ==> Checking buildtime dependencies...
> ==> ERROR: Could not resolve all dependencies.
> 
> After:
> 
> $ makepkg
> ==> Making package: pacman 5.2.1-4 (Tue 17 Mar 2020 14:01:13)
> ==> Checking runtime dependencies...
> ==> Checking buildtime dependencies...
> ==> ERROR: Could not resolve all dependencies.
> 
> I have reverted on my patchqueue branch for the time being, but won't
> push to master.  I need more time to look at this...
> 
> Allan
Hrm, the idea here was to rely on handle_deps running pacman -S and
returning pacman's own error messages. But if -s is not invoked, then we
do still need to print it ourselves.

Actually, thinking about this I'm not even sure I understand the patch
in the first place. We're dealing with two errors here: one from pacman,
saying it could not find the sync'able dependency, and one from makepkg,
saying that due to pacman aborting, all of these packages are missing.

Where it might make a difference is if the user reads the error message
from makepkg -s:

==> Installing missing dependencies...
error: target not found: foo
==> ERROR: 'pacman' failed to install missing dependencies.
==> Missing dependencies:
  -> bar
  -> foo

And following that logic, goes and installs pacman -U ./something-which
provides foo, plus pacman -S bar, and then runs makepkg without -s

With this commit, they only install foo, then makepkg fails again due to
bar being missing.

Patch

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 7fa791e1..bfbf165b 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -316,12 +316,6 @@  resolve_deps() {
 		[[ -z $deplist ]] && return $R_DEPS_SATISFIED
 	fi
 
-	msg "$(gettext "Missing dependencies:")"
-	local dep
-	for dep in ${deplist[@]}; do
-		msg2 "$dep"
-	done
-
 	return $R_DEPS_MISSING
 }