Differential D817

libabigail task
AbandonedPublic
Actions

Authored by kparal on Apr 18 2016, 7:27 AM.

Details

Reviewers
sinnykumari
Summary

Code re-factoring as per review comments

Test Plan

None

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
sinnykumari retitled this revision from to libabigail task.Apr 18 2016, 7:27 AM
sinnykumari updated this object.
sinnykumari edited the test plan for this revision. (Show Details)
sinnykumari added a reviewer: kparal.
kparal accepted this revision.Apr 21 2016, 2:07 PM

Looks good to me. I haven't checked the bitwise operations :-)

run_abipkgdiff.py
160

This might have been left here as a debug printout by accident.

This revision is now accepted and ready to land.Apr 21 2016, 2:07 PM
sinnykumari added a comment.Apr 21 2016, 5:18 PM

Thanks Kamil for the review :)

run_abipkgdiff.py
160

Ah, yes. Forgot to remove print statement after debugging.

tflink added a comment.May 4 2016, 1:07 PM

A few minor questions, otherwise looks good to me. Very much looking forward to getting this running in production.

libabigail.yml
12

Out of curiosity, is yum going to be a long term requirement?

run_abipkgdiff.py
154

Why is the check.ReportType.BODHI_UPDATE part hardcoded? Shouldn't this be reported as check.ReportType.KOJI_BUILD?

sinnykumari added a comment.May 4 2016, 5:13 PM

Thanks Tim for the review :)

libabigail.yml
12

As per usecase, it was easy to use splitFilename provided by rpmUtils.miscutils module. If there is some issues with keeping yum as dependency then I can remove it.

run_abipkgdiff.py
154

Ah, that's because initially (before discussion started on qa-devel ML) plan was to run task only on bodhi updates. Slipped from mind to update it to check.ReportType.KOJI_BUILD. Fixed it in commit https://github.com/sinnykumari/task-libabigail/commit/e203c0d86d84a716c38d1a58b89daa7a77bffd8e

kparal commandeered this revision.May 13 2016, 8:26 AM
kparal edited reviewers, added: sinnykumari; removed: kparal.

This was done (modulo some trailing comments), I'm closing this so that it gets removed from the queue.

Sinny, one last comment below.

libabigail.yml
12

I think Tim is asking because Fedora in general is trying to steer away from yum towards dnf (the transition is almost complete). Yum is going to be deprecated in the future. However, for this particular use case and for the mid-term future, I don't think it matters much.

If you wanted to switch, you can do this:

from libtaskotron.ext.fedora import rpm_utils

nevra = rpm[:-len('.rpm')]
name = rpm_utils.rpmformat(nevra, fmt='n', end_arch=True)

or this (add a dependency on python2-hawkey):

import hawkey

nevra = rpm[:-len('.rpm')]
name = hawkey.split_nevra(nevra).name

You don't actually use anything else than name in your code (you don't use version, release, epoch, etc). If you did, it's trivial to do with hawkey (it returns an object), and we could easily add it to rpm_utils.rpmformat as well, if requested.

Regardless of whether you do this or not, I'd recommend you to edit lines 34 and 50 and add a safety check that only rpm files are processed, like this:

for rpm in os.listdir(stable_rpmsdir):
    if not rpm.endswith('.rpm'):
        continue
    ...
This revision now requires review to proceed.May 13 2016, 8:26 AM
kparal abandoned this revision.May 13 2016, 8:27 AM

Revision Contents

Path
Mlibabigail.yml (1 line)
Mrun_abipkgdiff.py (82 lines)

Diff 2081

View Options

libabigail.yml

Context not available.
9environment: 9​environment:
10 rpm: 10​ rpm:
11 - libabigail 11​ - libabigail
12​ - yum
tflinkUnsubmitted
Not Done

Out of curiosity, is yum going to be a long term requirement?

sinnykumariUnsubmitted
Not Done

As per usecase, it was easy to use splitFilename provided by rpmUtils.miscutils module. If there is some issues with keeping yum as dependency then I can remove it.

kparalAuthorUnsubmitted
Not Done

I think Tim is asking because Fedora in general is trying to steer away from yum towards dnf (the transition is almost complete). Yum is going to be deprecated in the future. However, for this particular use case and for the mid-term future, I don't think it matters much.

If you wanted to switch, you can do this:

from libtaskotron.ext.fedora import rpm_utils

nevra = rpm[:-len('.rpm')]
name = rpm_utils.rpmformat(nevra, fmt='n', end_arch=True)

or this (add a dependency on python2-hawkey):

import hawkey

nevra = rpm[:-len('.rpm')]
name = hawkey.split_nevra(nevra).name

You don't actually use anything else than name in your code (you don't use version, release, epoch, etc). If you did, it's trivial to do with hawkey (it returns an object), and we could easily add it to rpm_utils.rpmformat as well, if requested.

Regardless of whether you do this or not, I'd recommend you to edit lines 34 and 50 and add a safety check that only rpm files are processed, like this:

for rpm in os.listdir(stable_rpmsdir):
    if not rpm.endswith('.rpm'):
        continue
    ...
12 13​
13actions: 14​actions:
14 - name: download rpms for given build for all architetcures 15​ - name: download rpms for given build for all architetcures
Context not available.
View Options

run_abipkgdiff.py

Context not available.
4 4​
5from rpmUtils.miscutils import splitFilename 5​from rpmUtils.miscutils import splitFilename
6from libtaskotron import check 6​from libtaskotron import check
7​from libtaskotron import arch_utils
7 8​
8def enum(**enums): 9​def enum(**enums):
9 return type('Enum', (), enums) 10​ return type('Enum', (), enums)
Context not available.
58 update_rpms_detail.append(rpm_details) 59​ update_rpms_detail.append(rpm_details)
59 60​
60 # Prepare abipkgdiff command with required args 61​ # Prepare abipkgdiff command with required args
61 base_command = ['abipkgdiff'] 62​ # We will compare ABI of only Shared libraries ELF files
63​ # This is because shared libraries are the one which is consumed by other
64​ # dependent packages
65​ base_command = ['abipkgdiff', '--dso-only']
62 66​
63 if len(stable_debuginfo_rpm) == 1: 67​ if len(stable_debuginfo_rpm) == 1:
64 base_command.append('--d1') 68​ base_command.append('--d1')
Context not available.
71 if not os.path.exists(log_path): 75​ if not os.path.exists(log_path):
72 output.append("ABI changes report summary for pacakge update id %s"\ 76​ output.append("ABI changes report summary for pacakge update id %s"\
73 " against latest stable pacakge in koji\n" % koji_build) 77​ " against latest stable pacakge in koji\n" % koji_build)
74 output.append("===================================================="\ 78​ padding = str('=' * 103)
75 "=================================================\n\n") 79​ output.append("%s\n\n" % padding)
76 output.append("On %s architecture\n" % arch) 80​ output.append("On %s architecture\n" % arch)
77 output.append("-------------------------\n\n") 81​ padding = str('-' * 25)
82​ output.append("%s\n\n" % padding)
78 for update_rpm in update_rpms_detail: 83​ for update_rpm in update_rpms_detail:
79 for stable_rpm in stable_rpms_detail: 84​ for stable_rpm in stable_rpms_detail:
80 if stable_rpm['name'] == update_rpm['name']: 85​ if stable_rpm['name'] == update_rpm['name']:
Context not available.
85 command.append(update_rpmsdir + '/' + update_rpm['rpm']) 90​ command.append(update_rpmsdir + '/' + update_rpm['rpm'])
86 91​
87 # Execute abipkgdiff command with arguments 92​ # Execute abipkgdiff command with arguments
93​ print 'Comparing rpms %s and %s...' % (stable_rpm['rpm'],
94​ update_rpm['rpm'])
88 proc = subprocess.Popen(command, stdout=subprocess.PIPE, 95​ proc = subprocess.Popen(command, stdout=subprocess.PIPE,
89 stderr=subprocess.PIPE) 96​ stderr=subprocess.PIPE)
90 # Get error and abipkgdiff execution output 97​ # Get error and abipkgdiff execution output
91 res = proc.communicate() 98​ stdout, stderr = proc.communicate()
92 # Contains error information" 99​ # Contains error information"
93 if res[1]: 100​ if stderr:
94 result = "FAILED" 101​ result = "FAILED"
95 output.append("* Some error encountered while running"\ 102​ output.append("* Some error encountered while running"\
96 " command %s\n\n" %(' '.join(command))) 103​ " command %s\n\n" %(' '.join(command)))
97 104​
98 else: 105​ else:
99 # Check for abipkgdiff exit status 106​ # Check for abipkgdiff exit status
107​ # No ABI change or error when exit status is ABIDIFF_OK whose value is 0
100 if proc.returncode == abipkgdiff_status.ABIDIFF_OK: 108​ if proc.returncode == abipkgdiff_status.ABIDIFF_OK:
101 output.append("* No ABI change between RPM packages %s " \ 109​ output.append("* No ABI change between RPM packages %s " \
102 "and %s\n\n" %(stable_rpm['rpm'], 110​ "and %s\n\n" %(stable_rpm['rpm'],
103 update_rpm['rpm'])) 111​ update_rpm['rpm']))
104 if proc.returncode & abipkgdiff_status.ABIDIFF_ERROR: 112​ else:
105 output.append("* Error while running command %s\n" \ 113​ # If ABIDIFF_USAGE_ERROR is set to true then ABIDIFF_ERROR
106 %(' '.join(command))) 114​ # also set to true. So, check for ABIDIFF_ERROR only if
107 result = 'FAILED' 115​ # ABIDIFF_USAGE_ERROR not set to true
108 if proc.returncode & abipkgdiff_status.ABIDIFF_USAGE_ERROR: 116​ if proc.returncode & abipkgdiff_status.ABIDIFF_USAGE_ERROR:
109 output.append("* Insufficient arguments to command"\ 117​ output.append("* Insufficient arguments to command"\
110 " abipkgdiff: %s\n" %(' '.join(command))) 118​ " abipkgdiff: %s\n" %(' '.join(command)))
111 result = 'FAILED' 119​ result = 'FAILED'
112 if proc.returncode & abipkgdiff_status.ABIDIFF_ABI_CHANGE: 120​ elif proc.returncode & abipkgdiff_status.ABIDIFF_ERROR:
113 output.append("* ABI changes found between" \ 121​ output.append("* Error while running command %s\n" \
114 " packages %s and %s. Please review them.\n\n" \ 122​ %(' '.join(command)))
115 %(stable_rpm['rpm'], update_rpm['rpm'])) 123​ result = 'FAILED'
116 output.append(res[0]) 124​
117 if result != 'FAILED': 125​ # Compared ABI are different and may be incompatible
118 result = 'NEEDS_INSPECTION' 126​ if proc.returncode & abipkgdiff_status.ABIDIFF_ABI_INCOMPATIBLE_CHANGE:
119 # Compared ABI are different and may be incompatible 127​ output.append("* Incompatible ABI changes between" \
120 if proc.returncode & abipkgdiff_status.ABIDIFF_ABI_INCOMPATIBLE_CHANGE: 128​ " packages %s and %s. Please review them.\n\n" \
121 output.append("* Incompatible ABI changes between" \ 129​ %(stable_rpm['rpm'], update_rpm['rpm']))
122 " packages %s and %s. Please review them.\n\n" \ 130​ output.append(stdout)
123 %(stable_rpm['rpm'], update_rpm['rpm'])) 131​ result = 'FAILED'
124 output.append(res[0]) 132​ # If exit status of abipkgdiff has ABIDIFF_ABI_INCOMPATIBLE_CHANGE (8)
125 result = 'FAILED' 133​ # set then no need to check for ABIDIFF_ABI_CHANGE bit(4)
134​ # because it will be definitely true.
135​ # But check for ABIDIFF_ABI_CHANGE value if
136​ # ABIDIFF_ABI_INCOMPATIBLE_CHANGE is not set to true.
137​ elif proc.returncode & abipkgdiff_status.ABIDIFF_ABI_CHANGE:
138​ output.append("* ABI changes found between" \
139​ " packages %s and %s. Please review them.\n\n" \
140​ %(stable_rpm['rpm'], update_rpm['rpm']))
141​ output.append(stdout)
142​ if result != 'FAILED':
143​ result = 'NEEDS_INSPECTION'
144​
126 145​
127 if len(stable_rpms) > 0: 146​ if len(stable_rpms) > 0:
128 output.append("* Following rpms are no longer available:\n") 147​ output.append("* Following rpms are no longer available:\n")
Context not available.
132 output.append("* Additional rpms has been included in update:\n") 151​ output.append("* Additional rpms has been included in update:\n")
133 output.append(" %s\n\n" %('\n'.join(update_rpms))) 152​ output.append(" %s\n\n" %('\n'.join(update_rpms)))
134 153​
135 summary = 'ABI check for koji build %s on %s architecture' \ 154​ detail = check.CheckDetail(koji_build, check.ReportType.BODHI_UPDATE, result)
tflinkUnsubmitted
Not Done

Why is the check.ReportType.BODHI_UPDATE part hardcoded? Shouldn't this be reported as check.ReportType.KOJI_BUILD?

sinnykumariUnsubmitted
Not Done

Ah, that's because initially (before discussion started on qa-devel ML) plan was to run task only on bodhi updates. Slipped from mind to update it to check.ReportType.KOJI_BUILD. Fixed it in commit https://github.com/sinnykumari/task-libabigail/commit/e203c0d86d84a716c38d1a58b89daa7a77bffd8e

136 % (koji_build, arch) ​
137 detail = check.CheckDetail(koji_build, check.ReportType.BODHI_UPDATE, ​
138 result, summary) ​
139 155​
140 # Save logs 156​ # Save logs
141 with open(log_path, 'a') as log_file: 157​ with open(log_path, 'a') as log_file:
142 log_file.writelines(output) 158​ log_file.writelines(output)
143 detail.artifact = log_path 159​ detail.artifact = log_path
160​ print arch_utils.basearch(arch), arch
kparalAuthorUnsubmitted
Not Done

This might have been left here as a debug printout by accident.

sinnykumariUnsubmitted
Not Done

Ah, yes. Forgot to remove print statement after debugging.

161​ detail.keyvals['arch'] = arch_utils.basearch(arch)
144 output = check.export_YAML(detail) 162​ output = check.export_YAML(detail)
145 163​
146 return output 164​ return output
Context not available.