Differential D948

Python Version Check for Taskotron
ClosedPublic
Actions

Authored by churchyard on Jul 22 2016, 8:27 AM.

Details

Reviewers
kparal
jskladan
Group Reviewers
libtaskotron
Summary

This is a check that makes sure packages do not require both Pythons (2 & 3), except explicitly listed packages. It is a common mistake when porting stuff to Python 3, that shebangs leave the dependency on Python 2 as well etc.

Test Plan
$ runtask -i tracer-0.6.9-1.fc23 -t koji_build runtask.yaml
...
[python-versions] 11:22:17 ERROR   tracer-0.6.9-1.fc23.noarch.rpm requires both Python 2 and 3, that's usually bad.
[python-versions] 11:22:17 INFO    python-versions FAILED for tracer-0.6.9-1.fc23 (tracer-0.6.9-1.fc23.noarch.rpm requires Python 2 and 3)
[libtaskotron] 11:22:17 INFO    Reporting to ResultsDB is disabled. Once enabled, the following would get reported:
results:
- artifact: /var/lib/taskotron/artifacts/20160920_112213_239750/output.log
  item: tracer-0.6.9-1.fc23
  note: tracer-0.6.9-1.fc23.noarch.rpm requires Python 2 and 3
  outcome: FAILED
  type: koji_build

$ runtask -i eric-6.1.6-2.fc25 -t koji_build runtask.yaml
...
[python-versions] 11:22:56 WARNING eric is excluded from this check
[python-versions] 11:22:56 INFO    python-versions PASSED for eric-6.1.6-2.fc25 (no problems found)
[libtaskotron] 11:22:56 INFO    Reporting to ResultsDB is disabled. Once enabled, the following would get reported:
results:
- item: eric-6.1.6-2.fc25
  note: no problems found
  outcome: PASSED
  type: koji_build

$ runtask -i python-six-1.10.0-3.fc25 -t koji_build runtask.yaml
...
[python-versions] 11:23:23 INFO    python-six-1.10.0-3.fc25.noarch.rpm requires Python 2 only, that's OK
[python-versions] 11:23:23 INFO    python3-six-1.10.0-3.fc25.noarch.rpm requires Python 3 only, that's OK
[python-versions] 11:23:23 INFO    python-versions PASSED for python-six-1.10.0-3.fc25 (no problems found)
[libtaskotron] 11:23:23 INFO    Reporting to ResultsDB is disabled. Once enabled, the following would get reported:
results:
- item: python-six-1.10.0-3.fc25
  note: no problems found
  outcome: PASSED
  type: koji_build

$ runtask -i python3-3.5.1-17.fc24 -t koji_build runtask.yaml
...
[python-versions] 11:40:12 INFO    python3-3.5.1-17.fc24.armv7hl.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-3.5.1-17.fc24.i686.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-3.5.1-17.fc24.x86_64.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-debug-3.5.1-17.fc24.armv7hl.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-debug-3.5.1-17.fc24.i686.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-debug-3.5.1-17.fc24.x86_64.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-devel-3.5.1-17.fc24.armv7hl.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-devel-3.5.1-17.fc24.i686.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-devel-3.5.1-17.fc24.x86_64.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-libs-3.5.1-17.fc24.armv7hl.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-libs-3.5.1-17.fc24.i686.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-libs-3.5.1-17.fc24.x86_64.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-test-3.5.1-17.fc24.armv7hl.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-test-3.5.1-17.fc24.i686.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-test-3.5.1-17.fc24.x86_64.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-tkinter-3.5.1-17.fc24.armv7hl.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-tkinter-3.5.1-17.fc24.i686.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-tkinter-3.5.1-17.fc24.x86_64.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-tools-3.5.1-17.fc24.armv7hl.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-tools-3.5.1-17.fc24.i686.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python3-tools-3.5.1-17.fc24.x86_64.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    system-python-3.5.1-17.fc24.armv7hl.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    system-python-3.5.1-17.fc24.i686.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    system-python-3.5.1-17.fc24.x86_64.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    system-python-libs-3.5.1-17.fc24.armv7hl.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    system-python-libs-3.5.1-17.fc24.i686.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    system-python-libs-3.5.1-17.fc24.x86_64.rpm requires Python 3 only, that's OK
[python-versions] 11:40:12 INFO    python-versions PASSED for python3-3.5.1-17.fc24 (no problems found)
[libtaskotron] 11:40:12 INFO    Reporting to ResultsDB is disabled. Once enabled, the following would get reported:
results:
- item: python3-3.5.1-17.fc24
  note: no problems found
  outcome: PASSED
  type: koji_build

$ runtask -i python-admesh-0.98.5-3.fc25 -t koji_build runtask.yaml
...
warning: /var/cache/taskotron/python-admesh-0.98.5-3.fc25.aarch64.rpm: Header V3 RSA/SHA1 Signature, key ID e372e838: NOKEY
[python-versions] 11:53:12 ERROR   python-admesh-0.98.5-3.fc25.aarch64.rpm: public key not available
[python-versions] 11:53:12 INFO    python-admesh-0.98.5-3.fc25.armv7hl.rpm requires Python 2 only, that's OK
[python-versions] 11:53:12 INFO    python-admesh-0.98.5-3.fc25.i686.rpm requires Python 2 only, that's OK
[python-versions] 11:53:12 INFO    python-admesh-0.98.5-3.fc25.x86_64.rpm requires Python 2 only, that's OK
[python-versions] 11:53:12 ERROR   python3-admesh-0.98.5-3.fc25.aarch64.rpm: public key not available
[python-versions] 11:53:12 INFO    python3-admesh-0.98.5-3.fc25.armv7hl.rpm requires Python 3 only, that's OK
[python-versions] 11:53:12 INFO    python3-admesh-0.98.5-3.fc25.i686.rpm requires Python 3 only, that's OK
[python-versions] 11:53:12 INFO    python3-admesh-0.98.5-3.fc25.x86_64.rpm requires Python 3 only, that's OK
[python-versions] 11:53:12 INFO    python-versions PASSED for python-admesh-0.98.5-3.fc25 (no problems found)
[libtaskotron] 11:53:12 INFO    Reporting to ResultsDB is disabled. Once enabled, the following would get reported:
results:
- item: python-admesh-0.98.5-3.fc25
  note: no problems found
  outcome: PASSED
  type: koji_build

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
churchyard retitled this revision from to Python Version Check for Taskotron.Jul 22 2016, 8:27 AM
churchyard updated this object.
churchyard edited the test plan for this revision. (Show Details)
churchyard added inline comments.Jul 22 2016, 8:51 AM
python_versions_check.py
134

What shall be here instead of TODO?

garretraziel added a subscriber: garretraziel.Jul 22 2016, 10:14 AM

Just a note: into "Test plan" you put what you've done to test that this DR works (and we should be able to repeat this test to verify it).

churchyard edited the test plan for this revision. (Show Details)Jul 22 2016, 10:37 AM
jskladan accepted this revision.Jul 26 2016, 12:31 PM
jskladan added a reviewer: jskladan.
jskladan added a subscriber: jskladan.

Looks good to me, apart of the comment below.
Sorry for the late reply - we're preparing for Flock, so the response rate is a bit low now...

python_versions_check.py
132–134

This is probably not well documented, but the note should be something quite short. I suggest creating a file containing the suggestion, and adding it as an artifact to the result. Something in the likes of:

if bads:
    s = 's' if len(bads) else ''
    detail.artifact = os.path.join(workdir, 'output.log'
    with open(detail.artifact, 'w') as output:
        output.write ('{} require{} both Pythons, '
	​                       'if you think that\'s false or intentional, '
	​                       'file a bug against TODO'.format(','.join(bads), s))
This revision is now accepted and ready to land.Jul 26 2016, 12:31 PM
kparal requested changes to this revision.Aug 3 2016, 2:52 PM
kparal added a reviewer: kparal.
kparal added a subscriber: kparal.

The script looks good. Please create an artifact instead of using note and better explain the packagers what's wrong with their packages in case of a failure, and I think we can soon push this to our dev instance.

python_versions_check.py
132–134

This is not completely correct. The result file (txt, html, whatever you want) needs to be created in artifacts directory, not workdir. You can pass ${artifactsdir} variable from the task formula to the python script so that you know where to create it.

But overall, yes, the note is optional and supposed to contain something short and useful (e.g. 1 fail, 49 passes) when manually reviewing the results in ResultsDB. It's not a good fit here. You can either print the errors to stdout and hope that people reading the logs will see it, or (more user friendly) create a specific file where you explain what went wrong, print helpful details, link some FAQ or project page for this check (so that people can report bugs or request whitelisting), etc.

The advantage of this is that you can be really verbose and explain what's wrong, since these reports will be likely read by many packagers who will have no idea what this check is for. Currently {} require{} both Pythons doesn't really explain what's wrong (and "both Pythons" in itself is very ambiguous).

runtask.yaml
4

This could be a bit more specific (what can go wrong with the dependencies?). We have no limit on description length.

This revision now requires changes to proceed.Aug 3 2016, 2:52 PM
churchyard updated this revision to Diff 2572.Sep 20 2016, 11:58 AM
churchyard edited the test plan for this revision. (Show Details)
  • In case of failure, create a log with more descriptive message
  • The note is less verbose
  • Log the filename, not the path
  • More verbose description
  • Add a list of packages not indicating a dependency (python-(s)rpm-macros)
  • Do not traceback when something goes bad with RPM
  • PEP 8 stuff
kparal accepted this revision.Oct 12 2016, 1:47 PM

Apart of that small nitpick, I don't see any problem with this.

Now you'll need to set up a git repo with this check somewhere and give us the URL. Set up at least two branches - develop and master. develop is going to be used at our dev and stg instance, and master is going to be used at our production instance. Both branches will be automatically pulled every hour.

python_versions_check.py
162–164

Listing all rpm file names in note can make it possibly very long. I'd say this is much more appropriate to put into the log file (where it already is). Are affected RPM filenames something that the maintainer should immediately see from the resultsdb interface, before clicking to see the log file? Is that going to be mostly useful?

I'd recommend removing the note completely, because I see no good usage for it in this particular check. However, that's just a recommendation. We'll need get used to field values which do not satisfy our aesthetic goals, or start restricting them (e.g. in length) somehow sooner or later :)

This revision is now accepted and ready to land.Oct 12 2016, 1:47 PM
kparal mentioned this in T860: deploy task-python-versions.Oct 21 2016, 12:12 PM

I created T860 to deploy this task. You can close the review now, thanks.

churchyard closed this revision.Oct 21 2016, 12:15 PM

Revision Contents

Path
AM.gitignore (3 lines)
AMpython_versions_check.py (175 lines)
AMruntask.yaml (36 lines)

Diff 2572

View Options

.gitignore

  • This file was added.
1​*.rpm
2​*.pyo
3​*.pyc
View Options

python_versions_check.py

  • This file was added.
1​import logging
2​if __name__ == '__main__':
3​ # Set up logging ASAP to see potential problems during import.
4​ # Don't set it up when not running as the main script, someone else handles
5​ # that then.
6​ logging.basicConfig()
7​
8​import os
9​import rpm
10​from libtaskotron import check
11​
12​log = logging.getLogger('python-versions')
13​log.setLevel(logging.DEBUG)
14​log.addHandler(logging.NullHandler())
15​
16​LINK = ('https://python-rpm-porting.readthedocs.io/en/latest/applications.html'
17​ '#are-shebangs-dragging-you-down-to-python-2')
18​TEMPLATE = '''{} require{} both Pythons, i.e. Python 2 and 3.
19​
20​Read {} to find more information and possible cause.
21​Or ask at #fedora-python IRC channel for help.
22​
23​If you think the result is false or intentional, file a bug against TODO.
24​'''
25​
26​
27​NEVRS_STARTS = {
28​ 2: (b'python(abi) = 2.',),
29​ 3: (b'python(abi) = 3.',)
30​}
31​
32​NAME_STARTS = {
33​ 2: (
34​ b'python-',
35​ b'python2',
36​ b'/usr/bin/python2',
37​ b'libpython2',
38​ b'pygtk2',
39​ b'pygobject2',
40​ b'pycairo',
41​ b'py-',
42​ ),
43​ 3: (
44​ b'python3',
45​ b'/usr/bin/python3',
46​ b'libpython3',
47​ b'system-python'
48​ )
49​}
50​
51​NAME_EXACTS = {
52​ 2: (
53​ b'/usr/bin/python',
54​ b'python',
55​ )
56​}
57​
58​NAME_NOTS = (
59​ b'python-rpm-macros',
60​ b'python-srpm-macros',
61​)
62​
63​WHITELIST = (
64​ 'eric', # https://bugzilla.redhat.com/show_bug.cgi?id=1342492
65​ 'pungi', # https://bugzilla.redhat.com/show_bug.cgi?id=1342497
66​)
67​
68​
69​def python_versions_check(path):
70​ '''
71​ For the binary RPM in path, report back what Python
72​ versions it depends on
73​ '''
74​ ts = rpm.TransactionSet()
75​ with open(path, 'rb') as fdno:
76​ try:
77​ hdr = ts.hdrFromFdno(fdno)
78​ except rpm.error as e:
79​ log.error('{}: {}'.format(os.path.basename(path), e))
80​ return None, None
81​
82​ py_versions = set()
83​
84​ for nevr in hdr[rpm.RPMTAG_REQUIRENEVRS]:
85​ for py_version, starts in NEVRS_STARTS.items():
86​ for start in starts:
87​ if nevr.startswith(start):
88​ log.debug('Found dependency {}'.format(nevr.decode()))
89​ log.debug('Requires Python {}'.format(py_version))
90​ py_versions.add(py_version)
91​ break
92​
93​ for name in hdr[rpm.RPMTAG_REQUIRENAME]:
94​ for py_version, starts in NAME_STARTS.items():
95​ if py_version not in py_versions:
96​ for start in starts:
97​ if name.startswith(start) and name not in NAME_NOTS:
98​ log.debug('Found dependency {}'.format(name.decode()))
99​ log.debug('Requires Python {}'.format(py_version))
100​ py_versions.add(py_version)
101​ break
102​ for py_version, exacts in NAME_EXACTS.items():
103​ if py_version not in py_versions:
104​ if name in exacts:
105​ log.debug('Found dependency {}'.format(name.decode()))
106​ log.debug('Requires Python {}'.format(py_version))
107​ py_versions.add(py_version)
108​
109​ return hdr[rpm.RPMTAG_NAME], py_versions
110​
111​
112​def run(koji_build, workdir='.', artifactsdir='artifacts'):
113​ '''The main method to run from Taskotron'''
114​ workdir = os.path.abspath(workdir)
115​
116​ # find files to run on
117​ files = sorted(os.listdir(workdir))
118​ rpms = []
119​ for file_ in files:
120​ path = os.path.join(workdir, file_)
121​ if file_.endswith('.rpm'):
122​ rpms.append(path)
123​ else:
124​ log.debug('Ignoring non-rpm file: {}'.format(path))
125​
126​ outcome = 'PASSED'
127​ bads = []
128​
129​ if not rpms:
130​ log.warn('No binary rpm files found in: {}'.format(workdir))
131​ for path in rpms:
132​ filename = os.path.basename(path)
133​ log.debug('Checking {}'.format(filename))
134​ name, py_versions = python_versions_check(path)
churchyardAuthorUnsubmitted
Not Done

What shall be here instead of TODO?

jskladanUnsubmitted
Not Done

This is probably not well documented, but the note should be something quite short. I suggest creating a file containing the suggestion, and adding it as an artifact to the result. Something in the likes of:

if bads:
    s = 's' if len(bads) else ''
    detail.artifact = os.path.join(workdir, 'output.log'
    with open(detail.artifact, 'w') as output:
        output.write ('{} require{} both Pythons, '
  ​                       'if you think that\'s false or intentional, '
  ​                       'file a bug against TODO'.format(','.join(bads), s))
kparalUnsubmitted
Not Done

This is not completely correct. The result file (txt, html, whatever you want) needs to be created in artifacts directory, not workdir. You can pass ${artifactsdir} variable from the task formula to the python script so that you know where to create it.

But overall, yes, the note is optional and supposed to contain something short and useful (e.g. 1 fail, 49 passes) when manually reviewing the results in ResultsDB. It's not a good fit here. You can either print the errors to stdout and hope that people reading the logs will see it, or (more user friendly) create a specific file where you explain what went wrong, print helpful details, link some FAQ or project page for this check (so that people can report bugs or request whitelisting), etc.

The advantage of this is that you can be really verbose and explain what's wrong, since these reports will be likely read by many packagers who will have no idea what this check is for. Currently {} require{} both Pythons doesn't really explain what's wrong (and "both Pythons" in itself is very ambiguous).

135​ if name is None:
136​ # RPM could not read that file, not our problem
137​ # error is already logged
138​ pass
139​ elif name in WHITELIST:
140​ log.warn('{} is excluded from this check'.format(name))
141​ elif len(py_versions) == 0:
142​ log.info('{} does not require Python, that\'s OK'.format(filename))
143​ elif len(py_versions) == 1:
144​ py_version = next(iter(py_versions))
145​ log.info('{} requires Python {} only, that\'s OK'
146​ .format(filename, py_version))
147​ else:
148​ log.error('{} requires both Python 2 and 3, that\'s usually bad.'
149​ .format(filename))
150​ outcome = 'FAILED'
151​ bads.append(filename)
152​
153​ detail = check.CheckDetail(koji_build,
154​ check.ReportType.KOJI_BUILD,
155​ outcome)
156​
157​ if bads:
158​ s = 's' if len(bads) == 1 else ''
159​ detail.artifact = os.path.join(artifactsdir, 'output.log')
160​ with open(detail.artifact, 'w') as f:
161​ f.write(TEMPLATE.format(','.join(bads), s, LINK))
162​ detail.note = '{} require{} Python 2 and 3'.format(', '.join(bads), s)
163​ else:
164​ detail.note = 'no problems found'
kparalUnsubmitted
Not Done

Listing all rpm file names in note can make it possibly very long. I'd say this is much more appropriate to put into the log file (where it already is). Are affected RPM filenames something that the maintainer should immediately see from the resultsdb interface, before clicking to see the log file? Is that going to be mostly useful?

I'd recommend removing the note completely, because I see no good usage for it in this particular check. However, that's just a recommendation. We'll need get used to field values which do not satisfy our aesthetic goals, or start restricting them (e.g. in length) somehow sooner or later :)

165​
166​ summary = 'python-versions {} for {} ({})'.format(
167​ outcome, koji_build, detail.note)
168​ log.info(summary)
169​
170​ output = check.export_YAML(detail)
171​ return output
172​
173​
174​if __name__ == '__main__':
175​ run('test')
View Options

runtask.yaml

  • This file was added.
1​name: python-versions
2​namespace: dist
3​desc: |
4​ Download specified Koji build and check each RPM for Python dependencies,
kparalUnsubmitted
Not Done

This could be a bit more specific (what can go wrong with the dependencies?). We have no limit on description length.

5​ if a binary RPM depends on Python 2 and 3 at the same time,
6​ it usually indicates some packaging errors, except for rare whitelisted cases.
7​maintainer: churchyard
8​
9​input:
10​ args:
11​ - koji_build
12​
13​environment:
14​ rpm:
15​ - rpm-python
16​
17​actions:
18​ - name: download rpms from koji
19​ koji:
20​ action: download
21​ koji_build: ${koji_build}
22​ arch: ['all']
23​ src: False
24​
25​ - name: check each rpm for python dependencies
26​ python:
27​ file: python_versions_check.py
28​ callable: run
29​ workdir: ${workdir}
30​ koji_build: ${koji_build}
31​ artifactsdir: ${artifactsdir}
32​ export: python_versions_output
33​
34​ - name: report results to resultsdb
35​ resultsdb:
36​ results: ${python_versions_output}