Closed dantefromhell closed 2 years ago
See my comment from here: https://github.com/purpleidea/mgmt/pull/679/#issuecomment-956569089
/cc @jordansissel the fpm genius in case you know what the best practice is here.
I am summoned! <3
I know you asked about fpm stuff, and I'll answer that. First, I'll help with the two open questions above. The fpm response is at the bottom of this comment.
A] Does this change work for all distributions mgmt packages get build for?
Probably! Most folks seem to just put systemd unit files in /usr/lib/systemd/system
, but I put mine in /etc/systemd/system
. The difference, per the systemd.unit(5)
docs, depends on your perspective I suppose:
path | purpose |
---|---|
/etc/systemd/system | System units created by the administrator |
/usr/lib/systemd/system | System units installed by the distribution package manager |
Are you the "distribution" or the "administrator"? There's no room for a 3rd party vendor like this project serving up its own packages in this world 🤦
Which path is correct? For the technology, I don't think it matters since systemd knows to check a bunch of paths (per systemd.unit(5)
). Humans might have opinions, but to compare, let's look at both options comparing Logstash and Elasticsearch systemd service file locations:
/etc/systemd/system/
: Logstash uses pleaserun to install a sysv/upstart/systemd script using a script run at package installation time. If systemd is detected, Logstash's package will write to /etc/systemd/system and this works well. Writing the service file to /usr/lib/systemd/system is also fine/usr/lib/systemd/system
: Elasticsearch writes its elasticsearch.service
file to /usr/lib/systemd/system
.Summary: The path you've chosen (/usr/lib/systemd/system/...) looks fine and should work. It might be worth, later, writing a test suite to make sure systemd stuff works as expected. I wouldn't worry about writing a test for now.
B] Is the systemd unit file path for packages consistent across linux distros?
In my experience, systemd is pretty consistent across Linux distros and releases. If you are concerned, testing with an older distro (Ubuntu 16.04 I think? Also CentOS 7) will have an older systemd available you can test with. Older Ubuntus use upstart
and not systemd
, as does CentOS 6, so I wouldn't try those distros for testing systemd stuff.
Reviewing the mgmt.service, it looks very simple (nice!) so I have some confidence that this service file will work on any distro that has systemd of nearly any version. I wrote pleaserun
to target multiple platforms, and I've never found the need to have more than one version of systemd handled -- Here's what pleaserun generates, for comparison: https://gist.github.com/jordansissel/81c208f945812f004020e70f0818e8e2
I think your systemd service looks good. 👍
Per https://github.com/purpleidea/mgmt/pull/679#issuecomment-956569089,
I'm missing more context about what's going on here and why you removed the --prefix flag
Ok so there's two behaviors provided by --prefix
:
1) It puts the prefix path before all paths in the package
2) For rpm
packages, it makes them Relocatable. In my experience, relocatable packages is a pretty uncommon thing. I don't think we need to worry, but you have more context on package usage for this project than I, so let that be your guide ;)
Part of this change (removing --prefix
and putting the prefix in the file path directly) has likely no negative impact and would have the same outcomes:
That is, in fpm, these are roughly the same result:
# With --prefix
% fpm -s dir -t rpm -n example --prefix /my/fancy/prefix README.rst
Created package {:path=>"example-1.0-1.x86_64.rpm"}
% rpm -ql example-1.0-1.x86_64.rpm
/my/fancy/prefix/README.rst
# Without --prefix, but still installing to the same place in the package:
% fpm -s dir -t rpm -n example README.rst=/my/fancy/prefix/README.rst
Created package {:path=>"example-1.0-1.x86_64.rpm"}
% rpm -ql example-1.0-1.x86_64.rpm
/my/fancy/prefix/README.rst
In order for this PR to work correctly, you must not use --prefix /usr/bin
otherwise you wouldn't be able to install files in /usr/lib/...
. That is, if you use --prefix /usr/bin
, then you can't have any files in the package exist in /usr/lib/systemd
because fpm will prefix all paths with /usr/bin
.
Technically, you could change PREFIX to PREFIX=/usr
and then install things based on /usr
but I don't think that benefits you much.
This PR replaces --prefix
with the fpm dir
package type's feature "path mapping" which lets you map a local file to a destination file when creating the package using this syntax: localpath=destinationpath
+ "$BINARY"="$PREFIX/mgmt" \
+ "misc/mgmt.service"="/usr/lib/systemd/system/mgmt.service"
This will put $BINARY (local file) in the path $PREFIX/mgmt in the package. It will also put the local file misc/mgmt.service
in the package as /usr/lib/systemd/system/mgmt.service
So best I can tell, this change looks good and the usage of fpm is correct.
If you want to test, have this script build a debian package and then list the contents with dpkg -c mgmt.deb
and see if the files are in the expected places. Similar for rpms, use rpm -qlp mgmt.rpm
. Neither of these commands require installing the package to list the contents.
@jordansissel Thank you for your excellent review! This part was the biggest aha for me:
This PR replaces --prefix with the fpm dir package type's feature "path mapping" which lets you map a local file to a destination file when creating the package using this syntax: localpath=destinationpath
Thank you again!
@dantefromhell So patch LGTM, except you'll need to change the commit message so it passes the commit message test. Also if you add the above quoted line to the commit message body and sourcing Jordan that would be most excellent.
Lastly, a nit: might be best to have: "$BINARY"="$PREFIX/mgmt"
be the last line (so re-order please)
Thanks all!
Ok I tried to test this and it seems alright. I had to work around some of the tooling probably because I'm using it for the wrong purpose ;)
% git branch --show-current
pull/680
# Fake a tag so the package tooling will work
% git tag 0.0.680
% mkdir -p releases/0.0.680/centos-7/
% ./misc/make-rpm-changelog.sh centos-7 0.0.680
% bash misc/fpm-pack.sh centos-7 0.0.680 mgmt-centos-7-0.0.680-1.x86_64.rpm libvirt-devel augeas-devel
Created package {:path=>"releases/0.0.680/centos-7/mgmt-centos-7-0.0.680-1.x86_64.rpm"}
# Check the file listing
% rpm -qlp releases/0.0.680/centos-7/mgmt-centos-7-0.0.680-1.x86_64.rpm
/usr/bin/mgmt
/usr/lib/.build-id
/usr/lib/.build-id/36
/usr/lib/.build-id/36/4f07c2620592af6a3e4393a47bb1adcc0babdb
/usr/lib/systemd/system/mgmt.service
The two files /usr/bin/mgmt
and /usr/lib/systemd/system/mgmt.service
look correct.
Taking a look at the file contents:
What is /usr/bin/mgmt? Let's ask.
% rpm2cpio releases/0.0.680/centos-7/mgmt-centos-7-0.0.680-1.x86_64.rpm | cpio -i --to-stdout ./usr/bin/mgmt | file -
123269 blocks
/dev/stdin: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=364f07c2620592af6a3e4393a47bb1adcc0babdb, for GNU/Linux 3.2.0, not stripped
Show the contents of the mgmt.service
% rpm2cpio releases/0.0.680/centos-7/mgmt-centos-7-0.0.680-1.x86_64.rpm | cpio -i --to-stdout ./usr/lib/systemd/system/m
gmt.service
[Unit]
Description=Run mgmt configuration management
Documentation=https://github.com/purpleidea/mgmt/
After=systemd-networkd.service
Requires=systemd-networkd.service
[Service]
ExecStart=/usr/bin/mgmt run empty $OPTS
RestartSec=5s
Restart=always
[Install]
WantedBy=multi-user.target
@jordansissel Big thanks for the in-depth explanation. Its really helped me with the understanding!
@purpleidea All comments/ nit Done. Tests should pass, but since this is my first PR to this repo the workflow is not triggered automatically.
LGTM, just waiting for tests.
Merged, thanks! LMK if you want to get involved more. Cheers!
According to [1] "/usr/lib/systemd/system/" is where systemd files from installed packages are stored in Arch Linux
According to FPM docs, the '--prefix' flag configures the "path to prefix files with when building the target package" [2].
With this change the contains more than a single binary, replacing the prefix flag with an absolute path constructed from the same shell variable should be safe and reduce the risk of the prefix flag interferring with other files added to the package.
Open question: A] Does this change work for all distributions mgmt packages get build for? B] Is the systemd unit file path for packages consistent across linux distros?
[1] https://wiki.archlinux.org/title/Systemd#Writing_unit_files [2] https://fpm.readthedocs.io/en/latest/cli-reference.html