Add Github Actions CI/CD for build, tests, and PyPI release #10

pirate · 2020-04-17T20:43:19Z

Key Points

does everything in Github Actions ubuntu environment, does not use standalone Dockerfile (to match style use by other OpenZIM projects)
installs C++ and Cython build environment, and builds python-libzim against libzim binary release dylib
checks code style and syntax with black and flake8
runs python-libzim unit test suite with pytest (also supports tox)
automatically pushes any new tagged releases to PyPI (using pypi API key)

TODOs

setup PyPI API key in repository secrets for pypa/gh-action-pypi-publish to use
limited support to Python 3.6, 3.7, and 3.8
remove libzim compilation in favor using the latest released binary
setup pyproject.toml + pytest + tox for Python tests, cython build, sdist/bdist packaging, and PyPI release process
~~set up statically linked cross-platform distribution with some combination of cibuildwheel, manylinux, auditwheel~~ (not needed for now, just do binary-based Linux x64 build cc @Kelson)
- add libzim build process back to github action? (needed to cross-platform compilation)
- https://github.com/pypa/auditwheel
- https://github.com/grantjenks/python-c2f
- https://github.com/pypa/manylinux
- https://github.com/RalfG/python-wheels-manylinux-build
- https://github.com/FedericoStra/cython-package-example

rgaudin · 2020-04-17T21:30:28Z

Please remove py35, we don't want to support it.
Didn't @kelson42 asked to build using the released libzim binary?

pirate · 2020-04-17T22:34:37Z

We just changed it to using the pre-built release of libzim, but there's one note: we wont be able to test with CI/CD on macOS/Windows automatically, as there are only pre-built libzim releases available for Linux at the moment here: http://download.openzim.org/release/libzim/libzim_linux-x86_64-6.1.1.tar.gz

The alternative is to set up https://cibuildwheel.readthedocs.io/en/stable/options/ and keep the libzim compilation in this build, that way the needed libzim binaries will be bundled with the PyPI package directly in the wheels for each platform, and users wont have to mess around with installing and dynamically linking libzim on their own.

MANIFEST.ini

mgautierfr · 2020-04-21T13:16:13Z

Please rebase this PR on master, not on cython-blend.

pirate · 2020-04-21T20:34:36Z

@mgautierfr this is rebased on master and ready for review. The tests are failing until libzim_api.h is merged into master from #3, but almost everything else should work as expected.

I suggest we try to get this merged into master pretty much as-is (small suggestions welcome of course), and work on any big changes as separate PRs against master. It will make it easier to coordinate work in other branches since this PR changes the testing system a little and the other branches need that work to continue.

.github/workflows/test.yml

mgautierfr · 2020-04-22T11:05:55Z

@mgautierfr this is rebased on master and ready for review. The tests are failing until libzim_api.h is merged into master from #3, but almost everything else should work as expected.

libzim_api.h is generated by Cython. It should not be in the repository. In fact, I have remove them from the code in master and in the PR #6.

mgautierfr

See the individual comments.

It is not a big deal, but as I said in a previous comment, if we are using pytest to run the tests, it would be a good thing (and easier) to also use pytest to write the test (instead of unittest).

.github/workflows/release.yml

.github/workflows/test.yml

MANIFEST.ini

setup.py

mgautierfr · 2020-04-22T11:31:58Z

setup.py

-    return open(os.path.join(os.path.dirname(__file__), fname)).read()
+
+PACKAGE_NAME = "libzim"
+VERSION = "6.1.1"  # pegged to be the same version as libzim since they are always released together


THis shouldn't be the same.
A minor version change of libzim doesn't need a new version of the wrapper.
And you may want to fix things in the wrapper even if libzim doesn't change.

In the NPM world it's common to pin the binding package version to the same version as the main library, and then use a script to bump the versions of both whenever either one wants to issue a new release. (e.g. in this way redux==1.2.3 is always guaranteed to work with react-redux==1.2.3, react-redux-addons==1.2.3, etc. and users don't have to look up compatibility charts or wonder if they have the correct versions installed)

I don't have a strong preference either way though, so I don't mind changing it.

How do they do in NPM to update the wrapper even if the library has not changed ?

They have a script to bump both versions anytime either one changes.

Want me to reset this to 0.0.1, or keep it as is?

FYI the project name on pypi has been reserved by making a 0.0.1 so whatever version we release first much be different.

tests/test_libzim.py

Pipfile

tests/test_libzim.py

pirate · 2020-04-23T02:28:47Z

Note: the build is failing because of these two files.

would reformat /home/runner/work/python-libzim/python-libzim/examples/basic.py
would reformat /home/runner/work/python-libzim/python-libzim/tests/test_libzim.py

I don't want to reformat these files with black in this PR, as it will create a lot of unnecessary conflicts with our changes to those files in other branches.
We should wait till there is a complete stable version merged in master, then reformat all the code at once in one PR.

jdcaballerov · 2020-04-23T13:00:33Z

@mgautierfr this is rebased on master and ready for review. The tests are failing until libzim_api.h is merged into master from #3, but almost everything else should work as expected.

libzim_api.h is generated by Cython. It should not be in the repository. In fact, I have remove them from the code in master and in the PR #6.

It is recommended to include the generated files so that the user doesn't need to have Cython installed.

kelson42 · 2020-04-23T13:55:14Z

@mgautierfr this is rebased on master and ready for review. The tests are failing until libzim_api.h is merged into master from #3, but almost everything else should work as expected.

libzim_api.h is generated by Cython. It should not be in the repository. In fact, I have remove them from the code in master and in the PR #6.

It is recommended to include the generated files so that the user doesn't need to have Cython installed.

Nothing which is automatically generated should be in the git. Cython is a strong dependence and it should be clearly written in the README.md, but any developer has to have it installed.

setup.py

pirate · 2020-04-27T19:24:23Z

Nothing which is automatically generated should be in the git. Cython is a strong dependence and it should be clearly written in the README.md, but any developer has to have it installed.

@kelson42 this is the case right now because we're only distributing bdist (binary builds), but assuming the goal is to change that in the future, the recommended best practice is to ship the headers and autogenerated cython code in the sdist so that users who want to install from source can do so without needing a whole Cython developer environment.

If you are 100% you don't want users installing via sdist and force PyPI users to only use the bdist binary distribution, then we can remove it, but note that this is fairly unusual for PyPI projects (where the assumption is that all projects have both an sdist and bdist).

rgaudin · 2020-04-27T19:53:47Z

If you are 100% you don't want users installing via sdist and force PyPI users to only use the bdist binary distribution, then we can remove it, but note that this is fairly unusual for PyPI projects (where the assumption is that all projects have both an sdist and bdist).

sdist is not a source release, it’s a source distribution for setup tools which is not a complex system like debian debs. Of all the dependencies I’ve used that required install-time compilation, none ever required cython or similar.
I think @kelson42 ’s comment was general as he may not be familiar with this.
As a reminder, this is generated and put into the sdist archive but not stored in the repo obviously.

pirate · 2020-04-27T20:21:58Z

Ok so just to be clear, the conclusion is:

keep the autogenerated files in the sdist so that cython is not needed during install-time compilation
don't keep the autogenerated files in git (they must be regenerated anytime a new sdist is rolled, and the machine doing the release will have Cython as a requirement)

I can push the change today.

rgaudin · 2020-04-27T20:28:37Z

Ok so just to be clear, the conclusion is:

keep the autogenerated files in the sdist so that cython is not needed during install-time compilation

don't keep the autogenerated files in git (they must be regenerated anytime a new sdist is rolled, and the machine doing the release will have Cython as a requirement)

I can push the change today.

@mgautierfr makes the decision but that’s how it should be done in my opinion, yes. that’s the expected python/pip behavior.

pirate · 2020-04-27T20:29:25Z

Ok that should now be done √:
.gitignore:

...
# Files autogenerated by Cython
libzim/libzim.cpp
libzim/libzim.h
libzim/libzim_api.h

MANIFEST.in:

...
# include the cython-generated libzim and libzim_api cpp+headers in the sdist
recursive-include libzim *

afreydev · 2020-05-12T16:49:07Z

I have opened an issue in libzim project related to the segmentation fault problem in the test github actions workflow:
openzim/libzim#333

afreydev · 2020-05-13T16:07:14Z

Would it be possible complete this workflow by using compiled source (instead of pre-built) because this (openzim/libzim#333)? @mgautierfr

mgautierfr · 2020-05-15T11:32:36Z

It would be difficult. The compilation of libzim should not be handle by the wrapper. There is kiwix-build for that.
openzim/libzim#333 should simply be fixed.

The PR is pretty old, it should be rebase on master (where the code has changed since). And ideally with the intermediate (wrong) commits removed.

mgautierfr · 2020-05-26T11:55:58Z

Seems good. We may change few things in the future but it is good for now and this PR is open since a long time now.
If I have things to change, I will open another PR.

pirate changed the base branch from master to cython-blend April 17, 2020 20:48

pirate assigned afreydev Apr 17, 2020

pirate force-pushed the pirate-patch-1 branch from 2b98792 to 89d884a Compare April 17, 2020 22:33

pirate requested review from jdcaballerov and afreydev April 18, 2020 10:19

pirate commented Apr 18, 2020

View reviewed changes

MANIFEST.ini Outdated Show resolved Hide resolved

pirate self-assigned this Apr 18, 2020

pirate force-pushed the pirate-patch-1 branch 6 times, most recently from 722215a to 4b82ec1 Compare April 21, 2020 20:26

pirate changed the base branch from cython-blend to master April 21, 2020 20:26

pirate force-pushed the pirate-patch-1 branch from 4b82ec1 to f3d3be2 Compare April 21, 2020 20:30

pirate requested a review from mgautierfr April 21, 2020 20:33

pirate force-pushed the pirate-patch-1 branch 2 times, most recently from 958c7e4 to 50071a0 Compare April 21, 2020 20:37

pirate commented Apr 22, 2020

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

mgautierfr requested changes Apr 22, 2020

View reviewed changes

pirate commented Apr 23, 2020

View reviewed changes

tests/test_libzim.py Outdated Show resolved Hide resolved

mgautierfr reviewed Apr 24, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

jdcaballerov mentioned this pull request Apr 25, 2020

Tests must not write zim files to "/opt" #15

Closed

pirate force-pushed the pirate-patch-1 branch 2 times, most recently from c19d678 to 75911cd Compare April 29, 2020 00:48

afreydev force-pushed the pirate-patch-1 branch 3 times, most recently from d89a3f9 to 039073d Compare May 18, 2020 18:01

add Github actions for lint, test, and release

c0080db

afreydev force-pushed the pirate-patch-1 branch from 039073d to c0080db Compare May 18, 2020 18:14

mgautierfr mentioned this pull request May 26, 2020

Using pre-built libzim libraries causes segmentation fault in pytest openzim/libzim#333

Closed

mgautierfr force-pushed the pirate-patch-1 branch from 0e9adb2 to 96b5752 Compare May 26, 2020 11:34

Format code using black and flake8.

ee55969

mgautierfr force-pushed the pirate-patch-1 branch from 96b5752 to ee55969 Compare May 26, 2020 11:37

mgautierfr merged commit 1ba2808 into master May 26, 2020

mgautierfr deleted the pirate-patch-1 branch May 26, 2020 11:56

Uh oh!

Add Github Actions CI/CD for build, tests, and PyPI release #10

Add Github Actions CI/CD for build, tests, and PyPI release #10

Uh oh!

Conversation

pirate commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key Points

TODOs

Uh oh!

rgaudin commented Apr 17, 2020

Uh oh!

pirate commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mgautierfr commented Apr 21, 2020

Uh oh!

pirate commented Apr 21, 2020

Uh oh!

Uh oh!

mgautierfr commented Apr 22, 2020

Uh oh!

mgautierfr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgautierfr Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

pirate Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

mgautierfr Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

pirate Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

pirate Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

rgaudin Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pirate commented Apr 23, 2020

Uh oh!

jdcaballerov commented Apr 23, 2020

Uh oh!

kelson42 commented Apr 23, 2020

Uh oh!

Uh oh!

pirate commented Apr 27, 2020

Uh oh!

rgaudin commented Apr 27, 2020

Uh oh!

pirate commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgaudin commented Apr 27, 2020

Uh oh!

pirate commented Apr 27, 2020

Uh oh!

afreydev commented May 12, 2020

Uh oh!

afreydev commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgautierfr commented May 15, 2020

Uh oh!

mgautierfr commented May 26, 2020

pirate commented Apr 17, 2020 •

edited

Loading

pirate commented Apr 17, 2020 •

edited

Loading

pirate commented Apr 27, 2020 •

edited

Loading

afreydev commented May 13, 2020 •

edited

Loading