Bug 1302876

Upstream source and inforamtion: https://github.com/toreanderson/clatd

Is there a reason for unconditional run-time dependency on initscripts? Should it be needed only on distributions without systemd?

Hello, Petr

I require initscripts for /etc/NetworkManager/dispatcher.d (in %install).

On my fedora 23 system:

$ rpm -qf /etc/NetworkManager/dispatcher.d
initscripts-9.65-1.fc23.x86_64
dhcp-client-4.3.3-8.P1.fc23.x86_64
NetworkManager-1.0.10-2.fc23.x86_64
puppet-4.2.1-2.fc23.noarch

The initscripts package seems the least intrusive package to require. Alternatively, I could add yet another ownership of that directory. Is there a policy for this?

Ingvar

There is <https://fedoraproject.org/wiki/Packaging:Guidelines#FileAndDirectoryOwnership>. If the 50-clatd file is for NetworkManager, the you should run-require NetworkManager instead of initscripts. If clatd does not need NetworkManager and it's a just an optional feature, then I would either own the directory by clatd or move the file into a subpackage and let the subpackage run-require NetworkManager.

However, I'm not expert on the NetworkManager. What puzzles me is that /etc/NetworkManager directory is not owned by NetworkManager. It's owned by initscripts. Maybe it's a bug in initscripts, or I just don't understand the purpose of the directory.

My impression about initscripts is that the package is deprecated, therefore I thought it's was a mistake to introduce a new dependency on it.

I'm CCing lnykryn who should know more about initscripts.

That directory is owned by NM as well
$ rpm -qf /etc/NetworkManager/dispatcher.d/
initscripts-9.62-1.fc22.x86_64
dhcp-client-4.3.2-2.fc22.x86_64
NetworkManager-1.0.2-1.fc22.x86_64

And initscripts co-own the directory because initscripts ships some drop-in for NM, but it does not need NM.

So maybe you want to do the same thing in your new package?

Thanks Lukáš and Petr. clatd now co-owns /etc/NetworkManager/dispatcher.d, but no longer requires initscripts.

https://ingvar.fedorapeople.org/tayga/clatd.spec
https://ingvar.fedorapeople.org/tayga/clatd-1.4-1.3.20160128git1abcec1.fc23.src.rpm

Ingvar

Taking this for a review.

* Named correctly
* Licensed correctly
* License text included
* SPEC file clean and legible
* Builds fine in mock

Looks generally good. There's some notes below -- some of them are just suggestions; the only thing that really needs a fix is the %config tag:

> Release:      1.3.20160128git%{?shortcommit0}%{?dist}

The "1.3" in release looks weird; you typically use only one digit unless the
first one is a zero. E.g. "0.3.<snapshot>" for a pre-release snapshot and
"3.<snapshot>" for a post-release snapshot.

Also, why are you packaging a snapshot instead of a released version?

> %if 0%{?fedora} > 23
> BuildRequires:        perl-podlators
> %endif

You can get rid of the conditional if you do a BuildRequires: /usr/bin/pod2man

> Requires:     perl(Net::IP)
> Requires:     perl(Net::DNS)
> Requires:     perl(IO::Socket::INET6)
> Requires:     perl(File::Temp)

Hmm, these should be autogenerated; but only Net::IP is. Seems like the 
dependency generator ignores requires if they don't start in column zero...

> %build
> pod2man       --name %{name} \
>       --center "clatd - a CLAT implementation for Linux" \
>       --section 8 \
>       README.pod %{name}.8
> gzip %{name}.8
> echo '# Default clatd.conf
> # See clatd(8) for a list of config directives' > %{name}.conf
> 
> sed -i "s,%{_sbindir}/clatd,%{_sbindir}/clatd -c %{_sysconfdir}/%{name}.conf," \
>       scripts/*
> 
> 
> %install
> install -p -D -m0755 %{name} %{buildroot}%{_sbindir}/%{name}
> install -p -D -m0644 %{name}.8.gz %{buildroot}%{_mandir}/man8/%{name}.8.gz
> install -p -D -m0644 %{name}.conf %{buildroot}%{_sysconfdir}/%{name}.conf
> install -p -D -m0755 scripts/%{name}.networkmanager %{buildroot}%{_sysconfdir}/NetworkManager/dispatcher.d/50-%{name}
> %if 0%{?fedora} >= 18 || 0%{?rhel} >= 7
> install -p -D -m 0644 scripts/%{name}.systemd %{buildroot}%{_unitdir}/%{name}.service
> %else
> install -p -D -m0644 scripts/%{name}.upstart %{buildroot}%{_sysconfdir}/init/%{name}.conf;
> %endif

This all should ideally be in the upstream Makefile. Perhaps you could ask upstream?

> %{_sysconfdir}/%{name}.conf

This one is a configuration file; please mark it woth %config(noreplace).

(In reply to Lubomir Rintel from comment #8)
> Also, why are you packaging a snapshot instead of a released version?
> 
> > %if 0%{?fedora} > 23
> > BuildRequires:        perl-podlators
> > %endif
> 
> You can get rid of the conditional if you do a BuildRequires:
> /usr/bin/pod2man
>
Better "%{_bindir}/pod2man". If Fedora decides to move the prefix again or if somebody wants to port the spec file to SCL.

But the condition is wrong. E.g. F23 has the program in perl-podlators package. Actually, the first Fedora with perl-podlators is F19. Also RHEL-7 has the package.
 
> > Requires:     perl(Net::IP)
> > Requires:     perl(Net::DNS)
> > Requires:     perl(IO::Socket::INET6)
> > Requires:     perl(File::Temp)
> 
> Hmm, these should be autogenerated; but only Net::IP is. Seems like the 
> dependency generator ignores requires if they don't start in column zero...
> 
Collective wisdom says indented "require" statements generates too many false positives. Therefore generators omit them. But I'm not fully convinced about helpfulness of the omission.

> > Release:      1.3.20160128git%{?shortcommit0}%{?dist}
> 
> The "1.3" in release looks weird; you typically use only one digit unless the
> first one is a zero. E.g. "0.3.<snapshot>" for a pre-release snapshot and
> "3.<snapshot>" for a post-release snapshot.

Yeah, it should perhaps rather be version 1.5 and release 0.3.<snapshot>, but se below.
 
> Also, why are you packaging a snapshot instead of a released version?

While there are few or no actual changes in the code from the 1.4 release, the license was changed from some unclear beerware like license to the actual BSD license after the release. This goes for the license text inside the clatd source, and was clearly just forgotten by upstream, as the LICENSE file had already been updated to BSD before the release. It felt wrong to change the license through a patch, though even when fetched from upstream. I did not want to package clatd with two different licenses.

In the spec linked below, I wrap the 1.4 release, and change the license through a patch, noting that the author acknowledges this. I also added the documentation fixes. (This is in effect what happens by using the snapshot.)

> > %if 0%{?fedora} > 23
> > BuildRequires:        perl-podlators
> > %endif
> 
> You can get rid of the conditional if you do a BuildRequires:
> /usr/bin/pod2man

Changed to BuildRequires: %{_bindir}/pod2man as Petr suggested.

> > Requires:     perl(Net::IP)
> > Requires:     perl(Net::DNS)
> > Requires:     perl(IO::Socket::INET6)
> > Requires:     perl(File::Temp)
> 
> Hmm, these should be autogenerated; but only Net::IP is. Seems like the 
> dependency generator ignores requires if they don't start in column zero...

* Petr Pisar
> Collective wisdom says indented "require" statements generates too many false 
> positives. Therefore generators omit them. But I'm not fully convinced about 
> helpfulness of the omission.

I'll just remove perl(Net::IP), and keep the rest for now.
 
> > %build
> > (...)

> This all should ideally be in the upstream Makefile. Perhaps you could ask
> upstream?

Yep, I'll ask upstream for this.
 
> > %{_sysconfdir}/%{name}.conf
> 
> This one is a configuration file; please mark it with %config(noreplace).

Fixed

Updated spec and srpm:
https://ingvar.fedorapeople.org/tayga/clatd.spec
https://ingvar.fedorapeople.org/tayga/clatd-1.4-2.fc23.src.rpm

I said BSD. I meant MIT.

Ingvar

Looks reasonable.

APPROVED

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/clatd

clatd-1.4-2.el6 has been submitted as an update to Fedora EPEL 6. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-13f43f568d

clatd-1.4-2.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-64de0b334e

clatd-1.4-2.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-539259c3b2

clatd-1.4-2.el5 has been submitted as an update to Fedora EPEL 5. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-80d9bf670b

clatd-1.4-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-6a8f0b2dcf

clatd-1.4-2.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-01a13b182f

clatd-1.4-2.el5 has been pushed to the Fedora EPEL 5 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-80d9bf670b

clatd-1.4-2.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-64de0b334e

clatd-1.4-2.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-6a8f0b2dcf

clatd-1.4-2.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-01a13b182f

clatd-1.4-2.el6 has been pushed to the Fedora EPEL 6 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-13f43f568d

clatd-1.4-2.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-539259c3b2

clatd-1.4-2.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

clatd-1.4-2.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

clatd-1.4-2.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

clatd-1.4-2.el5 has been pushed to the Fedora EPEL 5 stable repository. If problems still persist, please make note of it in this bug report.

clatd-1.4-2.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.