https://bugzilla.redhat.com/show_bug.cgi?id=1302876
Bug ID: 1302876 Summary: Review Request: clatd - CLAT / SIIT-DC Edge Relay implementation for Linux Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: ingvar@linpro.no QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://ingvar.fedorapeople.org/tayga/clatd.spec SRPM URL: https://ingvar.fedorapeople.org/tayga/clatd-1.4-1.2.20160128git1abcec1.fc22....
Description: clatd implements the CLAT component of the 464XLAT network architecture specified in RFC 6877. It allows an IPv6-only host to have IPv4 connectivity that is translated to IPv6 before being routed to an upstream PLAT (which is typically a Stateful NAT64 operated by the ISP) and there translated back to IPv4 before being routed to the IPv4 internet.
Fedora Account System Username: ingvar
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #1 from Ingvar Hagelund ingvar@linpro.no --- Upstream source and inforamtion: https://github.com/toreanderson/clatd
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
Petr Pisar ppisar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ppisar@redhat.com
--- Comment #2 from Petr Pisar ppisar@redhat.com --- Is there a reason for unconditional run-time dependency on initscripts? Should it be needed only on distributions without systemd?
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #3 from Ingvar Hagelund ingvar@linpro.no --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
Petr Pisar ppisar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |lnykryn@redhat.com Flags| |needinfo?(lnykryn@redhat.co | |m)
--- Comment #4 from Petr Pisar ppisar@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
Lukáš Nykrýn lnykryn@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(lnykryn@redhat.co | |m) |
--- Comment #5 from Lukáš Nykrýn lnykryn@redhat.com --- 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?
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #6 from Ingvar Hagelund ingvar@linpro.no --- 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....
Ingvar
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
Lubomir Rintel lkundrak@v3.sk changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |lkundrak@v3.sk Assignee|nobody@fedoraproject.org |lkundrak@v3.sk Flags| |fedora-review?
--- Comment #7 from Lubomir Rintel lkundrak@v3.sk --- Taking this for a review.
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #8 from Lubomir Rintel lkundrak@v3.sk --- * 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).
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #9 from Petr Pisar ppisar@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #10 from Ingvar Hagelund ingvar@linpro.no ---
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
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #11 from Ingvar Hagelund ingvar@linpro.no --- I said BSD. I meant MIT.
Ingvar
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
Lubomir Rintel lkundrak@v3.sk changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #12 from Lubomir Rintel lkundrak@v3.sk --- Looks reasonable.
APPROVED
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #13 from Jon Ciesla limburgher@gmail.com --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/clatd
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #14 from Fedora Update System updates@fedoraproject.org --- clatd-1.4-2.el6 has been submitted as an update to Fedora EPEL 6. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-13f43f568d
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #15 from Fedora Update System updates@fedoraproject.org --- clatd-1.4-2.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-64de0b334e
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #16 from Fedora Update System updates@fedoraproject.org --- clatd-1.4-2.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-539259c3b2
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #17 from Fedora Update System updates@fedoraproject.org --- clatd-1.4-2.el5 has been submitted as an update to Fedora EPEL 5. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-80d9bf670b
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #18 from Fedora Update System updates@fedoraproject.org --- clatd-1.4-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-6a8f0b2dcf
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #19 from Fedora Update System updates@fedoraproject.org --- clatd-1.4-2.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-01a13b182f
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #20 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #21 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #22 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #23 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #24 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #25 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #26 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA Last Closed| |2016-09-05 13:53:43
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #27 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #28 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #29 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1302876
--- Comment #30 from Fedora Update System updates@fedoraproject.org --- 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.
package-review@lists.fedoraproject.org