diff mbox series

fedabipkgdiff: make --self-compare use abipkgdiff --self-check

Message ID 86r1ojqc00.fsf@redhat.com
State New
Headers show
Series fedabipkgdiff: make --self-compare use abipkgdiff --self-check | expand

Commit Message

Dodji Seketeli Nov. 24, 2020, 10:34 a.m. UTC
Hello,

Now that the abipkgdiff program supports the --self-check option to
make it compare the binaries in an RPM against their own ABIXML
representation, make the --self-compare option of fedabipkgdiff use
the abipkgdiff --self-check.

	* tools/fedabipkgdiff (abipkgdiff): If the user provides the
	--self-compare options, generate the abipkgdiff command by using
	the --self-check option.
	(run_abipkgdiff): Each return value of the abipkgidiff runs can be
	negative because they are unsigned values in essence, but as
	python doesn't seem to have a unsigned integer type.  So we need
	to consider the max of the absolute value of the return codes
	here.
	* tests/data/test-fedabipkgdiff/test7-self-compare-from-fc23-dbus-glib-report-0.txt:
	Adjust.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 ...7-self-compare-from-fc23-dbus-glib-report-0.txt |  6 ++++
 tools/fedabipkgdiff                                | 39 ++++++++++++++--------
 2 files changed, 31 insertions(+), 14 deletions(-)

Comments

Ben Woodard Nov. 24, 2020, 9:43 p.m. UTC | #1
I like the concept of fedabipkgdiff and abipkgdiff —self-check but I think that it really should NOT suppress harmless changes when doing a self-check. When comparing a library to itself “harmless” changes are indicative of changes not captured faithfully enough in either the abixml or the intermediate representation to represent the full ABI. The fact that in the context of comparing a library to itself libabigail concludes that these changes happen to be harmless is just luck.

-ben

> On Nov 24, 2020, at 2:34 AM, Dodji Seketeli via Libabigail <libabigail@sourceware.org> wrote:
> 
> Hello,
> 
> Now that the abipkgdiff program supports the --self-check option to
> make it compare the binaries in an RPM against their own ABIXML
> representation, make the --self-compare option of fedabipkgdiff use
> the abipkgdiff --self-check.
> 
> 	* tools/fedabipkgdiff (abipkgdiff): If the user provides the
> 	--self-compare options, generate the abipkgdiff command by using
> 	the --self-check option.
> 	(run_abipkgdiff): Each return value of the abipkgidiff runs can be
> 	negative because they are unsigned values in essence, but as
> 	python doesn't seem to have a unsigned integer type.  So we need
> 	to consider the max of the absolute value of the return codes
> 	here.
> 	* tests/data/test-fedabipkgdiff/test7-self-compare-from-fc23-dbus-glib-report-0.txt:
> 	Adjust.
> 
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
> ---
> ...7-self-compare-from-fc23-dbus-glib-report-0.txt |  6 ++++
> tools/fedabipkgdiff                                | 39 ++++++++++++++--------
> 2 files changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/tests/data/test-fedabipkgdiff/test7-self-compare-from-fc23-dbus-glib-report-0.txt b/tests/data/test-fedabipkgdiff/test7-self-compare-from-fc23-dbus-glib-report-0.txt
> index 63a6dca..e037e7e 100644
> --- a/tests/data/test-fedabipkgdiff/test7-self-compare-from-fc23-dbus-glib-report-0.txt
> +++ b/tests/data/test-fedabipkgdiff/test7-self-compare-from-fc23-dbus-glib-report-0.txt
> @@ -1,12 +1,18 @@
> Comparing the ABI of binaries between dbus-glib-0.106-1.fc23.i686.rpm and dbus-glib-0.106-1.fc23.i686.rpm:
> 
> +==== SELF CHECK SUCCEEDED for 'dbus-binding-tool' ====
> +==== SELF CHECK SUCCEEDED for 'libdbus-glib-1.so.2.3.3' ====
> 
> Comparing the ABI of binaries between dbus-glib-devel-0.106-1.fc23.i686.rpm and dbus-glib-devel-0.106-1.fc23.i686.rpm:
> 
> +==== SELF CHECK SUCCEEDED for 'dbus-bash-completion-helper' ====
> 
> Comparing the ABI of binaries between dbus-glib-0.106-1.fc23.x86_64.rpm and dbus-glib-0.106-1.fc23.x86_64.rpm:
> 
> +==== SELF CHECK SUCCEEDED for 'dbus-binding-tool' ====
> +==== SELF CHECK SUCCEEDED for 'libdbus-glib-1.so.2.3.3' ====
> 
> Comparing the ABI of binaries between dbus-glib-devel-0.106-1.fc23.x86_64.rpm and dbus-glib-devel-0.106-1.fc23.x86_64.rpm:
> 
> +==== SELF CHECK SUCCEEDED for 'dbus-bash-completion-helper' ====
> 
> diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff
> index 2191613..06ab573 100755
> --- a/tools/fedabipkgdiff
> +++ b/tools/fedabipkgdiff
> @@ -1118,19 +1118,30 @@ def abipkgdiff(cmp_half1, cmp_half2):
>     else:
>         debuginfo_pkg2 = format_debug_info_pkg_options("--d2", cmp_half2.ancillary_debug);
> 
> -    cmd = [
> -        abipkgdiff_tool,
> -        suppressions,
> -        '--show-identical-binaries' if global_config.show_identical_binaries else '',
> -        '--no-default-suppression' if global_config.no_default_suppr else '',
> -        '--dso-only' if global_config.dso_only else '',
> -        debuginfo_pkg1,
> -        debuginfo_pkg2,
> -        devel_pkg1,
> -        devel_pkg2,
> -        cmp_half1.subject.downloaded_file,
> -        cmp_half2.subject.downloaded_file,
> -    ]
> +    cmd = []
> +
> +    if global_config.self_compare:
> +        cmd = [
> +            abipkgdiff_tool,
> +            '--dso-only' if global_config.dso_only else '',
> +            '--self-check',
> +            debuginfo_pkg1,
> +            cmp_half1.subject.downloaded_file,
> +        ]
> +    else:
> +        cmd = [
> +            abipkgdiff_tool,
> +            suppressions,
> +            '--show-identical-binaries' if global_config.show_identical_binaries else '',
> +            '--no-default-suppression' if global_config.no_default_suppr else '',
> +            '--dso-only' if global_config.dso_only else '',
> +            debuginfo_pkg1,
> +            debuginfo_pkg2,
> +            devel_pkg1,
> +            devel_pkg2,
> +            cmp_half1.subject.downloaded_file,
> +            cmp_half2.subject.downloaded_file,
> +        ]
>     cmd = [s for s in cmd if s != '']
> 
>     if global_config.dry_run:
> @@ -1193,7 +1204,7 @@ def run_abipkgdiff(rpm_col1, rpm_col2):
>     return_codes = [
>         abipkgdiff(cmp_half1, cmp_half2) for cmp_half1, cmp_half2
>         in generate_comparison_halves(rpm_col1, rpm_col2)]
> -    return max(return_codes) if return_codes else 0
> +    return max(return_codes, key=abs) if return_codes else 0
> 
> 
> @log_call
> -- 
> 1.8.3.1
> 
> 
> -- 
> 		Dodji
>
diff mbox series

Patch

diff --git a/tests/data/test-fedabipkgdiff/test7-self-compare-from-fc23-dbus-glib-report-0.txt b/tests/data/test-fedabipkgdiff/test7-self-compare-from-fc23-dbus-glib-report-0.txt
index 63a6dca..e037e7e 100644
--- a/tests/data/test-fedabipkgdiff/test7-self-compare-from-fc23-dbus-glib-report-0.txt
+++ b/tests/data/test-fedabipkgdiff/test7-self-compare-from-fc23-dbus-glib-report-0.txt
@@ -1,12 +1,18 @@ 
 Comparing the ABI of binaries between dbus-glib-0.106-1.fc23.i686.rpm and dbus-glib-0.106-1.fc23.i686.rpm:
 
+==== SELF CHECK SUCCEEDED for 'dbus-binding-tool' ====
+==== SELF CHECK SUCCEEDED for 'libdbus-glib-1.so.2.3.3' ====
 
 Comparing the ABI of binaries between dbus-glib-devel-0.106-1.fc23.i686.rpm and dbus-glib-devel-0.106-1.fc23.i686.rpm:
 
+==== SELF CHECK SUCCEEDED for 'dbus-bash-completion-helper' ====
 
 Comparing the ABI of binaries between dbus-glib-0.106-1.fc23.x86_64.rpm and dbus-glib-0.106-1.fc23.x86_64.rpm:
 
+==== SELF CHECK SUCCEEDED for 'dbus-binding-tool' ====
+==== SELF CHECK SUCCEEDED for 'libdbus-glib-1.so.2.3.3' ====
 
 Comparing the ABI of binaries between dbus-glib-devel-0.106-1.fc23.x86_64.rpm and dbus-glib-devel-0.106-1.fc23.x86_64.rpm:
 
+==== SELF CHECK SUCCEEDED for 'dbus-bash-completion-helper' ====
 
diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff
index 2191613..06ab573 100755
--- a/tools/fedabipkgdiff
+++ b/tools/fedabipkgdiff
@@ -1118,19 +1118,30 @@  def abipkgdiff(cmp_half1, cmp_half2):
     else:
         debuginfo_pkg2 = format_debug_info_pkg_options("--d2", cmp_half2.ancillary_debug);
 
-    cmd = [
-        abipkgdiff_tool,
-        suppressions,
-        '--show-identical-binaries' if global_config.show_identical_binaries else '',
-        '--no-default-suppression' if global_config.no_default_suppr else '',
-        '--dso-only' if global_config.dso_only else '',
-        debuginfo_pkg1,
-        debuginfo_pkg2,
-        devel_pkg1,
-        devel_pkg2,
-        cmp_half1.subject.downloaded_file,
-        cmp_half2.subject.downloaded_file,
-    ]
+    cmd = []
+
+    if global_config.self_compare:
+        cmd = [
+            abipkgdiff_tool,
+            '--dso-only' if global_config.dso_only else '',
+            '--self-check',
+            debuginfo_pkg1,
+            cmp_half1.subject.downloaded_file,
+        ]
+    else:
+        cmd = [
+            abipkgdiff_tool,
+            suppressions,
+            '--show-identical-binaries' if global_config.show_identical_binaries else '',
+            '--no-default-suppression' if global_config.no_default_suppr else '',
+            '--dso-only' if global_config.dso_only else '',
+            debuginfo_pkg1,
+            debuginfo_pkg2,
+            devel_pkg1,
+            devel_pkg2,
+            cmp_half1.subject.downloaded_file,
+            cmp_half2.subject.downloaded_file,
+        ]
     cmd = [s for s in cmd if s != '']
 
     if global_config.dry_run:
@@ -1193,7 +1204,7 @@  def run_abipkgdiff(rpm_col1, rpm_col2):
     return_codes = [
         abipkgdiff(cmp_half1, cmp_half2) for cmp_half1, cmp_half2
         in generate_comparison_halves(rpm_col1, rpm_col2)]
-    return max(return_codes) if return_codes else 0
+    return max(return_codes, key=abs) if return_codes else 0
 
 
 @log_call