Differential D813

libabigail review
AbandonedPublic
Actions

Authored by kparal on Apr 14 2016, 12:46 PM.

Details

Reviewers
None
Test Plan

-

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
kparal retitled this revision from to libabigail review.Apr 14 2016, 12:46 PM
kparal updated this object.
kparal edited the test plan for this revision. (Show Details)
kparal added inline comments.Apr 14 2016, 2:53 PM
run_abipkgdiff.py
5

You're relying on rpmUtils being installed, but you haven't specified it in the environment -> rpm section in task formula. Either you need to specify a dependency, or you're welcome to use a libtaskotron method for the same purpose, look at libtaskotron.ext.fedora.rpm_utils.rpmformat().

74–75

Do you know '='*70 ? :-)

87–89

When I ran this check on lyx package, it took my computer 12 minutes to compare packages for a single architecture. That's really long, and we have a watchdog which kills a task that doesn't print anything to terminal for 20 minutes. I'd recommend you to add a print command before running each comparison to make sure the watchdog doesn't consider the task as stuck. Something like:

print 'Comparing files %s and %s...' % (rpm1, rpm2)
proc = subprocess.Popen(...
91

You can do

stdout, stderr = proc.communicate()

which immediately makes it more readable. Just a tip.

100–125

I have tried to compare lyx-2.1.4-6.fc24 and lyx-2.1.4-7.fc24. I have seen the same output printed out twice, once prefixed with "ABI changes found between" and once with "Incompatible ABI changes between". I think it is caused by using & (bitwise and) instead of == (equals), but my head spins around when I'm thinking about bitwise operations, so that's just a guess (please don't use them, they are ugly). Also, you should use elif in all those if clauses after the first one (in that case you can get rid of proc.returncode checking in all of them), or use continue at the end of each block, just to prevent such issues. (You should also print some warning or raise an error if the return code is something that you did not expect).

135–136

We have recently renamed summary to note and intend to use it for additional useful information on top of the basic info like koji_build etc. See note documentation at https://docs.qadevel.cloud.fedoraproject.org/libtaskotron/latest/docs/library.html#libtaskotron.check.CheckDetail . I'd probably avoid using note in this case, because it does not add any extra information. Unless you want to parse the output and display something like 5 function changes, 3 variable changes.

137–138

Since you're checking this per-arch, you need to specify in the CheckDetail that this is a result just for a particular arch. You can do that like this:

details.keyvals['arch'] = arch

(This is obscure and undocumented, we'll need to improve this.)

Please note that you should use the "base arch" here, so i386 instead of i686, and armhfp instead of armv7hl. Use libtaskotron.arch_utils.basearch() to convert it.

kparal added inline comments.Apr 14 2016, 8:45 PM
run_abipkgdiff.py
100–125

I looked at abidiff return values documentation, and realized these are really supposed to be bitwise operations. In that case please ignore the previous comment. However, there is still a bug somewhere, because the output was duplicated in my case. Here's the log:
http://paste.fedoraproject.org/355658/46066663/

sinnykumari added a comment.Apr 18 2016, 7:21 AM

Thanks Kamil for the quick review.

This is first time I am using Phabricator, so please excuse me if I miss something or made any mistake while commenting :)

Made changes as per inline comments provided and changes has been done in commit https://github.com/sinnykumari/task-libabigail/commit/752ace2c74b2ccc5aa37a3b6fcaa10e18d4d7cba in develop branch . I have created develop branch and started pushing changes in this branch first.

run_abipkgdiff.py
5

Added yum package in environment section so that it can use rpmUtils.
I looked into libtaskotron.ext.fedora.rpm_utils.rpmformat() function but can't use it right now because it returns a single string for one request. I am interested in storing n,v,e,r,a separately and rpmUtils.miscutils.splitFilename() provides required information in a single call.

74–75

Thanks, I didn't come to my mind that such feature will be already available in Python. Changed code accordingly.

87–89

Ah, didn't know that there is a 20 minutes watchdog. I have added print message before start of each ABI comparison.

91

Thanks for suggestion. Saving return values in above format now.

100–125

Good catch. Reason of having twice same message is that when Incompatible ABI changes occur then it is obvious that ABI changes also occur. So, both bit gets set during Incompatible ABI changes. It is also mentioned in abidiff return value documentation https://sourceware.org/libabigail/manual/abidiff.html#return-values .

I have modified code to look for ABI change (ABIDIFF_ABI_CHANGE.) bit value only when Incompatible ABI changes (ABIDIFF_ABI_INCOMPATIBLE_CHANGE.) didn't occur.

135–136

For now, I have removed summary since I am not parsing abi changes result. In future, if there is a plan to parse output then I will keep in mind to use note instead of summary.

137–138

Thanks for the info. Added

detail.keyvals['arch'] = arch_utils.basearch(arch)

to save per arch detail.

kparal abandoned this revision.May 13 2016, 8:04 AM

This was done, closing.

Revision Contents

Path
AMlibabigail.yml (77 lines)
AMrun_abipkgdiff.py (146 lines)

Diff 2073

View Options

libabigail.yml

  • This file was added.
PropertyOld ValueNew Value
File Modenull100755
1​name: libabigail
2​desc: download updates and latest stable package from koji and compare ABI changes between them
3​maintainer: sinnykumari
4​
5​input:
6​ args:
7​ - koji_build
8​
9​environment:
10​ rpm:
11​ - libabigail
12​
13​actions:
14​ - name: download rpms for given build for all architetcures
15​ koji:
16​ action: download_latest_stable
17​ koji_build: ${koji_build}
18​ arch: ['all']
19​ debuginfo: True
20​ target_dir: ${workdir}/stable
21​
22​ - name: download bodhi updates rpms from koji for all arcitetcures
23​ koji:
24​ action: download
25​ koji_build: ${koji_build}
26​ arch: ['all']
27​ debuginfo: True
28​ target_dir: ${workdir}/update
29​
30​ - name: run libabigail's abipkgdiff tool between updates and latest stable package for x86_64
31​ python:
32​ file: run_abipkgdiff.py
33​ callable: run_abipkgdiff
34​ arch: 'x86_64'
35​ koji_build: ${koji_build}
36​ artifactsdir: ${artifactsdir}
37​ stable_rpmsdir: ${workdir}/stable
38​ update_rpmsdir: ${workdir}/update
39​ export: abidiff_output_x86_64
40​
41​ - name: run libabigail's abipkgdiff tool between updates and latest stable package for i686
42​ python:
43​ file: run_abipkgdiff.py
44​ callable: run_abipkgdiff
45​ arch: 'i686'
46​ koji_build: ${koji_build}
47​ artifactsdir: ${artifactsdir}
48​ stable_rpmsdir: ${workdir}/stable
49​ update_rpmsdir: ${workdir}/update
50​ export: abidiff_output_i686
51​
52​ - name: run libabigail's abipkgdiff tool between updates and latest stable package for armv7hl
53​ python:
54​ file: run_abipkgdiff.py
55​ callable: run_abipkgdiff
56​ arch: 'armv7hl'
57​ koji_build: ${koji_build}
58​ artifactsdir: ${artifactsdir}
59​ stable_rpmsdir: ${workdir}/stable
60​ update_rpmsdir: ${workdir}/update
61​ export: abidiff_output_armv7hl
62​
63​
64​ - name: report results to resultsdb
65​ resultsdb:
66​ results: ${abidiff_output_x86_64}
67​ export: abidiff_output_x86_64
68​
69​ - name: report results to resultsdb
70​ resultsdb:
71​ results: ${abidiff_output_i686}
72​ export: abidiff_output_i686
73​
74​ - name: report results to resultsdb
75​ resultsdb:
76​ results: ${abidiff_output_armv7hl}
77​ export: abidiff_output_armv7hl
View Options

run_abipkgdiff.py

  • This file was added.
PropertyOld ValueNew Value
File Modenull100755
1​import os
2​import sys
3​import subprocess
4​
5​from rpmUtils.miscutils import splitFilename
kparalAuthorUnsubmitted
Not Done

You're relying on rpmUtils being installed, but you haven't specified it in the environment -> rpm section in task formula. Either you need to specify a dependency, or you're welcome to use a libtaskotron method for the same purpose, look at libtaskotron.ext.fedora.rpm_utils.rpmformat().

sinnykumariUnsubmitted
Not Done

Added yum package in environment section so that it can use rpmUtils.
I looked into libtaskotron.ext.fedora.rpm_utils.rpmformat() function but can't use it right now because it returns a single string for one request. I am interested in storing n,v,e,r,a separately and rpmUtils.miscutils.splitFilename() provides required information in a single call.

6​from libtaskotron import check
7​
8​def enum(**enums):
9​ return type('Enum', (), enums)
10​
11​def run_abipkgdiff(koji_build, stable_rpmsdir, update_rpmsdir,
12​ arch, artifactsdir='artifacts'):
13​ # Create artifacts directory if doesn't exist
14​ if not os.path.exists(artifactsdir):
15​ os.makedirs(artifactsdir)
16​
17​ update_debuginfo_rpm = []
18​ stable_debuginfo_rpm = []
19​ stable_rpms_detail = []
20​ update_rpms_detail = []
21​ output = []
22​ result = 'PASSED'
23​ log_path = os.path.join(artifactsdir, '%s.log' % koji_build)
24​ update_rpms = []
25​ stable_rpms = []
26​ # abipkgdiff return status as menioned on libabigail wiki page
27​ # https://sourceware.org/libabigail/manual/abidiff.html#return-values
28​ abipkgdiff_status = enum(ABIDIFF_OK = 0, ABIDIFF_ERROR = 1,
29​ ABIDIFF_USAGE_ERROR = 2, ABIDIFF_ABI_CHANGE = 4,
30​ ABIDIFF_ABI_INCOMPATIBLE_CHANGE = 8)
31​
32​ # Get debuginfo rpm for specified architecture
33​ for rpm in os.listdir(stable_rpmsdir):
34​ if arch in rpm:
35​ if 'debuginfo' in rpm:
36​ stable_debuginfo_rpm.append(rpm)
37​ else:
38​ stable_rpms.append(rpm)
39​ (n, v, r, e, a) = splitFilename(rpm)
40​ rpm_details = {'rpm': rpm, 'name': n, 'version':v, 'release': r,
41​ 'e': e, 'arch': a}
42​ stable_rpms_detail.append(rpm_details)
43​
44​ '''
45​ Extract and save NVER of rpms. Right now, Name field of rpm is used to
46​ to find similar rpm in both update and stable rpm list for running
47​ abipkgdiff on them
48​ '''
49​ for rpm in os.listdir(update_rpmsdir):
50​ if arch in rpm:
51​ if 'debuginfo' in rpm:
52​ update_debuginfo_rpm.append(rpm)
53​ else:
54​ update_rpms.append(rpm)
55​ (n, v, r, e, a) = splitFilename(rpm)
56​ rpm_details = {'rpm': rpm, 'name': n, 'version':v,
57​ 'release': r, 'e': e, 'arch': a}
58​ update_rpms_detail.append(rpm_details)
59​
60​ # Prepare abipkgdiff command with required args
61​ base_command = ['abipkgdiff']
62​
63​ if len(stable_debuginfo_rpm) == 1:
64​ base_command.append('--d1')
65​ base_command.append(stable_rpmsdir + '/' + stable_debuginfo_rpm[0])
66​
67​ if len(update_debuginfo_rpm) == 1:
68​ base_command.append('--d2')
69​ base_command.append(update_rpmsdir + '/' + update_debuginfo_rpm[0])
70​
71​ if not os.path.exists(log_path):
72​ output.append("ABI changes report summary for pacakge update id %s"\
73​ " against latest stable pacakge in koji\n" % koji_build)
74​ output.append("===================================================="\
75​ "=================================================\n\n")
kparalAuthorUnsubmitted
Not Done

Do you know '='*70 ? :-)

sinnykumariUnsubmitted
Not Done

Thanks, I didn't come to my mind that such feature will be already available in Python. Changed code accordingly.

76​ output.append("On %s architecture\n" % arch)
77​ output.append("-------------------------\n\n")
78​ for update_rpm in update_rpms_detail:
79​ for stable_rpm in stable_rpms_detail:
80​ if stable_rpm['name'] == update_rpm['name']:
81​ update_rpms.remove(update_rpm['rpm'])
82​ stable_rpms.remove(stable_rpm['rpm'])
83​ command = list(base_command)
84​ command.append(stable_rpmsdir + '/' + stable_rpm['rpm'])
85​ command.append(update_rpmsdir + '/' + update_rpm['rpm'])
86​
87​ # Execute abipkgdiff command with arguments
88​ proc = subprocess.Popen(command, stdout=subprocess.PIPE,
89​ stderr=subprocess.PIPE)
kparalAuthorUnsubmitted
Not Done

When I ran this check on lyx package, it took my computer 12 minutes to compare packages for a single architecture. That's really long, and we have a watchdog which kills a task that doesn't print anything to terminal for 20 minutes. I'd recommend you to add a print command before running each comparison to make sure the watchdog doesn't consider the task as stuck. Something like:

print 'Comparing files %s and %s...' % (rpm1, rpm2)
proc = subprocess.Popen(...
sinnykumariUnsubmitted
Not Done

Ah, didn't know that there is a 20 minutes watchdog. I have added print message before start of each ABI comparison.

90​ # Get error and abipkgdiff execution output
91​ res = proc.communicate()
kparalAuthorUnsubmitted
Not Done

You can do

stdout, stderr = proc.communicate()

which immediately makes it more readable. Just a tip.

sinnykumariUnsubmitted
Not Done

Thanks for suggestion. Saving return values in above format now.

92​ # Contains error information"
93​ if res[1]:
94​ result = "FAILED"
95​ output.append("* Some error encountered while running"\
96​ " command %s\n\n" %(' '.join(command)))
97​
98​ else:
99​ # Check for abipkgdiff exit status
100​ if proc.returncode == abipkgdiff_status.ABIDIFF_OK:
101​ output.append("* No ABI change between RPM packages %s " \
102​ "and %s\n\n" %(stable_rpm['rpm'],
103​ update_rpm['rpm']))
104​ if proc.returncode & abipkgdiff_status.ABIDIFF_ERROR:
105​ output.append("* Error while running command %s\n" \
106​ %(' '.join(command)))
107​ result = 'FAILED'
108​ if proc.returncode & abipkgdiff_status.ABIDIFF_USAGE_ERROR:
109​ output.append("* Insufficient arguments to command"\
110​ " abipkgdiff: %s\n" %(' '.join(command)))
111​ result = 'FAILED'
112​ if proc.returncode & abipkgdiff_status.ABIDIFF_ABI_CHANGE:
113​ output.append("* ABI changes found between" \
114​ " packages %s and %s. Please review them.\n\n" \
115​ %(stable_rpm['rpm'], update_rpm['rpm']))
116​ output.append(res[0])
117​ if result != 'FAILED':
118​ result = 'NEEDS_INSPECTION'
119​ # Compared ABI are different and may be incompatible
120​ if proc.returncode & abipkgdiff_status.ABIDIFF_ABI_INCOMPATIBLE_CHANGE:
121​ output.append("* Incompatible ABI changes between" \
122​ " packages %s and %s. Please review them.\n\n" \
123​ %(stable_rpm['rpm'], update_rpm['rpm']))
124​ output.append(res[0])
125​ result = 'FAILED'
kparalAuthorUnsubmitted
Not Done

I have tried to compare lyx-2.1.4-6.fc24 and lyx-2.1.4-7.fc24. I have seen the same output printed out twice, once prefixed with "ABI changes found between" and once with "Incompatible ABI changes between". I think it is caused by using & (bitwise and) instead of == (equals), but my head spins around when I'm thinking about bitwise operations, so that's just a guess (please don't use them, they are ugly). Also, you should use elif in all those if clauses after the first one (in that case you can get rid of proc.returncode checking in all of them), or use continue at the end of each block, just to prevent such issues. (You should also print some warning or raise an error if the return code is something that you did not expect).

kparalAuthorUnsubmitted
Not Done

I looked at abidiff return values documentation, and realized these are really supposed to be bitwise operations. In that case please ignore the previous comment. However, there is still a bug somewhere, because the output was duplicated in my case. Here's the log:
http://paste.fedoraproject.org/355658/46066663/

sinnykumariUnsubmitted
Not Done

Good catch. Reason of having twice same message is that when Incompatible ABI changes occur then it is obvious that ABI changes also occur. So, both bit gets set during Incompatible ABI changes. It is also mentioned in abidiff return value documentation https://sourceware.org/libabigail/manual/abidiff.html#return-values .

I have modified code to look for ABI change (ABIDIFF_ABI_CHANGE.) bit value only when Incompatible ABI changes (ABIDIFF_ABI_INCOMPATIBLE_CHANGE.) didn't occur.

126​
127​ if len(stable_rpms) > 0:
128​ output.append("* Following rpms are no longer available:\n")
129​ output.append(" %s\n\n" %('\n'.join(stable_rpms)))
130​ passflag = False
131​ if len(update_rpms) > 0:
132​ output.append("* Additional rpms has been included in update:\n")
133​ output.append(" %s\n\n" %('\n'.join(update_rpms)))
134​
135​ summary = 'ABI check for koji build %s on %s architecture' \
136​ % (koji_build, arch)
kparalAuthorUnsubmitted
Not Done

We have recently renamed summary to note and intend to use it for additional useful information on top of the basic info like koji_build etc. See note documentation at https://docs.qadevel.cloud.fedoraproject.org/libtaskotron/latest/docs/library.html#libtaskotron.check.CheckDetail . I'd probably avoid using note in this case, because it does not add any extra information. Unless you want to parse the output and display something like 5 function changes, 3 variable changes.

sinnykumariUnsubmitted
Not Done

For now, I have removed summary since I am not parsing abi changes result. In future, if there is a plan to parse output then I will keep in mind to use note instead of summary.

137​ detail = check.CheckDetail(koji_build, check.ReportType.BODHI_UPDATE,
138​ result, summary)
kparalAuthorUnsubmitted
Not Done

Since you're checking this per-arch, you need to specify in the CheckDetail that this is a result just for a particular arch. You can do that like this:

details.keyvals['arch'] = arch

(This is obscure and undocumented, we'll need to improve this.)

Please note that you should use the "base arch" here, so i386 instead of i686, and armhfp instead of armv7hl. Use libtaskotron.arch_utils.basearch() to convert it.

sinnykumariUnsubmitted
Not Done

Thanks for the info. Added

detail.keyvals['arch'] = arch_utils.basearch(arch)

to save per arch detail.

139​
140​ # Save logs
141​ with open(log_path, 'a') as log_file:
142​ log_file.writelines(output)
143​ detail.artifact = log_path
144​ output = check.export_YAML(detail)
145​
146​ return output