Skip to content

Conversation

lvh
Copy link
Member

@lvh lvh commented Jun 19, 2014

This adds some features for tox to test some meta stuff about PyOpenSSL. Specifically:

  • pyflakes
  • check-manifest
  • pip-review, which notifies us when there are newer packages than what we install in virtualenvs
  • pyroma, which should keep us generally informed about how good a packaging citizen we're being.

This isn't ready to be merged. It fails, and not as a false positive: MANIFEST.in is incomplete, and there's a few pyflakes warnings. I'm happy to do these on this PR, or on separate PRs.

@lvh lvh mentioned this pull request Jun 19, 2014
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ade80a1 on lvh:test-metadata into 1110bc9 on pyca:master.

@lvh
Copy link
Member Author

lvh commented Jun 23, 2014

The Travis CI build failure is actually spurious: https://travis-ci.org/pyca/pyopenssl/jobs/27969282#L890

Looks like it's probably actually something to do with the ASN1 mask change in #129?

I'll add the meta testenv to the Travis build configuration.

@lvh
Copy link
Member Author

lvh commented Jun 23, 2014

Okay, so just adding a meta testenv to the Travis build configuration won't work so easily. The .travis.yml file insists on just running python setup.py test with a bunch of different Pythons, which makes it hard to run environments that aren't about just the test suite (like the metadata).

Fortunately, all Python versions are actually always available in Travis builders; IIUC the main effect of the python config file entry is changing what the python symlink refers to. You can also just set environment variables like this:

env:
    - TOX_ENV=py26
    - TOX_ENV=py27
    - TOX_ENV=py33
    - TOX_ENV=py33
    - TOX_ENV=pypy

...and then use - tox -e $TOX_ENV. Technically you don't even need that: you can just use tox and build everything in a single Travis run, but that's undesirable for several reasons, such as:

  • It's a lot harder to tell what actually failed from the Travis reports.
  • If you have a large combination of test environments, combined with tests that occasionally take a long time (either because they always take a long time, or because the builder is busy, or because PyPI is slow, or...), you hit the Travis upper limit for a test run quicker, and Travis may just shoot a perfectly good build in the head.

So, I suggest moving .travis.yml to use tox. This doesn't affect testing if python setup.py test actually works: tox runs that internally to run the test suite. I'll publish those changes on this branch.

@lvh
Copy link
Member Author

lvh commented Jun 23, 2014

This would mean lots of changes, so I'm going to do them in separate PRs.

@dstufft
Copy link
Member

dstufft commented Jun 23, 2014

More or less the .travis.yml entry just controls which virtual environment is activated.

@lvh
Copy link
Member Author

lvh commented Jun 23, 2014

Thanks, @dstufft :) I'll push the commits I'm working on here so you get an idea what I'm doing with the travis.yml, but FWIW this shouldn't be considered for merging right now until I get those other smaller PR's landed :)

@reaperhulk
Copy link
Member

Conceptually we should review this and see what has already been done and what we need in 0.16. I'll add this to the milestone.

@reaperhulk reaperhulk added this to the 0.16 milestone Apr 15, 2015
@reaperhulk
Copy link
Member

rebase time! 💃

@hynek
Copy link
Contributor

hynek commented Apr 17, 2015

Do we want to keep them in one env? Is that to save time?

@lvh
Copy link
Member Author

lvh commented Apr 20, 2015

@hynek Yep, that's why.

@hynek
Copy link
Contributor

hynek commented Apr 20, 2015

You don’t have to pin the tools btw, if they break we wanna know.

@lvh lvh self-assigned this Apr 20, 2015
@lvh
Copy link
Member Author

lvh commented Apr 20, 2015

I've merged forward and removed the irrelevant pinnings.

@lvh
Copy link
Member Author

lvh commented Apr 20, 2015

Please note that it does find some (alleged) problems:

~/P/pyopenssl: tox -e meta                                                                                                                                                                          10:24:52
GLOB sdist-make: /Users/lvh/Projects/pyopenssl/setup.py
meta inst-nodeps: /Users/lvh/Projects/pyopenssl/.tox/dist/pyOpenSSL-0.15.1.zip
meta runtests: PYTHONHASHSEED='1899853923'
meta runtests: commands[0] | pyflakes OpenSSL
OpenSSL/crypto.py:1534: local variable 'result_code' is assigned to but never used
OpenSSL/crypto.py:2598: '_ssl' imported but unused
OpenSSL/test/test_crypto.py:17: 'PY3' imported but unused
OpenSSL/test/test_crypto.py:18: redefinition of unused 'simplefilter' from line 9
OpenSSL/test/test_crypto.py:19: redefinition of unused 'catch_warnings' from line 9
OpenSSL/test/test_crypto.py:2286: local variable 'passwd' is assigned to but never used
OpenSSL/test/test_crypto.py:2287: local variable 'p12' is assigned to but never used
OpenSSL/test/test_ssl.py:2013: local variable 'conn' is assigned to but never used
OpenSSL/test/util.py:10: 'os' imported but unused
ERROR: InvocationError: '/Users/lvh/Projects/pyopenssl/.tox/meta/bin/pyflakes OpenSSL'
meta runtests: commands[1] | check-manifest
lists of files in version control and sdist match
meta runtests: commands[2] | pip-review
meta runtests: commands[3] | pyroma -d .
------------------------------
Checking .
Found pyOpenSSL
<string>:6: (WARNING/2) Definition list ends without a blank line; unexpected unindent.
------------------------------
Your package does not have keywords data.
You are using Setuptools or Distribute but do not specify if this package is zip_safe or not. You should specify it, as it defaults to True, which you probably do not want.
------------------------------
Final rating: 9/10
Cottage Cheese
------------------------------
_________________________________________________________________________________________________ summary __________________________________________________________________________________________________
ERROR:   meta: commands failed

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.31% when pulling f7e552d on lvh:test-metadata into 9a2c732 on pyca:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.31% when pulling f7e552d on lvh:test-metadata into 9a2c732 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.31% when pulling f7e552d on lvh:test-metadata into 9a2c732 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.31% when pulling f7e552d on lvh:test-metadata into 9a2c732 on pyca:master.

@lvh
Copy link
Member Author

lvh commented Apr 20, 2015

thanks coveralls

@lvh
Copy link
Member Author

lvh commented May 18, 2015

It returns 0 on success, 1 on incorrect options and 2 if the rating is below 8. Our rating is 9, which is apparently Good Enough(TM). Alas, there doesn't appear to be an option to customize that...

@hynek
Copy link
Contributor

hynek commented May 18, 2015

So two thoughts:

  1. that shouldn’t be marked as failok then, no?
  2. how about using actually flake8 which is much more comprehesible and ignore certain errors like (E303 == too many free lines)? My vim is currently borderline unusable because of all the errors so I would love to curb that a bit.

Cc @alex and @reaperhulk I guess.

(I would also love to change the empty line rule to PEP8 but baby steps)

@reaperhulk
Copy link
Member

I like using flake8 myself. We have an env named pep8 on cryptography that runs flake8 (along with pep8-naming and @public's flake8-import-order that was built specifically for our project's import order preferences).

@lvh
Copy link
Member Author

lvh commented May 18, 2015

pyroma and flake8 check different things, no? pyroma is about packaging quality, flake8 is about code quality. They appear orthogonal.

@reaperhulk
Copy link
Member

yeah flake8 is just pyflakes + pep8 + mccabe. pyroma is definitely orthogonal.

@hynek
Copy link
Contributor

hynek commented May 18, 2015

Yes, flake8 was intended instead of pyflakes, not pyroma. Sorry for being ambiguous.

@hynek
Copy link
Contributor

hynek commented May 18, 2015

so to be clear: I think pyroma should not be in allow_failures.

@lvh
Copy link
Member Author

lvh commented May 18, 2015

OK. I will remove pyroma from allow_failures and use flake8 instead of pyflakes.

@hynek
Copy link
Contributor

hynek commented May 18, 2015

w00t!

Last step: we need to add

[flake8]
ignore = 303

to setup.cfg to ignore “too many blank lines” which are currently on purpose.

Any other suggestions that should be added?

Thanks Laurens for bearing with me. :)

@lvh
Copy link
Member Author

lvh commented May 18, 2015

done

@hynek
Copy link
Contributor

hynek commented May 18, 2015

long lines aren’t fine but that’s fortunately not what 303 means ;)

waiting for Travis now, thanks <3

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.03% when pulling 2513adf on lvh:test-metadata into 410b6d3 on pyca:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.03% when pulling 2513adf on lvh:test-metadata into 410b6d3 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.03% when pulling 2513adf on lvh:test-metadata into 410b6d3 on pyca:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.03% when pulling 2513adf on lvh:test-metadata into 410b6d3 on pyca:master.

@lvh
Copy link
Member Author

lvh commented May 18, 2015

:shipit:

@hynek
Copy link
Contributor

hynek commented May 19, 2015

Damit, it has to be E303, not 303 in setup.cfg. :( And I wondered why the errors still occurred! Sorryyy!

@lvh
Copy link
Member Author

lvh commented May 19, 2015

done

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.03% when pulling 98316d7 on lvh:test-metadata into 410b6d3 on pyca:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.03% when pulling 98316d7 on lvh:test-metadata into 410b6d3 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.03% when pulling 98316d7 on lvh:test-metadata into 410b6d3 on pyca:master.

hynek added a commit that referenced this pull request May 19, 2015
Add a meta testenv for metadata: style & packaging
@hynek hynek merged commit 389f78b into pyca:master May 19, 2015
@hynek
Copy link
Contributor

hynek commented May 19, 2015

thanks, let’s finish this clusterfuck until I get some insane ideas :)

@lvh
Copy link
Member Author

lvh commented May 19, 2015

revert merge button watching intensifies

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants