-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add build script to create .deb packages for Debian/Ubuntu style systems using FPM, changes to setup.py to facilitate this #73
Conversation
README.md
Outdated
* `apt-get -f install` | ||
|
||
Install any missing python requirements that don't have system packages. | ||
* `pip install "stomp.py>=3.1.1" python-daemon python-ldap dirq` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these have Ubuntu packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stomp.py, no - there is a confusingly named python-stompy but its a complete different library
dirq - seemingly not
there does look like there are suitable packages for python-ldap and python-daemon, so I'll use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly I just wanted to understand the differences. Can discuss later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I remember now, the python python-ldap
requires the system python-ldap
headers in order to install. That's why it's listed here and in the build script as a dependency.
And python-daemon is an optional requirement, so should stay out of the package dependencies as per #70.
I'll reword this line to be clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Infact, system python-ldap
seems to be sufficient!
README.md
Outdated
## Installing the DEB | ||
|
||
### Installation | ||
Install APEL SSM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the following "Install..." lines should prob end in a colon as the code bit is a follow on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
scripts/ssm-build-deb.sh
Outdated
@@ -0,0 +1,84 @@ | |||
#!/bin/bash | |||
|
|||
# Execute the following as root to install lintian and fpm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Colon at end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
scripts/ssm-build-deb.sh
Outdated
# apt-get install ruby ruby-dev rubygems build-essential | ||
# gem install --no-ri --no-rdoc fpm | ||
|
||
# Then run this file, altering the version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the version in TAG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment clarified
scripts/ssm-build-deb.sh
Outdated
# This file will create two versions of the deb file: | ||
# - apel-ssm_<version>_all.deb contains all the files necessary to run a | ||
# the SSM as a sender. | ||
# - apel-ssm-service_<version>_amd64.deb will install service files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this one AMD 64 specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is the more system specific / service files. The exact value should probably be a configurable option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you mean just because this is what we use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although it looks like we can package it as all
, the deb equivalent of noarch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, that would seem to be more consistent with the RPMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chaged
scripts/ssm-build-deb.sh
Outdated
|
||
# Split the tag into version and package number | ||
# so they can be passed to fpm separately. | ||
# This will work with RCs of the form X.Y.Z-0.A.rcA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And any other package that uses our version scheme? e.g 1.2.3-1.1.alpha1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should do, I think it will split on the -
. If it does I'll clarify the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clarified
scripts/ssm-build-deb.sh
Outdated
-v $VERSION \ | ||
--iteration $ITERATION \ | ||
-m "Apel Administrators <apel-admins@stfc.ac.uk>" \ | ||
--description "Secure Stomp Messenger (SSM) unit files." \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the system files that let you do service apel-ssm start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought there weren't any for Ubuntu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No there are, they just differ from the RedHat(6) style file the RPM would install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed "unit files" to "Service Daemon files"
@@ -28,15 +30,6 @@ def main(): | |||
copyfile('scripts/apel-ssm.logrotate', 'conf/apel-ssm') | |||
copyfile('README.md', 'apel-ssm') | |||
|
|||
if not path.exists('/var/log/apel'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this has been replaced by the bit below in data_files
? Is that the better way to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so, it lets setuptools
worry about the directory creation rather than doing it 'manually'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
scripts=['bin/ssmreceive', 'bin/ssmsend'], | ||
data_files=[(conf_dir, conf_files), | ||
('/etc/logrotate.d', ['conf/apel-ssm']), | ||
('/etc/init.d', ['bin/apel-ssm']), | ||
('/usr/share/doc', ['apel-ssm'])], | ||
('/usr/share/doc/apel-ssm', ['apel-ssm']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where'd the docs go before this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it went in a file under /usr/share/doc
rather than a file under /usr/share/doc/apel-ssm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that sounds better now.
-n apel-ssm \ | ||
-v $VERSION \ | ||
--iteration $ITERATION \ | ||
-m "Apel Administrators <apel-admins@stfc.ac.uk>" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of config here that seems like it would be better handled with a config file or as constants earlier in the script. Email is repeated.
Can the depends
be given as a list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably would be better as a config file, or at the very least reducing the duplication of the email.
I don't think so, the documentation around depends
says A dependency. This flag can be specified multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is there an option to read a list of dependencies or anything like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. Ideally, dependencies would be read from the setup.py
script but as some of the python packages don't have a system packages I didn't use that feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is best to do this as a config file? Sourcing a separate file that list the variables? Like this:
TAG=...
SOURCE_DIR=...
BUILD_DIR=...
# etc etc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As there's no built in way, I'd leave it as is.
The Codacy suggestion could also be fixed: https://app.codacy.com/app/apel/ssm/pullRequest?prid=1888023 |
scripts/ssm-build-deb.sh
Outdated
@@ -61,21 +62,21 @@ fpm -s python -t deb \ | |||
--python-install-bin /usr/bin \ | |||
--python-install-lib $PYTHON_INSTALL_LIB \ | |||
--exclude *.pyc \ | |||
--package $BUILD_DIR \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot neater. 👍
Think that's everything covered. Shall we rebase? |
- ubuntu (and RedHat) seem to prefer this to file directly under /usr/share/doc/apel-ssm - and fpm errors because it seems to assume apel-ssm is a directory (see jordansissel/fpm#586)
- it is very RedHat/Centos/SL specific and whereas the setup.py script is meant to be a more general way of installing ssm - it also seems to cause problems with fpm and the packages generated by pleaserun as the bad init script conflicts with the unit files - add a note saying this script does not install init scripts
- so packages created by fpm will also create them
- so fpm generated package is appropriately named
- as it mangle them and those it doesn't mangle aren't packages under ubunutu so need to be installed by pip
- as pip is needed to install the python requirements and the system python-ldap package is needed for the python python-ldap to install
- to prevent a nasty surprise on systems where python 3 is the default python
- in its previous location, it seemed to have no effect. - it has been moved to before any "depends" options
- following review
- rather than moving it
- as suggested by Codacy
- as it is not limited to amd64 style Ubuntu systems - `all` is the deb equivalent of `noarch`
Resolves #60
This PR:
test
package of SSM (i.e. the unittests)Manual Testing
Running this stripped down version of the build script, that works within a checked out copy of the code base i.e. it doesn't go any fetch a version from GitHub.