@keszybz commented on this pull request.


> @@ -0,0 +1,15 @@
+#!/bin/sh
+
+version_deps () {
+       while read dep
+       do
+               if [[ "${dep}" =~ ^[^\(]*\(\) ]] && rpm -q --whatprovides 
"${dep}" &> /dev/null
+               then
+                       printf "($dep with %s)\n" "$(rpm -q --whatprovides 
"${dep}" --qf '%{NAME} >= %{VERSION}')"
+               else
+                       printf "%s\n" "${dep}"
+               fi
+       done
+}
+
+/usr/lib/rpm/find-requires | version_deps

Shouldn't this be %{_rpmconfigdir}/find-requires, i.e. without a hardcoded path?

Without `set -o pipefail`, this will ignore the return value from 
`find-requires`, which seems iffy.

> @@ -0,0 +1,15 @@
+#!/bin/sh
+
+version_deps () {
+       while read dep
+       do
+               if [[ "${dep}" =~ ^[^\(]*\(\) ]] && rpm -q --whatprovides 
"${dep}" &> /dev/null

`[[ =~ ]]` is a bash syntax, but the file is declared as pure-sh.

I think it's it's nicer to avoid a space after the redirection operator.

The conditional is not robust wrt. to failure. If `rpm -q` fails, we just take 
the other branch. If that is intentional, it'd be nice to add a comment 
explaining that.

> @@ -0,0 +1,15 @@
+#!/bin/sh
+
+version_deps () {
+       while read dep
+       do
+               if [[ "${dep}" =~ ^[^\(]*\(\) ]] && rpm -q --whatprovides 
"${dep}" &> /dev/null
+               then
+                       printf "($dep with %s)\n" "$(rpm -q --whatprovides 
"${dep}" --qf '%{NAME} >= %{VERSION}')"

Why not make this a separate dependency, i.e. keep the original dep as it was, 
and just add a separate version dep?

I think `%{?_isa}` should be used, i.e. the dependency should specify the arch, 
so that a package for another arch is not used to satisfy the dep 
inadvertently. 

-- 
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2372#pullrequestreview-1274223226
You are receiving this because you are subscribed to this thread.

Message ID: <rpm-software-management/rpm/pull/2372/review/1274223...@github.com>
_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Reply via email to