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

Package for Debian (.deb) #56

Merged
merged 2 commits into from Jan 7, 2018
Merged

Package for Debian (.deb) #56

merged 2 commits into from Jan 7, 2018

Conversation

rgson
Copy link
Contributor

@rgson rgson commented Sep 25, 2017

I've added a basic configuration to build .deb packages of the module for Debian-based systems.

The configurations is not complete enough to get accepted into the official Debian repository, but it ought to suffice if someone wanted to set up a PPA, for example. Personally, I need the .deb-packaged version to be able to distribute the module in a particularly Debian-centric environment.

Pybuild does the heavy lifting based on your existing setup.py file. A single command, which I've encapsulated in the Makefile, is enough to build the packages for both Python 2 and 3.

If this is merged, the Maintainer field (line 4 in debian/control) should probably be changed to (one of) the repository maintainers from here.

@codecov
Copy link

codecov bot commented Sep 25, 2017

Codecov Report

Merging #56 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #56   +/-   ##
=======================================
  Coverage   99.19%   99.19%           
=======================================
  Files           8        8           
  Lines         874      874           
=======================================
  Hits          867      867           
  Misses          7        7

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e428ed...e62873b. Read the comment docs.

@yakky
Copy link
Member

yakky commented Dec 30, 2017

As debian users ourselves it's a really nice addition. My only concern is maintenability over time: are you aware of any way to test package building on travis?

@rgson
Copy link
Contributor Author

rgson commented Dec 30, 2017

Glad to hear that it's an appreciated addition.

I've never tried building the packages through Travis, but based on some quick reading it seems to be doable without too much trouble. I'll give it a shot in the near future and report back.

@yakky
Copy link
Member

yakky commented Dec 31, 2017

@rgson that would be awesome!

@rgson
Copy link
Contributor Author

rgson commented Dec 31, 2017

@yakky So I've actually had a chance to test this out already and it works just fine. However, a decision must be made before I can propose the change.

The default Travis container does not have the .deb build dependencies installed. It's easy enough to install them, but there seems to be two approaches to how that's done.

The first approach uses sudo to installs the dependencies defined in the debian/control file, as usual. Unfortunately, Travis only allows sudo access on the slower VMs, leading to a rather annoying delay before each new build is started.

The second approach is sudoless, but requires the build dependencies to be listed in the .travis.yml file (to grab the packages off of Travis' apt whitelist). A build starts almost instantaneously like they currently do, but the build dependencies are duplicated across two files, which is of course suboptimal from a maintenance perspective.

Personally, I lean towards the second (sudoless) approach. I expect the build dependencies to be pretty much invariable, leading me to believe that the delayed builds would be the bigger nuisance. Ultimately it's your decision, though.

@yakky
Copy link
Member

yakky commented Dec 31, 2017

@rgson thanks for taking time to investigate. The second solution looks perfectly sensible. build-deps are not going to change often and they are going to be spotted quickly because of the test failures

@yakky
Copy link
Member

yakky commented Dec 31, 2017

@rgson I saw the failure on travis on the package building tests
probably pyjwkest and python-dateutil must be declared as dependencies

@rgson
Copy link
Contributor Author

rgson commented Jan 1, 2018

@yakky Huh, that's surprising. My fork builds fine on Travis. 🤔 I'll investigate it tomorrow.

@yakky
Copy link
Member

yakky commented Jan 1, 2018

I'll fork this and I'll give it a try myself
Long time has passed since I knew how to do Debian packages
I need to refresh my knowledge a bit!

@rgson
Copy link
Contributor Author

rgson commented Jan 1, 2018

@yakky The reason it works on my fork is that I forgot to pull the new upstream commits. 🤦‍♂️ Commit b2ea550 turns the runtime dependencies into build dependencies.

Indeed, pyjwkest and python-dateutil (and some others) should be declared as build dependencies then. Unfortunately, it's not as straight-forward as one would wish.

Firstly, pyjwkest is not available in the Debian/Ubuntu repositories. To formally satisfy that dependency, I'd have to build .deb packages for that, too. It's actually not strictly required at build-time since the imports are conditional, but it really should've been specified as an install-time dependency already (I guess pybuild doesn't understand the git-based dependency in requirements.txt), leading back to the same problem.

Secondly, python-urllib3 (used through python-requests) is too outdated on Travis' version of Ubuntu (14.04). So while that dependency can be satisfied on newer distros, it won't work on Travis. Of course, working versions of the dependencies can be installed through pip the same way it's done for the tests, but the dependencies listed in debian/control would still be seen as unfulfilled.

I'll probably end up making some minor changes to turn pyjwkest into an optional dependency by disabling the affected features if it's absent. I believe it should be fairly easy, seeing how I've been using python-taiga without pyjwkest for a while without even realizing. That would save me, or at least postpone, the grief of packaging yet another module.

The python-urllib3 problem ought to be solvable by pulling a newer version from a non-default software repository. That would probably require the builds to run on the slower VMs with root privileges, though.

If you have any other suggestions, I'm all ears.

@yakky
Copy link
Member

yakky commented Jan 1, 2018

@rgson
Sorry this is turning out more complicated than anticipated

  1. pyjwktest is basically already an optional requirement as it's only used here
    if cyphered_token:
    catching exception and failing gracefully if not present should be enough
  2. dependecies: it's perfectly possible to add new sources to cointainer-based environments https://docs.travis-ci.com/user/installing-dependencies/#Installing-Packages-on-Container-Based-Infrastructure
    Thanks for your time to contribute this

@rgson
Copy link
Contributor Author

rgson commented Jan 2, 2018

@yakky
Indeed. It seemed rather simple initially.

Good to know that I've not missed anything regarding pyjwkest. That should be easily fixed then.

As for apt sources, it's indeed possible to add any of the whitelisted sources, but no others. The only whitelisted source with a newer version of python-urllib3 seems to be Debian's unstable version (debian-sid). Adding that will, rather unsurprisingly, break the upgrade. Without apt pinning it seems impossible to grab the newer version from there.

I'll try to find a work-around.

@yakky
Copy link
Member

yakky commented Jan 2, 2018

@rgson feel free to switch to the non-container version, if this makes the tests easier

@rgson
Copy link
Contributor Author

rgson commented Jan 6, 2018

@yakky There, an up-to-date version which passes on Travis. 🎉

I did have to move to the non-container version to get the dependencies sorted. The difference in runtime is much lower now than during my previous attempts, though, so it's probably not as big of a deal as I previously thought. As a bonus, that got rid of the duplicated dependency list in .travis.yml (well, except for the modules that need updates from xenial).

@yakky
Copy link
Member

yakky commented Jan 7, 2018

@rgson this is just awesome 🙇‍♂️ thank you for your hard work

@yakky yakky merged commit 344e76f into nephila:master Jan 7, 2018
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

Successfully merging this pull request may close these issues.

None yet

2 participants