Add unversioned shebangs check #30

mcyprian · 2017-08-02T13:26:49Z

This check searches for #!/usr/bin/python and /usr/bin/env python shebangs inside RPMs. If some of these shebangs is present, check fails and the complete list of problematic files is stored into the log.
Unit and integration tests included.

hroncok · 2017-08-02T14:38:39Z

I'll review it. Could you fix the Travis failure in the meantime?

hroncok

Added a lot of nitpicking comments, but generally this looks good.

Please document the new dependency in the README (it talks about dependencies on several places).

Adding the required Fedora package (python2-libarchive-c) to the Docker image will also make the Travis run a bit faster, because the dependency will not by installed in the first integration test, but together with all other required packages. Please adjust the Dockerfile accordingly.

hroncok · 2017-08-02T14:40:39Z

setup.py

+    install_requires=['libarchive-c'],
    setup_requires=['setuptools', 'pytest-runner'],
-    tests_require=['pytest', 'pyyaml'],
+    tests_require=['pytest', 'pyyaml', 'libarchive-c'],


Is it necessary to repeat stuff from install_requires here?

hroncok · 2017-08-02T14:44:02Z

tox.ini

 deps =
    pytest
    pyyaml
+    libarchive-c


Is Python 3 libarchive-c actually needed to run the integration tests? Note that the integration tests are run by Python 3, but they call the runtask command and that runs on Python 2. I think that listing the package here is not needed, but I haven't tried it without.

Seems to work without this.

hroncok · 2017-08-02T14:47:55Z

taskotron_python_versions/unversioned_shebangs.py

+FORBIDDEN_SHEBANGS = ['#!/usr/bin/python', '#!/usr/bin/env python']
+
+if sys.version > '3':
+    def to_bytes(s):


This is Python 2:

>>> 'aaa'.encode() 'aaa'

This is Python 3:

>>> 'aaa'.encode() b'aaa'

Why don't just use that? It seems to have the same effect.

hroncok · 2017-08-02T14:49:32Z

taskotron_python_versions/unversioned_shebangs.py

+
+
+def matches(line, query):
+    return line == query or line.startswith(query + b' ')


I'd say explicitly in the docstring that this operates on bytes to avoid confusion in the future.

hroncok · 2017-08-02T14:52:20Z

taskotron_python_versions/unversioned_shebangs.py

+            try:
+                first_line = next(entry.get_blocks(), '').splitlines()[0]
+            except IndexError:
+                continue


Please indicate in the comment what's happening. Something like:

continue # empty file

hroncok · 2017-08-02T15:12:41Z

taskotron_python_versions/unversioned_shebangs.py

+    log.debug("\n\nPROBLEM RPMS: {}".format(problem_rpms))
+    for package, pkg_summary in problem_rpms.items():
+        for shebang, scripts in pkg_summary.items():
+            if scripts:


Now if you start with an empty dict in get_scripts_summary(), you will not need this if.

hroncok · 2017-08-02T15:13:37Z

taskotron_python_versions/unversioned_shebangs.py

+                outcome = 'FAILED'
+                shebang_message += \
+                    '{}\n * Scripts containing `{}` shebang:\n   {}'.format(
+                        package, shebang, '\n   '.join(scripts))


Sort the scripts for deterministic output and easier testing in the future.

hroncok · 2017-08-02T15:21:03Z

test/functional/test_unversioned_shebangs.py

+)
+from taskotron_python_versions.common import Package
+
+fixtures_dir = os.path.dirname(


See pkg and gpkg functions from common and use them instead please. That would make your code more consistent with other tests.

They work like this:

gpkg('tracer*') # -> Package('../fixtures/tracer-0.6.9-2.fc25.noarch.rpm') gpkg('tracer*').path # -> '../fixtures/tracer-0.6.9-2.fc25.noarch.rpm' pkg('tracer-0.6.9-2.fc25.noarch.rpm') # -> '../fixtures/tracer-0.6.9-2.fc25.noarch.rpm'

(Untested, but at least they work similarly to this.)

hroncok · 2017-08-02T15:24:19Z

test/functional/test_unversioned_shebangs.py

+     '#!/usr/bin/python', {'/usr/bin/tracer'}),
+    ('python3-django-1.10.7-1.fc26.noarch.rpm', '#!/usr/bin/env python',
+     {'/usr/lib/python3.6/site-packages/django/bin/django-admin.py',
+      '/usr/lib/python3.6/site-packages/django/conf/project_template/manage.py-tpl'}),


Note that this is a template and the shebang there is probably intentional, but that is probably not important right now.

hroncok · 2017-08-02T15:25:47Z

test/functional/test_unversioned_shebangs.py

+@pytest.mark.parametrize(('path', 'expected'), (
+    ('tracer-0.6.9-2.fc25.noarch.rpm',
+     {'#!/usr/bin/python': {'/usr/bin/tracer'},
+      '#!/usr/bin/env python': set()}),


This will need to be updated once the empty sets will not be there at all.

hroncok · 2017-08-02T15:39:45Z

Plase add commits (do not squash), so it's easier for me to check what has changed. Later, we can squash the PR once approved.

hroncok · 2017-08-03T12:29:34Z

setup.py

    author='Miro Hrončok, Iryna Shcherbina, Michal Cyprian',
-    author_email='mhroncok@redhat.com, ishcherb@redhat.com, mcyprian@redhat.com',
+    author_email=('mhroncok@redhat.com, ishcherb@redhat.com,'
+                  'mcyprian@redhat.com'),


There's a missing space now and the string is: 'mhroncok@redhat.com, ishcherb@redhat.com,mcyprian@redhat.com'

mcyprian · 2017-08-03T19:30:48Z

Thanks for the review @hroncok, I've tried to make all the changes you requested.

hroncok

Thanks for the changes.

I've added a bit in mcyprian#1

Also, please update the README.

irushchyshyn · 2017-08-04T12:07:16Z

taskotron_python_versions/unversioned_shebangs.py

+
+from .common import log, write_to_artifact
+
+MESSAGE = """These RPMs contains problematic shebang in some of the scripts:


nitpick:) contains -> contain

hroncok

Once more PR form me mcyprian#2 and we can squash and merge.

This check searches for #!/usr/bin/python and /usr/bin/env python shebangs inside RPM packages. If some of these shebangs is present, check fails and the complete list of problematic files is stored into the log. Resolves fedora-python#28.

hroncok · 2017-08-07T09:08:39Z

Thanks.

mcyprian requested a review from hroncok August 2, 2017 13:26

hroncok suggested changes Aug 2, 2017

View reviewed changes

hroncok reviewed Aug 3, 2017

View reviewed changes

mcyprian force-pushed the unversioned_shebangs branch from 1ee943d to 8874c6f Compare August 3, 2017 19:15

hroncok suggested changes Aug 4, 2017

View reviewed changes

irushchyshyn reviewed Aug 4, 2017

View reviewed changes

mcyprian force-pushed the unversioned_shebangs branch 2 times, most recently from 10259c0 to affc096 Compare August 4, 2017 13:01

hroncok suggested changes Aug 4, 2017

View reviewed changes

hroncok added the enhancement label Aug 4, 2017

Add unversioned shebangs check

c2b8df0

This check searches for #!/usr/bin/python and /usr/bin/env python shebangs inside RPM packages. If some of these shebangs is present, check fails and the complete list of problematic files is stored into the log. Resolves fedora-python#28.

mcyprian force-pushed the unversioned_shebangs branch from 132ddd0 to c2b8df0 Compare August 7, 2017 07:07

hroncok approved these changes Aug 7, 2017

View reviewed changes

hroncok merged commit c2b8df0 into fedora-python:develop Aug 7, 2017



		def matches(line, query):
		return line == query or line.startswith(query + b' ')


		from .common import log, write_to_artifact

		MESSAGE = """These RPMs contains problematic shebang in some of the scripts:

Add unversioned shebangs check #30

Add unversioned shebangs check #30

Uh oh!

Conversation

mcyprian commented Aug 2, 2017

Uh oh!

hroncok commented Aug 2, 2017

Uh oh!

hroncok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hroncok Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hroncok commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcyprian commented Aug 3, 2017

Uh oh!

hroncok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hroncok left a comment

Choose a reason for hiding this comment

Uh oh!

hroncok commented Aug 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hroncok Aug 2, 2017 •

edited

Loading

hroncok commented Aug 2, 2017 •

edited

Loading