setup.py: build 'universal' py2.py3 wheels

[anthrotype]

the afdko doesn't include any extenstion modules, only native C executables. These are independent from the Python version, they only depend on the system's platform and architecture (32 vs 64 bit).

You can greatly reduce the number of wheel packages that you build and publish if you name the wheels with a "universal" py2.py3 tag, an ABI tag of "none" (meaning it does not depend on CPython ABI), and with a platform tag that reflects the different platforms and architecture supported (Linux, MacOS, Windows, 32 vs 64 bit, etc. all automatically selected by bdist_wheel command).

I used the same approach in ttfautohint-py, where I bundle a DLL of libttfautohint (wrapped via ctypes). It reduced the number of wheels I publish to only 4: 1 for macOS (a fat "intel" binary including both 32 and 64 bits); 1 for linux (only the manylinux1_x86_64, the i686 or 32-bit version is not really needed on linux desktop any more); and finally 2 for windows, win32 and win_amd64 (on Windows, 32 bit Pythons are still a thing, since the default url from Python.org is still the win32 version, but I encourage everyone to download the 64 bit version instead).

Since the wheel is universal, you can build it either with py27 or with py37 or whatever. It will be the same.

[anthrotype]

unlike the case of a DLL that is dynamically loaded by the current process and thus needs to have the same bitness, in your case you always build 64-bit native executables (with the exception of autohintexe, I think), and then run them as subprocesses. This means that it doesn't matter if Python interpreter was compiled for 32-bit as long as the host machine can run 64-bit executables.

This means we don't need two wheels for windows that have exactly the same content (same pure-python lib, and same 64-bit executables), but only differ because of the wheel platform tag. We can actually merge the two platform tags with a dot (like I did for the python version tag py2.py3) and make
a single wheel named afdko-2.8.0-py2.py3-none-win32.win_amd64.whl. This can be installed on either python versions and on either 32-bit or 64-bit python. Of course, if the machine architecture is really 32 bit (unlikely but possible), then the 64-bit executable won't run.

[miguelsousa]

in your case you always build 64-bit native executables

Nope. On Windows we're always building 32-bit executables. If you look at the Visual Studio files you'll see that there's no 64-bit configuration.
Not sure about the other platforms.

[anthrotype]

ah, ok (why?).
in any case, you can still produce a single win32.win_amd64 wheel for windows, containing only the 32 bit executables. They will work fine on a 64-bit machine (not the other way around) running a 64-bit Python.

[anthrotype]

looks like cibuildwheel is failing on macos because it's assuming the names of the wheels generated by the different pythons are unique, whereas here the py36 produces a wheel which is identical (both in content, and now also in the filename) to the one produced by the previos py27 environment, and the two are sharing the same filesystem.
What's the advantage of running the various envs serially one after the other, when Travis offers parallel builds and you could run both the py27 and the py36 at the same time in different isolated instances...

[miguelsousa]

ah, ok (why?).

Unfinished work.

in any case, you can still produce a single win32.win_amd64 wheel for windows, containing only the 32 bit executables. They will work fine on a 64-bit machine (not the other way around) running a 64-bit Python.

Since it's not possible to make a fat wheel for Windows, I prefer you keep the distinction between 32- and 64-bit there.

[anthrotype]

from the point of view of the afdko (where you want to build standalone executable once, with a relatively recent toolchain, unlike for python extensions without concerns with python ABI compatibility issues), the only advantage of cibuildwheel (or the similar one, multibuild) is that it makes it easier to build the manylinux1 wheels (pulls the docker image and runs the command inside the old CentOS 5 to make the wheel portable across distros). But for mac or windows it doesn't add anything to running the same commands in the Travis or Appveyor environments.

Ideally, what you'd do is: build the py2.py3 wheel only once, and then install and test it on all the supported environments. You don't need to rebuild the same thing over and over, it's going to be exactly the same if you build it under py27, py36 or py37. And when I say exactly, I mean it.

[miguelsousa]

Also, I don't think the coverage numbers will be correct if the results don't go thru a combine step. I've already spent a lot of time sorting that out.

[miguelsousa]

@anthrotype it's not clear to me what you're trying to convey in this comment but what I'm trying to say above is that the goal on Windows is to have proper binaries inside those wheels instead of using the 32-bit executables for both.

[anthrotype]

Since it's not possible to make a fat wheel for Windows, I prefer you keep the distinction between 32- and 64-bit there.

that "fat" thing is only for mac Mach-O binaries. I may be wrong but I don't think you're building 32+64 bit executables for mac. You're probably only building 64-bit ones, what xcode would default to nowadays.
Still, you're building with a Python (from python.org) that was compiled as a fat binary, and thus the generated wheel bears the platform tag macosx_10_6_intel, instead of say macosx_10_12_x86_64 (if you were building it with the Python that comes with homebrew on your mac developer machine).
However that 'intel' in the tag is kind of a lie, because it doesn't reflect the architecture of the bundled executables (most likely 64-bit). Your executables are not compiled using distutils/setuptools -- which takes care of using the same compiler toolchain and that pass the same compiler and linker flags used to build python itself. In fact, you don't need to, as you're not building a module that needs to be dlopen-ed, you're making an executable that's independent of python, so you can use whatever compiler you prefer.

Sorry i'm digressing. The point is, the afdko wheels are actually "universal" wheel, in the sense that I can take the one tagged cp27-cp27m-win32 and literally rename it cp37-cp37-win_amd64 and pip will install it just fine (despite the metadata in the wheel is not in sync with the filename..), and it will work. Because it is the same!

[anthrotype]

so what I'm proposing here is that you make the wheel filename itself reflect its content:

1. it is universal, from the point of view of the python version tag: py2.py3
2. it does not have any requirement as far as Python ABI (application binary interface, build time options for memory management or the width of unicode characters, etc.) because it doesn't link with the python interpreter at a binary level: thus, wheel abi flag set to none
3. it only depends on the platform (mac, linux, win) and architecture (32 vs 64), so it is not pure python, in fact it contains native C executables launched as sub-processes from python.

[anthrotype]

FWIW, I'm not making this things up. Other projects have used this very same approach of subclassing bdist_wheel command and force it to produce universal, yet platform-dependent wheels.
See for example https://github.com/getsentry/milksnake, which generate bindings for rust libraries using cffi.

[anthrotype]

Also, I don't think the coverage numbers will be correct if the results don't go thru a combine step. I've already spent a lot of time sorting that out.

indeed, it does look suspicious that it fails like that. I'll investigate more tomorrow.

what i was trying to say in that comment above was that cibuildwheel was primarily meant for packages that have extension module and need to build many wheels, each one with a different combination of python versions and corresponding compiler toolchains. In your case, the matrix is much simpler as I tried to explain, when you consider that the pure python part of the afdko is (or will be) both py2.py3, and that the native C part is independent of python itself.

anyway, we'll discuss this things more next week in Antwerp, I hope ;)

[miguelsousa]

so what I'm proposing here is that you make the wheel filename itself reflect its content

Sounds good. But for Windows use the architecture tag that reflects the nature of the binaries. It's fine if that tag is not present in the Mac or Linux wheels, since I expect most people are using 64-bit OSes these days anyway.

FWIW, I'm not making this things up.

Never thought you were, especially on these matters.

[miguelsousa]

anyway, we'll discuss this things more next week in Antwerp, I hope ;)

I'm not attending ATypI.

I get your point about cibuildwheel and the reduced nature of our matrix. But I really enjoy how cibuildwheel takes care of building and testing the wheel, and how uncluttered the appveyor and Travis configuration files are because of that. Is there a way to tailor cibuildwheel to our matrix? We're already using a fork for Travis.

[anthrotype]

Maybe related to the coverage combine fail after running pytest-cov with —parallel pytest-dev/pytest-cov#222

[miguelsousa]

That seems like a different failure to me, possibly something that we'll run into.

[anthrotype]

I’m pretty sure it’s the same thing. I’ve just restarted an Appveyor build from develop branch at the commit just before I created my PR branch, and you can see that with the current pytest-cov setup and the latest version of pytest-cov released two days ago the build fails with the same “No data to combine” error:
https://ci.appveyor.com/project/adobe-type-tools/afdko#L646

It appears that new pytest-cov is running combine for us, so when we call it ourselves it fails (can’t be run twice).
It doesn’t have to do with my bdist_wheel patch.

[anthrotype]

quoting from https://pytest-cov.readthedocs.io/en/latest/config.html

Note that this plugin controls some options and setting the option in the config file will have no effect. These include specifying source to be measured (source option) and all data file handling (data_file and parallel options).

which means that the parallel = True and the source = afdko in the .coveragerc file may be effectively ignored by pytest --cov.

From time to time I try to use pytest-cov in my projects, then revert back to running plain coverage.py out of frustration..

[miguelsousa]

I’m pretty sure it’s the same thing.

What I meant was that that issue is a new failure that was not happening when I made this change: abd9282

Sure, now we have one more thing to deal with, and I see two options: cap the version of pytest-cov until that issue is fixed, or not run coverage combine.

Nonetheless, my earlier comment about the parallel = True option being needed still stands, because if the tests are not run with this option the coverage results will be incorrect.

Hope this part of the discussion is clear now. 😃

[anthrotype]

Yes, I understand the need for --parallel-mode. It's just that pytest-cov seems to be handling that somewhat differently from plain coverage.py.
Maybe we need to experiment with --cov-append option:

If you need to combine the coverage of several test runs you can use the --cov-append option to append this coverage data to coverage data from previous test runs.

https://pytest-cov.readthedocs.io/en/latest/readme.html

but not now, I'll get back to this some other time.

[miguelsousa]

Maybe we need to experiment with --cov-append option

I've had my share of experimentation around coverage issues for the next six months, at least. So I'm pretty committed to not let anyone break it. 😁

[miguelsousa]

@anthrotype sort of related topic:
psautohint is now a dependency of the afdko, but it doesn't come with a cp36-win32 wheel, so it has to be built from source. That seem to cause the whl file to not have a standard name, and consequently it fails to upload to PyPI. See https://ci.appveyor.com/project/adobe-type-tools/afdko/build/1.0.1225#L1277

Any ideas on how to get an afdko Windows wheel that is correctly labeled -cp36-cp36m-win32.whl without the commit hash part?

[anthrotype]

the error from the PyPI server is because the filename contain a PEP440 local specifier (+something), which is not allowed to publish. That version string also seems to include a timestamp, and this is generated by setuptools_scm when the working directory is dirty, something is modifying the source files during the build.

you can see that the first py27 wheel bears the correct name (from the tag), whereas the subsequent py36 one has the incorrect (local timestamped) version:
https://ci.appveyor.com/project/adobe-type-tools/afdko/build/1.0.1225#L1270

not sure why this is happening..

[anthrotype]

psautohint doesn't come with a cp36-win32 wheel

we could make it build one, given how widespread using 32-bit Python on a 64-bit Windows is.

[anthrotype]

honestly, I don't understand why building psautohint from source (which is a dependency in install_requires, so it gets build in a temp folder and installed in the current environment) would affect the source files in a way that makes the working dir "dirty"...

[miguelsousa]

we could make it build one, given how widespread using 32-bit Python on a 64-bit Windows is.

I was trying to avoid asking for that since the psautohint AppVeyor build takes a long time already. But if you and @khaledhosny can cope with it for a bit —while I work on decommissioning the old autohintexe— I think it might be the simplest solution for now.

[anthrotype]

again, we can make the psautohint win32 wheel, but why does that have any influence on this? Installing a dependency from source or from a binary wheel should not modify at all the source files that are committed to version control in a way that makes setuptools_scm generate a dirty version string.

[anthrotype]

the psautohint AppVeyor build takes a long time already

we could skip the test and only build the wheel for win32 :-P

[miguelsousa]

but why does that have any influence on this?

you're asking the wrong guy

[miguelsousa]

we could skip the test and only build the wheel for win32 :-P

Skip the test suite on the py3.6/win32 config? That's fine. The tests on the other configs should be enough to catch any major problems.

[anthrotype]

the modification seems to be... stupid carriage returns :(

[miguelsousa]

Perhaps adding .plist to .gitattributes file will fix it?

[anthrotype]

which commands created that plist file?

[anthrotype]

yes, let's add plist as text format in .gitattributes with eol=lf.
you do it on master and i rebase?

[anthrotype]

err.. develop, I meant

[miguelsousa]

It may be test_get_glyph_names_mapping_names_from_goadb that messes with that file

[miguelsousa]

yeah, I'll do it

[miguelsousa]

plist change is in https://ci.appveyor.com/project/adobe-type-tools/afdko/build/1.0.1244

[anthrotype]

@miguelsousa sorry Miguel, in the rush I left a syntax error in the previous cibuildwheel PR, so that Appveyor build is probably going to fail. Let's restart after merging https://github.com/adobe-type-tools/cibuildwheel/pull/3

[anthrotype]

oh it looks like you're using the upstream cibuildwheel in that branch, so it probably will not get the SyntaxError

[miguelsousa]

yep, cancelled. Let's try again...

[miguelsousa]

The plist change didn't fix it. The wheel is still afdko-2.8.0rc1.dev3+gac8c2e1.d20180906-cp36-cp36m-win32.whl
I'll look into it tomorrow. Thanks for your help.

[anthrotype]

The plist change didn't fix it

it probably only affects line endings when you add/commit a file, not when the file is modified. Anyway, that .gitattribute modification is still good to have in nevertheless, in case one accidentally commits a plist which has CR in it.

Basiclaly, to fix this we need to find out who actually creates that plist file with the CR in it, and make it use LF only.

[miguelsousa]

plist issue fixed by da23770

[anthrotype]

I just rebased on develop.
I think it would be better if you merge this before cutting 2.8.0. Most of afdko users have Macs, and install python3 using homebrew (which is the easiest way to get it on the mac), and homebrew defaults to python 3.7 since a few months now. Having a universal py2.py3 wheel means that your users can install it on either python 2.7, 3.6 or 3.7 or whichever.

[miguelsousa]

@anthrotype I hear you, but my paranoid self really doesn't want to include this change in 2.8.0 😬

[anthrotype]

you're the boss :)
mac homebrew users will have to compile the afdko from source then, or fall back to installing it for python2

[anthrotype]

just in case it wasn't clear, this change is only in the filename of the wheel and related metadata, to make pip accept the same wheel file for both major versions of python. It is the same as taking either of the wheel files generated by python2 or python3 and rename it (as in mv) to py2.py3-none-<platform>.
You can try yourself to do that and feed it to pip, you'll see both the wheels tagged cp27 and cp36 will work interchangeably in either pythons once they have been tagged like that.
And if you unzip them and inspect the content, you'll see they are identical (as far as both pure python code and binary C executables) except in the metadata that links one to cp27 the other to cp36.

[miguelsousa]

It was all very clear before, but thanks for the additional clarification.
I honestly think this change is so important that I'm considering doing a dot-dot release with it alone, right after 2.8.0.

[miguelsousa]

@anthrotype we're now generating a win64 wheel as well (see https://github.com/adobe-type-tools/afdko/releases/tag/2.8.2a). But the win32 wheel now contains 64-bit binaries. Do you know how to fix it?

[anthrotype]

@miguelsousa I don't have a PC to check this right now, I will try when I'm back to London next week (do ping me in case I forget).
I think it should be enough to do the build the after having activated the correct visual studio environment, using e.g. the VS2017's vcvarsall.bat script with either x86 (for 32-bit) or x64 (for 64-bit).
Or there may be come configuration options in the visual studio solution files that maybe set to always compile for 64 bit -- I'm not sure. Also, cibuildwheel may itself be interfering with the build environment.

[miguelsousa]

@anthrotype thanks. Your comments gave me a few ideas to try out. It's working now. These are the changes I had to make: https://github.com/adobe-type-tools/cibuildwheel/commit/f58d6dac36f063f8442d04cdacb6bae3a333bb63