nmutils icon indicating copy to clipboard operation
nmutils copied to clipboard

Add spec file

Open SpareSimian opened this issue 3 years ago • 50 comments

Needs an RPM spec file to maintain file ownership on RPM-based distros. Here's a rudimentary one that I can build on Rocky 8. nmutils.spec.txt

SpareSimian avatar May 17 '22 21:05 SpareSimian

(Since the project lacks a version number, I gave it an arbitrary one of 0.1. It would be useful to give it an official one and tag the project to make packaging a little easier.)

SpareSimian avatar May 17 '22 21:05 SpareSimian

I forgot to make the dispatcher scripts executable in first release. This fixes that. nmutils.spec.txt

SpareSimian avatar May 17 '22 21:05 SpareSimian

Very nice spec file... I was wondering if when installing with RPM it might be more friendly to:

  • place the dispatchers in /usr/lib (so they can be overridden in /etc)
  • leave the non-conf files in /etc/nmutils as regular config so that they are replaced on upgrade (but edits are kept as .rpmsave)
  • adapt the files to actually use %{_sysconfdir} and %{prefix}/lib as distributed defaults

I guess if since the spec file would call for versioning, I should probably create a release to go with it... Here's a suggested spec file that includes these changes... see if that looks ok to you. nmutils.spec.txt

sshambar avatar May 18 '22 07:05 sshambar

Consider using libexec for the function libraries in /etc/nmutils. As you say, leave the conf files in /etc/nmutils. https://unix.stackexchange.com/questions/312146/what-is-the-purpose-of-usr-libexec

I wasn't aware of /usr/lib/NetworkManager/dispatcher.d. Is that like the systemd/system directory for units? Where a file in /usr/lib is ignored if one of the same name is found in /etc? If so, that would be the right place to put the stock dispatcher scripts. I do prefer mechanisms that move customizations to separate files that don't get overwritten on updates or need customization edits migrated and it sounds like this accomplishes that.

There are rpm macros for /usr/lib and /usr/libexec: https://docs.fedoraproject.org/en-US/packaging-guidelines/RPMMacros/

SpareSimian avatar May 18 '22 15:05 SpareSimian

FHS lists libexec as for "internal binaries that are not intended to be executed directly by users or shell scripts." Since the current /etc/nmutils files are just sourced (not executed, ie no #!/bin/bash at the top, or +x perms), they actually might be a better candidate for /usr/share (as they are platform independent, non-executable).

The dispatcher scripts appear to follow the systemd override logic between the same names in etc/usr - the NetworkManager-dispatcher man page isn't clear on the subject, but it works in my tests :).

I checked the rpm macros but couldn't find anything apart from %prefix for the /usr/lib/NetworkManager... as %_libdir is /usr/lib64 on x86_64.

sshambar avatar May 21 '22 05:05 sshambar

Agreed that /usr/share makes more sense, similar to where web-based packages put their scripts.

Good point about lib64. That's a NM packaging issue. You can see that systemd addressed it with the _unitdir macro. I wonder if anyone has suggested something like an _nm_scriptdir RPM macro?

SpareSimian avatar May 21 '22 16:05 SpareSimian

My thoughts on /usr/share were to install the "official" script libraries there, and then symlink them to /etc/nmutils where they could (optionally) be replaced/edited by the admin... upgrades would need to handle edits (.rpmsave?) so that new script versions can at least depend on version compatibility. Alternatively, script-library edits in /etc/nmutils could be ignored with the assumption that the admin at least makes an effort to source the /usr/share version :)

sshambar avatar May 21 '22 17:05 sshambar

I started a thread here on the macros and found the macros used in NetworkManager's own spec file. Perhaps those should be hoisted into the spec file here? https://forums.rockylinux.org/t/rpm-for-networkmanager-utilities-for-gateway-configurations/6132

SpareSimian avatar May 23 '22 21:05 SpareSimian

I did use %{_prefix} is the previous .spec file uploaded above... not sure it's worth defining a %global for it as NM does as it doesn't get used much :)

Here's the latest version of the spec file with modifications to put libraries in /usr/share, and symlink them to /etc/nmutils on install/upgrade (saving any edits in .rpmsave). I also cleaned up the systemd ddns-onboot@ handling. nmutils.spec.txt - I ran a few upgrade tests and things appear to work as expected (keep in mind %preun runs from the old-package spec when testing!)

sshambar avatar May 24 '22 01:05 sshambar

I rewrote the spec file to actually patch the install/config locations so that the symlink hack isn't needed. Installed scripts (/usr/share, /usr/lib/...) will default to using their own library versions, but still use the global /etc/nmutils/conf files. Edits/overrides can be placed in /etc/NetworkManager, and they may of course be modified to pull functions from edited libraries in /etc/nmutils (if desired).

I also added the SELinux module with rules to allow NM scripts to spawn dhclient, and dhclient scripts to manage radvd and nsupdate/dig. The spec file also builds nmutils-selinux as a subpackage so it can be conditionally installed if needed.

The rpm build now also runs the test suite (unless rpmbuild --nocheck is used) on the buildroot scripts/libraries to make sure things appear to be installed corrrectly.

Give it a try, and let me know if you have any changes... if it looks good, I'll create a release :)

sshambar avatar Jun 03 '22 20:06 sshambar

Hold on checking... I think there's a bug in the sed script I need to fix first.

sshambar avatar Jun 03 '22 21:06 sshambar

OK, I've updated the scripts to not set NMCONF (general-functions defaults it), and fixed the "sed" in the spec to correctly edit the files with the new paths. Please give it a try!

sshambar avatar Jun 04 '22 00:06 sshambar

If I change the version to "master", this works for me:

wget -nd -c https://github.com/sshambar/nmutils/archive/master/nmutils-master.tar.gz
rpmbuild -ba --nocheck ~buildmeister/rpmbuild/BUILD/nmutils/nmutils.spec

If I leave tests enabled, it fails. There's no tag for version 20220603 so wget can't find that version on GitHub.

SpareSimian avatar Jun 04 '22 02:06 SpareSimian

Yeah, I haven't created a release... if you want to build a release, just tar up the source in a directory called nmutils-20220603 and you're good to go. If you'd like, I can just create a release and you can try pulling it directly, but I was going to wait until you'd had a chance to test.

sshambar avatar Jun 04 '22 03:06 sshambar

Your checks might have failed because you're using a "patched" spec file (in the BUILD directory)... try pulling the spec directly from the source (and edit the version), and let me know if you still have any problems. If it works for you, I'll publish a release!

sshambar avatar Jun 05 '22 20:06 sshambar

Changing the tarball name and version back didn't help. Here's my build log showing the test failures. Could I be missing a prerequisite that the tests depend on? https://pastebin.com/S6xbBG79

SpareSimian avatar Jun 06 '22 21:06 SpareSimian

Interesting... I think the "strict mode" errors might be related to the bash version in the path, could you add a "bash --version" to the test/Makefile (perhaps general: rule) and see which version is actually being used? I can then try to reproduce the error with that version...

sshambar avatar Jun 06 '22 23:06 sshambar

GNU bash, version 4.4.20(1)-release (x86_64-redhat-linux-gnu)

This seems to be the latest for RHEL/CentOS/Rocky 8.

SpareSimian avatar Jun 06 '22 23:06 SpareSimian

OK, I'll build a Rocky 8 VM and see if I can't get to the bottom of this :)

sshambar avatar Jun 06 '22 23:06 sshambar

OK, I had a bug in the testing's "strict function whitelisting" - but fixing that exposed a bug in bash4's variable expansion in "=~" comparisons... so I updated shtest_setup to work around it.

Also found a few edge-case problems with assoc array comparisons (bash4 ordering was not the same as bash3/5), so I've updated shtest_setup to strictly order the array indexes in comparisons. I also sped up the array_copy function...

With those fixes, I was able to run the rpmbuild on Rocky 8 with tests, so give it a try and see if you still have any problems :)

sshambar avatar Jun 09 '22 05:06 sshambar

I'm now able to run "rpmbuild -bb" to completion with no test errors! The RPMs (main and selinux) install fine. My next challenge will be to configure and test the interfaces, but I think this makes the spec file ready for a general audience.

SpareSimian avatar Jun 09 '22 23:06 SpareSimian

Okay, I'll join this party too. :-)

I've pulled down the latest version and tried it out. I've update the spec file to pull commits, rather than final versions (do you want me to do a PR for it)? The details are here https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/.

Anyway, built it on Fedora 36, it fails with F45f F46f F48 F48f. Any ideas what may be going on there, before I investigate it further.

frankcrawford avatar Jun 11 '22 10:06 frankcrawford

I'm hoping the test failures are just filename ordering issues - I've added LC_ALL=C to the wildcard testing (but with names like xx.match,yy.match I didn't think that was necessary). If you still get test errors, please paste the test failure here :)

I was actually planning on creating a release (so the specfile source can reference it directly), but I wanted to get enough working builds first.

I also added a systemd-rpm-macros build requires as that appears to be necessary on F36 when I mock build it. I didn't get the test failures under mock though, so I am curious what errors you were seeing.

sshambar avatar Jun 11 '22 19:06 sshambar

Really interesting, the test issues seem to only occur on NFS, not on local filesystems. On NFS I get the following issue:

 F45  OK   | nmg::foreach_filematch(<2 matches>) - returns true
 F45f FAIL | nmg::foreach_filematch(<2 matches>) - makes callbacks (reg 1)
diff: <expected> <found>
1,2c1
< ./results/xx.match xx
< ./results/yy.match yy
 ---
> 
 F46  OK   | nmg::foreach_filematch(<2 matches>) - returns true
 F46f FAIL | nmg::foreach_filematch(<2 matches>) - makes callbacks (reg 1)
diff: <expected> <found>
1,2c1
< ./results/xx.match xx arg
< ./results/yy.match yy arg
 ---
> 
 F47  OK   | nmg::foreach_filematch(<no-pat>) - returns true
 F47f OK   | nmg::foreach_filematch(<no-pat>) - makes no callbacks
 F48  FAIL | nmg::foreach_filematch(<2 matches, callback fails>) - returns fail code
    expected: 3
       found: 0
 F48f FAIL | nmg::foreach_filematch(<2 matches, callback fails>) - makes 1 callback (reg 1)
    expected: './results/xx.match xx fail'
       found: ''

but the issue does not occur on local (ext4) filesystem.

Of note the failed tests are tose that involve writing 2 empty files and comparing them.

frankcrawford avatar Jun 12 '22 11:06 frankcrawford

Okay, pulling in your latest changes doesn't seems to fix those errors on NFS, but a new one sometimes appears, ipv6-prefix-nm-test L2F fails, if the others seem to succeed, which happens about 1 in 5 runs, again it seems to be NFS related.

As this looks to be entirely related to NFS caching, and that shouldn't be the case for a normal installation, I'd say just ignore it. I can build on the server, so it doesn't stop me, and I can push it all to COPR anyway.

frankcrawford avatar Jun 12 '22 12:06 frankcrawford

FYI, I built the latest in github on COPR with no issues for Fedora Rawhide (F37). You can see it here https://copr.fedorainfracloud.org/coprs/frankcrawford/nmutils/

frankcrawford avatar Jun 12 '22 12:06 frankcrawford

Really interesting, the test issues seem to only occur on NFS, not on local filesystems. On NFS I get the following issue:

That really is fascinating! I'm curious if it's a quirk with your nfs server, or a bug in my wildcard handling that could affect others (for example) using a nfs mounted root filesystem...

I setup a nfs mount and tried on my fedora server, but couldn't reproduce it... could you edit the general-test file and add the following around line 1120 (just below the printf that creates the empty files):

echo "$TEST_OUT/xx.match"
/bin/ls "$TEST_OUT"
for arg in "$TEST_OUT/"*.match; do /bin/ls -l "$arg"; done

and then run ./general-test +F45*

sshambar avatar Jun 12 '22 21:06 sshambar

I've taken this to a new issue #9 as it is independent of the spec file creation.

frankcrawford avatar Jun 13 '22 02:06 frankcrawford

Back to spec file issues, I've run a build of the current spec file on COPR and while F34, F35, F36, rawhide, EPEL8 and EPEL9 work, EPEL7 fails due to rpmbuild being old, and giving the following error:

error: line 36: Dependency tokens must begin with alpha-numeric, '_' or '/': Requires: (selinux-policy >= %{_selinux_policy_version} if selinux-policy-targeted)

From memory systemd-rpm-macros does not exist in RHEL7/EPEL7, and you need to just add a BuildRequires: systemd in that case.

frankcrawford avatar Jun 13 '22 03:06 frankcrawford

OK, I've updated the spec to handle epel7 and also pre-4.13 rpmbuild boolean conditionals, and made selinux a build condition parameter.

In building for epel7, I discovered some old bash3 quoting behavior in bash 4.2 used there, and patched to 95-radvd-gen to handle it.

Let me know if you see any other build issues!

sshambar avatar Jun 14 '22 03:06 sshambar