Page MenuHomePhabricator

run at least pep8 and pep257 for new changesets submitted to pywikibot/core for any user
Closed, ResolvedPublic

Description

In January 2015, jenkins stopped automatically running rules in tox.ini for non-whitelisted users. See https://lists.wikimedia.org/pipermail/pywikibot/2015-January/009192.html

A proposed workaround was to hardwire pep8 and pep257 into jenkins, like the mediawiki lint tests. That is not the direction that the CI team wish to go.

Details regarding the hardwiring approach:

pep8 runs without arguments, using the config in tox.ini for the exclude and ignore list.

pep257 doesn't have an exclude parameter (yet), but looks like they are interested in that feature:
https://github.com/GreenSteam/pep257/pull/22#issuecomment-70471875

While waiting for feedback on adding exclude to pep257, it would be nice if we didnt need an exclude list .. ;-)

date.py is being ignored by flake8, but is processed by pep257, so I've tackled that.
https://gerrit.wikimedia.org/r/#/c/185815/

pep257 is validating ez_setup.py , which I've tried to fix at the source of the problem:
https://bitbucket.org/pypa/setuptools/pull-request/117/pep8-and-pep257-compliance/diff

pep257 doesnt ignore items with '# noqa' , so it is emitting errors for interwiki.py (easy) and pywikibot/exceptions.py (messy).

Also if the job is hardwired into jenkins, and we can only maintain one match list in tox.ini , we cant reimplement the 'mandatory docstring' list without some serious cleanup first.

Details

Related Gerrit Patches:

Event Timeline

jayvdb created this task.Jan 19 2015, 10:42 AM
jayvdb claimed this task.
jayvdb raised the priority of this task from to Needs Triage.
jayvdb updated the task description. (Show Details)
jayvdb added subscribers: jayvdb, Legoktm, hashar.
Restricted Application added subscribers: Aklapper, Unknown Object (MLST). · View Herald TranscriptJan 19 2015, 10:42 AM
gerritbot added a subscriber: gerritbot.

Change 185815 had a related patch set uploaded (by John Vandenberg):
PEP8 and PEP257 fixes

https://gerrit.wikimedia.org/r/185815

Patch-For-Review

Change 185815 merged by jenkins-bot:
PEP8 and PEP257 fixes

https://gerrit.wikimedia.org/r/185815

@Legoktm pointed me towards this as where the mediawiki linter is hardwired into jenkins
https://github.com/wikimedia/integration-config/blob/master/jjb/job-templates.yaml#L138

jayvdb triaged this task as Unbreak Now! priority.Jan 23 2015, 12:33 AM

It looks like there will not be a quick resolution with the pep257 project ( https://github.com/GreenSteam/pep257/issues/97) , and I had no luck fixing the pep issues in ez_setup in setuptools.

So I request that flake8 with docstrings plugin is run on patch upload by *anyone*. No other plugins should be run, of course.

@jayvdb: This task has "Unbreak now" priority since January 2015 which seems unrealistic.
Are you working on this, or should the priority value be corrected?

My hacky idea is to have a pywikibot-core-tox-flake8 job that only runs if "tox.ini" is *not* touched for non-whitelisted users. Whitelisted users (and on gate-and-submit) we would run the normal tox-flake8 job.

Is pep257 part of flake8? or is that flake8-docstrings?

jayvdb added a comment.Apr 3 2015, 7:18 AM

@Legoktm, that sounds much better than the current situation - tox.ini changes are very rare by non-whitelisted users.

package flake8-docstrings is the wrapper around pep257.

tox.ini entry flake8-docstrings is non-voting, so ignore that one.

flake8 , flake8-py3 and flake8-docstrings-mandatory (pep257) are the important entries in tox.ini

Any decision / movement is welcome here (I repeat myself by saying that this task has "Unbreak now" priority since January 2015 so if this is not "drop anything else and fix this", please decrease the Priority value!).

Ricordisamoa lowered the priority of this task from Unbreak Now! to High.Apr 24 2015, 2:41 PM
Ricordisamoa removed a project: Patch-For-Review.
Ricordisamoa set Security to None.
jayvdb added a comment.Oct 3 2015, 1:30 AM

@hashar, this can be resolved if jenkins runs tox instead of specific commands, and runs them for non-whitelisted users (that is a separate task somewhere?).

Currently tox.ini is:

envlist = flake8,flake8-py3,flake8-docstrings-mandatory,py26,py27,py34

Do we need to change that?

jayvdb renamed this task from run pep8 and pep257 for pywikibot/core to run at least pep8 and pep257 for new changesets submitted to pywikibot/core for any user.Oct 3 2015, 1:40 AM

Currently the Zuul conf has:

- name: pywikibot/core
  test:
    - tox-flake8
    - pywikibot-core-tox-flake8-py3-jessie
    - pywikibot-core-tox-flake8-docstrings
    - pywikibot-core-tox-flake8-docstrings-mandatory
    - pywikibot-core-tox-nose
    - pywikibot-core-tox-nose34-jessie
    - tox-doc-jessie
  gate-and-submit:
    - tox-flake8
    - pywikibot-core-tox-flake8-py3-jessie
    - pywikibot-core-tox-flake8-docstrings-mandatory
    - pywikibot-core-tox-nose
    - pywikibot-core-tox-nose34-jessie
    - tox-doc-jessie

We can replace most of that with tox-jessie.

jayvdb added a comment.EditedOct 8 2015, 4:24 AM

Do we need to remove py26 from the envlist in tox.ini? If we get linting working again, it sounds like a good deal!

jayvdb updated the task description. (Show Details)Oct 8 2015, 4:53 AM

The tox envlist has:

flake8
flake8-py3
flake8-docstrings-mandatory
py26
py27
py34

Running all of them ends up taking 11 minutes IIRC. Potentially we could use detox to have them run in parallel.

python2.6 is no more available on Jessie. But we can keep a job running on Trusty where it is available. And if we keep py26 split, we can well keep py27,py34 split.

What I found out is that tox supports a specific env jenkins which is to override setting when tox detects it runs under Jenkins (i.e.: when env variable JENKINS_URL is set). We can probably abuse that feature to override envlist in Jenkins context and aggregate the lint/doc envs. So potentially:

tox.ini

[tox]
# Defaults env targets for developers
envlist = flake8,flake8-py3,flake8-docstrings-mandatory,py26,py27,py34

[testenv:jenkins]
# Override default for Jenkins
# The py26,py27,py34 run in their own individual jobs on WMF Jenkins
envlist = flake8,flake8-py3,flake8-docstrings-mandatory,doc

And In Zuul:

 - name: pywikibot/core
   test:
-    - tox-flake8
-    - pywikibot-core-tox-flake8-py3-jessie
     - pywikibot-core-tox-flake8-docstrings  # non voting
-    - pywikibot-core-tox-flake8-docstrings-mandatory
     - pywikibot-core-tox-nose
     - pywikibot-core-tox-nose34-jessie
-    - tox-doc-jessie
+    - tox-jessie  # runs envs from testenv::jenkins ie: flake8,flake8-py3,flake8-docstrings-mandatory,doc
   gate-and-submit:
-    - tox-flake8
-    - pywikibot-core-tox-flake8-py3-jessie
-    - pywikibot-core-tox-flake8-docstrings-mandatory
     - pywikibot-core-tox-nose
     - pywikibot-core-tox-nose34-jessie
-    - tox-doc-jessie
+    - tox-jessie  # runs envs from testenv::jenkins ie: flake8,flake8-py3,flake8-docstrings-mandatory,doc

That aggregates flake8/doc, but that might end up being over complicated to maintain.

Also I noticed we run the nose/nose34 envs, not the py27, py34 ones. Maybe that can be revisited as well.

jayvdb added a comment.Oct 8 2015, 9:47 AM

11 minutes is IMO acceptable. That is roughly how long the jenkins job took before the recent upgrades.

If py26 is taking a considerable amount of that timeframe, we could kill it and/or replace it with a flake8-py26 which would catch 95% of the problems -- at least flake8-py26 would catch all the easy problems ; the hard ones can be handled as post-commit fixes when someone is interested.

Note, detox on pypi says it is Python 2 only, but this commit says otherwise.

Note, detox on pypi says it is Python 2 only, but this commit says otherwise.

According to https://pypi.python.org/pypi/detox/ "supports creation of python3 and all environments supported of the underlying “tox” command".

Note, detox on pypi says it is Python 2 only, but this commit says otherwise.

According to https://pypi.python.org/pypi/detox/ "supports creation of python3 and all environments supported of the underlying “tox” command".

Right; they were in the process of changing the documentation.

To get detox to be available on the salves one needs to add it to operations/puppet.git/modules/contint/manifests/packages/python.pp just like tox is installed from that file via pip (there seems to be no debian package). Then test that that works on all 3 distributions, get it merged, then make a new image for nodepool. But that can wait, changing the tox.ini as said in T87169#1711877 is probably the next step here. Once that works with check experimental it can be enabled for everyone.

Change 245582 had a related patch set uploaded (by JanZerebecki):
Add tox default envs for WM Jenkins

https://gerrit.wikimedia.org/r/245582

Change 245582 merged by jenkins-bot:
Add tox default envs for WM Jenkins

https://gerrit.wikimedia.org/r/245582

Change 247291 had a related patch set uploaded (by JanZerebecki):
Replace some pywikibot/core jobs with the generic tox-jessie version.

https://gerrit.wikimedia.org/r/247291

Change 247291 merged by jenkins-bot:
Replace some pywikibot/core jobs with the generic tox-jessie version.

https://gerrit.wikimedia.org/r/247291

@hashar Can we add the tox-jessie job also to the check pipeline to make it run for everone? Or is something still blocking that?

We talked about this during the CI weekly and while T86168 is not yet done, it is fine to go ahead and use nodepool based jobs in the check or check-voter pipeline.

Change 247623 had a related patch set uploaded (by JanZerebecki):
pywikibot/core: also run tox-jessie in check pipeline

https://gerrit.wikimedia.org/r/247623

Change 247623 merged by jenkins-bot:
pywikibot/core: also run tox-jessie in check pipeline

https://gerrit.wikimedia.org/r/247623

The tox-jessie job now runs for non-whitelisted users. Example: https://gerrit.wikimedia.org/r/#/c/241283/ .

jayvdb closed this task as Resolved.Oct 21 2015, 8:50 PM

The tox-jessie job now runs for non-whitelisted users. Example: https://gerrit.wikimedia.org/r/#/c/241283/ .

That is a whitelisted user, but I assume it works for non-whitelisted users.

jayvdb reassigned this task from jayvdb to JanZerebecki.Oct 21 2015, 8:51 PM

Change 248714 had a related patch set uploaded (by John Vandenberg):
Add tox default envs for WM Jenkins

https://gerrit.wikimedia.org/r/248714

Change 248714 merged by jenkins-bot:
Add tox default envs for WM Jenkins

https://gerrit.wikimedia.org/r/248714