Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Packaging #87

Closed
jayvdb opened this issue Jun 8, 2020 · 3 comments
Closed

Packaging #87

jayvdb opened this issue Jun 8, 2020 · 3 comments

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Jun 8, 2020

With my other hat on, I am a volunteer packager for openSUSE, and am looking to submit drf-spectacular for including in its main django collection. It is easy to maintain in my package collection, as I have a large set of django packages, of varying quality. But it is worth polishing and getting into the main django collection of openSUSE so it is easier to adopt by others.

The main problems at the moment are

  • the sdist on PyPI should contains the tests, and the PyPI tests are runnable,
  • the tests skip packages which are optional,
  • dependency versions should be kept lower where possible, especially for optionals

None of them prevent distro packaging, but they each help the packager in different ways, and drf-spectacular team can pick and choose which ones they want.

Most users will now be pulling wheels from PyPI, so including tests in the sdist has very few downsides. It typically requires a few extra lines in MANIFEST.in. Python packaging tools fetch releases from PyPI, not GitHub. However packagers can list a GitHub archive as the source, with a version variable in the URL, and some tools will help fetching the tarball. But it isnt as pleasant an experience.

drf-spectacular integration tests with other DRF extensions is great, but distros usually wont have every DRF extension, and doesnt want them all, and doesnt want their deps. e.g. django-oauth-toolkit has no chance of getting into a distro, as it is actively unmaintained and assumed to be a bundle of security problems rolled into one.

And a packager doesn't want to create packages for every DRF extension that drf-spectacular supports, as that typically means getting all of their deps and their tests also working. Doing that on each release of drf-spectacular will make distro maintenance a quite daunting task.

The easy fix is for packagers to delete the test file before running the tests. I have done that with the current .spec. Another is to build a long pytest -k 'not (djangorestframework_camel_case or ...)' . Better would be if there is a single flag that tells the test runner / test code to use skip if an optional dependency is missing, and that single flag keeps working across releases. Something like export TESTS_ALLOW_MISSING=1.

My tests pass with djangorestframework-simplejwt 4.3.0 , while requirements/optionals.txt says >=4.4.0. This is a messy problem for dev projects, with conflicting needs, as it is hard to validate lower versions. One approach is to replace >= with == in an extra CI job - doing that within tox is harder.

@tfranzel
Copy link
Owner

tfranzel commented Jun 9, 2020

@jayvdb aweseome! i like it and most of it should be relatively easy to achieve. i only have very minimal knowledge of the packaging process so that very helpful feedback. i'll look into point 1&2. regarding point 3. the main deps are already minimal requirements. the optionals i "fixed" at the
current version when i introduced them. they could potentially be lowered. that is correct.

i noticed that @fladi has added a debian package. thanks for that! @fladi do you have any further comment for making packaging more pleasant?

@fladi
Copy link

fladi commented Jun 12, 2020

Having the tests included in the source tarball would indeed be nice, but my Debian package uses the release tags from github so for me it's not that of an issue.

I patched your tests to use pytest.importorskip("...") to skip those for integrations that are not yet packaged for Debian. If you see this as an useful feature, I can turn my patch into a PR.

Other that this, there's nothing I woul dsee as an issue for packaging.

@tfranzel
Copy link
Owner

@fladi thanks for your feedback! i will be working on the sdist next.

i guess there is not need for you to put work in a PR. the commit above already adds that option. ./runtests.py --skip-missing-contrib (also works on pytest). i figured that by default the tests should fail on missing packages. with that option, tests run for what is available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants