search

albertz / music-player-core

Music player core Python module
BSD 2-Clause "Simplified" License
74 stars 22 forks source link

Build wheel from travis #21

Closed n-peugnet closed 4 years ago

n-peugnet commented 4 years ago

This is to automatically deploy the package on pypi from travis. It includes the wheel package (2.5Mo so I guess that the dependencies are not statically linked).

All you need to do is add a PYPI_TOKEN variable to your travis environment variables. Then each time you push a commit with a tag travis will upload a new version on PyPi.

I removed the manual installation of boost and ffmpeg as current ubuntu versions now ships a compatible version in their repositories (this makes the builds a lot faster). I also used the language: python as this is the only way in travis to make use of the python: x.x version selection directive (previously the python and matrix directives had no effect at all). This disabled the compiler setting so I removed it also. Maybe I could add back a job matrix with an env variable to test different compilers.

Also, as I added pbr to generate the version number from git, I moved the configuration of setup.py into setup.cfg.

Fixes #13 (at least for linux_x86_64)

albertz commented 4 years ago

How exactly will the version number look like? Is it compatible with the old one? I really liked that it had the date inside the version string (time.strftime("1.%Y%m%d.%H%M%S", time.gmtime())), so it was immediately clear from when the version is. Is it possible to have the same with pbr?

n-peugnet commented 4 years ago

How exactly will the version number look like? Is it compatible with the old one? I really liked that it had the date inside the version string (time.strftime("1.%Y%m%d.%H%M%S", time.gmtime())), so it was immediately clear from when the version is. Is it possible to have the same with pbr?

Pbr reads the version from the git tag which should respect the format major.minor.patch. The reason I choose this over the current datetime version number was to be able to upload wheels for different platforms (in the future). Indeed, if we kept the datetime version it would have been different on each platform as they will be generated on each build.

I don't know how to do it otherwise. It it that much of an issue ?

albertz commented 4 years ago

I really liked that it had the date inside the version string (time.strftime("1.%Y%m%d.%H%M%S", time.gmtime())), so it was immediately clear from when the version is. Is it possible to have the same with pbr?

Pbr reads the version from the git tag which should respect the format major.minor.patch. The reason I choose this over the current datetime version number was to be able to upload wheels for different platforms (in the future). Indeed, if we kept the datetime version it would have been different on each platform as they will be generated on each build.

I don't know how to do it otherwise. It it that much of an issue ?

We could just take the date & time from the latest Git commit. That will be unique. I still prefer the date & time.

n-peugnet commented 4 years ago

We could just take the date & time from the latest Git commit. That will be unique. I still prefer the date & time.

But this would be confusing as the release version will be different from whatever will be in the git tag. Maybe we could just add a release script that updates a VERSION file (which would be read by setup.py) then commits the changes and tag the new commit with the same version number.

What do you think about this ?

n-peugnet commented 4 years ago

I added a commit to make it clearer. This way, to make a release from travis you can run python bump.py then git push on master, which will trigger Travis' deploy script

albertz commented 4 years ago

I thought that the intention was that we get a wheel automatically for every single push to GitHub? It looks much more complicated to add such tags. Why not just directly take it from the last Git commit?

albertz commented 4 years ago

Btw, for reference, here is a similar setup.py which does exactly that, i.e. it takes the last Git commit, and the date & time from that.

n-peugnet commented 4 years ago

I thought that the intention was that we get a wheel automatically for every single push to GitHub? It looks much more complicated to add such tags. Why not just directly take it from the last Git commit?

I made it that way based on the assumption that maybe we didn't want to publish a new release on every push on master and instead have the possibility to choose which commit should be published.

If we choose to publish every new commit pushed on master then we indeed don't need any tag and we can use the commit date as you suggested. If this is definitely what you want I can finish it tomorrow.

Btw, for reference, here is a similar setup.py which does exactly that, i.e. it takes the last Git commit, and the date & time from that.

Tank you for the example it will save me some time.

albertz commented 4 years ago

Looks good! Did you test? All works as expected? So you said to make it work I just need to add PYPI_TOKEN to the Travis env, nothing else?

albertz commented 4 years ago

I added the token now. Will that also trigger the upload now from any pull request or other branch? Maybe the upload should only be done if this is in the master branch?

n-peugnet commented 4 years ago

I added the token now. Will that also trigger the upload now from any pull request or other branch? Maybe the upload should only be done if this is in the master branch?

Yes only the PYPI_TOKEN is sufficient. By default only master branch is deployed. Pull requests and other branches are ignored.

I tested to install the resulting tar and wheel built on my machine in a venv and it managed to install and the example players worked without issue.

albertz commented 4 years ago

Cool. Thanks for this. I merge it in now.

albertz commented 4 years ago

There seems to be a problem, see the Travis log. HTTPError: 400 Client Error: Binary wheel 'musicplayer-1.20200131.150725-cp36-cp36m-linux_x86_64.whl' has an unsupported platform tag 'linux_x86_64'. for url: https://upload.pypi.org/legacy/

n-peugnet commented 4 years ago

Yes I just saw it, I wasn't aware of the manylinux wheel requirement on pypi. Given the external dependencies this project have it is not possible to upload a linux wheel. I guess we should just disable the bdist_wheel upload and only keep the sdist one...

albertz commented 4 years ago

Ah I see. Or we statically link it in? We could strip down FFmpeg to only audio codecs. Maybe it's not too big then?

n-peugnet commented 4 years ago

Maybe cibuildwheel could be the solution.

albertz commented 4 years ago

I don't understand. That is orthogonal to the problem, right? That would allow for building it for other targets as well.

But we still have the problem whether we have external deps or not. And this can be solved by statically linking it, right? The code which I had before was already quite close to that, only that it did not statically link.

n-peugnet commented 4 years ago

There are two problems stated from the pep:

  1. "Compiling on a more recently-released linux distribution will generally introduce dependencies on too-new versioned symbols."
  2. "a Python wheel must therefore both (a) contain binary executables and compiled code that links only to libraries with SONAMEs included in the following list: [...] (b) work on a stock CentOS 5.11"

That's why a Docker image and a tool to check and bundle third party dependencies (auditwheel) exists.

Cibuildwheel uses the docker image to build using an older tool chain and then auditwheel to bundle the dependencies into the wheel. Plus it runs the build on many targets. I managed to make a working build using this cibuildwheel.

Here is the result:

$ du dist -ha
7.9M    dist/musicplayer-1.20200203.90545-cp37-cp37m-manylinux2010_x86_64.whl
7.8M    dist/musicplayer-1.20200203.90545-cp35-cp35m-manylinux2010_i686.whl
7.9M    dist/musicplayer-1.20200203.90545-cp36-cp36m-manylinux2010_x86_64.whl
7.8M    dist/musicplayer-1.20200203.90545-cp37-cp37m-manylinux2010_i686.whl
7.8M    dist/musicplayer-1.20200203.90545-cp38-cp38-manylinux2010_i686.whl
7.9M    dist/musicplayer-1.20200203.90545-cp35-cp35m-manylinux2010_x86_64.whl
7.9M    dist/musicplayer-1.20200203.90545-cp38-cp38-manylinux2010_x86_64.whl
7.8M    dist/musicplayer-1.20200203.90545-cp36-cp36m-manylinux2010_i686.whl
albertz commented 4 years ago

In those wheels, all the dependencies (FFMpeg mostly, right?) are bundled inside, and linked with a relative path? The resulting wheel files look quite small to me. Is that correct? I would not have expected that FFMpeg with all options turned on + other deps + this music-player module itself is so small.

And there is even still much room to get it much smaller, by disabling all the video part of FFMpeg, and also most of the encoders, and maybe other things we do not need.

Anyway, do you think this is ready? Can you maybe do another pull request with this? Or what are the problems now?

n-peugnet commented 4 years ago

There are still problems. I uploaded the resulting wheels on https://test.pypi.org/project/musicplayer-test-wheel/#files. But when I install the lib with pip install -i https://test.pypi.org/simple/ musicplayer-test-wheel I get the error unknown option character l with every demo files and no sound is playing. Unfortunately the error is not very explicit.

Here is the content of one of the uncompressed wheels:

├── [4.0K]  .libsmusicplayer
│   ├── [1017K]  libasound-dc1b785a.so.2.0.0
│   ├── [ 10M]  libavcodec-450aada1.so.56.60.100
│   ├── [1.9M]  libavformat-e025101f.so.56.40.101
│   ├── [336K]  libavutil-2f5e8ff5.so.54.31.100
│   ├── [ 70K]  libchromaprint-d8bc0adb.so.0.2.1
│   ├── [1.0M]  libfftw3-3a26e910.so.3.2.3
│   ├── [ 96K]  libjack-34dad952.so.0.0.28
│   ├── [196K]  libportaudio-126f40f2.so.2.0.0
│   └── [109K]  libswresample-4711ef87.so.1.2.101
├── [4.4M]  musicplayer.cpython-36m-x86_64-linux-gnu.so
└── [4.0K]  musicplayer_test_wheel-1.20200203.93933.dist-info
    ├── [9.4K]  METADATA
    ├── [1.4K]  RECORD
    ├── [  12]  top_level.txt
    └── [ 112]  WHEEL
albertz commented 4 years ago

Searching for this error (unknown option character l) yields some Jack related errors (e.g. here, here, here, here). Why is there JACK anyway in it? PortAudio is used for the audio playback, and I guess PortAudio could/might use JACK. However, it should also support all other relevant backends, like ALSA. Maybe PortAudio is configured wrongly here?

n-peugnet commented 4 years ago

On the CentOs build image, libjack is installed by the portaudio-devel package: --> Processing Dependency: libjack.so.0()(64bit) for package: portaudio-19-9.el6.x86_64.

Is seems that auditwheels recursively includes the librairies. I guess that libportaudio.so includes libjack.so and that is why it is bundled. Maybe there is a way not to install it by using a different portaudio package.

albertz commented 4 years ago

You can try demo_dumpwav.py, which just dumps the audio to a wav file, instead of playing it. So that way we can verify whether all the remaining stuff works fine (except of playing / audio output, i.e. PortAudio).

n-peugnet commented 4 years ago

Okay I'll do that right now. Otherwise maybe we could try to compile portaudio from source without Jack as in this srackoverflow answer