zopefoundation / zodbpickle

Fork of Python's pickle module to work with ZODB
Other
17 stars 15 forks source link

Create more wheels (manylinux and Mac OS). #33

Closed icemac closed 6 years ago

icemac commented 6 years ago

Additionally add coverage/coveralls support. The changes are taken from zope.interface.

Fixes #32.

icemac commented 6 years ago

Hm, how do I check that creating the wheels actually works? Do I have to create a new tag for that?

mgedmin commented 6 years ago

I have some ideas for testing:

Be sure to delete the temporary branch from GitHub afterwards.

icemac commented 6 years ago

@jamadden wrote:

I don't like a tox environment that does coverage erase and coverage combine because it breaks under detox.

I do not use detox because I got strange errors when trying it on several projects (errors which do not happen when running plain tox.

Why/how does my change break on detox? Looking at https://hynek.me/articles/testing-packaging/ it seems be possible to have only one coverage-report section. IMHO it should not matter calling coverage erase as the first step when running coverage-report as the last step as described by Hynek Schlawack.

I would like the coverage environment to specify a percentage with --fail-under so it's doing something useful, not just for human viewing.

:+1: I am adding one.

The coveralls.io data looks weird and disorganised. Maybe this new "tree" feature they suddenly seem to have is buggy (or I don't understand it) or maybe we need to install and then run the tests (i.e., stop using 'setup.py test')?

Maybe this is because the first run was without a proper coverage config. Let's see what happens after the merge.

I don't know much about the wheel building parts, but ISTM that if we're going to be doing that, we need an appveyor.yml that does the same---it's Windows users who tend to have the most difficulty building C extensions in my experience.

Windows wheels are already there. :)

Anyway, it's all better than what we had, so basically LGTM.

Thank you.

jamadden commented 6 years ago

I do not use detox because I got strange errors when trying it on several projects (errors which do not happen when running plain tox.

Then those projects are broken 😄 There are some legit reasons why projects can't be tested in parallel (gevent is one---it spawns a number of servers that need to use fixed network ports, which obviously can conflict in parallel), but in my experience most often it's just poorly written tests that assume constant paths to temp files and such.

Why/how does my change break on detox?

I guess "break" is a strong word. It's just pointless under detox. All the environments listed in envlist are run at the same time, so the coverage environment has nothing to report because everything else is still running. At best you get partial data, depending on exactly when 'coverage erase' and 'coverage combine' happen with respect to everything else.

I understand that this project has large swaths of code that are only run under either python 2 or python 3, so having one single environment produce the canonical coverage report is not really tractable. Luckily, Coverage 4.5 was recently released, and its key feature is designed to address exactly this situation, so that both Python 2 and Python 3 can run coverage, and Python 3 can explicitly say "ignore the Python 2 files" and vice versa, allowing each individual environment (theoretically) to have 100% coverage of its own relevant code.

jamadden commented 6 years ago

Windows wheels are already there. :)

Argh, I completely missed that file. Sorry!

jamadden commented 6 years ago

TIL that environment variables are expanded in the coverage config, so it should be possible to write a .coveragerc that properly excludes the irrelevant files/lines with a bit of work, even before coverage 4.5.

mgedmin commented 6 years ago

Windows wheels are already there. :)

Argh, I completely missed that file. Sorry!

You're the third person to miss it :)

(And I was the one who added it in the first place, and then forgot!)

icemac commented 6 years ago

@mgedmin Thank you for your testing ideas. I decided to give the third one a try.