Bug 1302876 - Review Request: clatd - CLAT / SIIT-DC Edge Relay implementation for Linux
Review Request: clatd - CLAT / SIIT-DC Edge Relay implementation for Linux
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-28 16:01 EST by Ingvar Hagelund
Modified: 2016-09-16 00:47 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-09-05 13:53:43 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Ingvar Hagelund 2016-01-28 16:01:03 EST
Spec URL: https://ingvar.fedorapeople.org/tayga/clatd.spec
SRPM URL: https://ingvar.fedorapeople.org/tayga/clatd-1.4-1.2.20160128git1abcec1.fc22.src.rpm

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
Comment 1 Ingvar Hagelund 2016-01-28 16:05:46 EST
Upstream source and inforamtion: https://github.com/toreanderson/clatd
Comment 2 Petr Pisar 2016-02-22 08:43:29 EST
Is there a reason for unconditional run-time dependency on initscripts? Should it be needed only on distributions without systemd?
Comment 3 Ingvar Hagelund 2016-02-23 08:47:05 EST
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
Comment 4 Petr Pisar 2016-02-23 10:00:55 EST
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.
Comment 5 Lukáš Nykrýn 2016-02-23 10:24:15 EST
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?
Comment 6 Ingvar Hagelund 2016-02-23 17:49:54 EST
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
Comment 7 Lubomir Rintel 2016-06-27 14:48:05 EDT
Taking this for a review.
Comment 8 Lubomir Rintel 2016-06-27 15:04:27 EDT
* 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).
Comment 9 Petr Pisar 2016-06-28 01:53:58 EDT
(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.
Comment 10 Ingvar Hagelund 2016-06-29 10:04:05 EDT
> > 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
Comment 11 Ingvar Hagelund 2016-06-29 10:06:38 EDT
I said BSD. I meant MIT.

Ingvar
Comment 12 Lubomir Rintel 2016-07-04 13:14:38 EDT
Looks reasonable.

APPROVED
Comment 13 Gwyn Ciesla 2016-08-29 11:21:18 EDT
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/clatd
Comment 14 Fedora Update System 2016-08-30 03:05:50 EDT
clatd-1.4-2.el6 has been submitted as an update to Fedora EPEL 6. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-13f43f568d
Comment 15 Fedora Update System 2016-08-30 03:06:00 EDT
clatd-1.4-2.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-64de0b334e
Comment 16 Fedora Update System 2016-08-30 03:06:06 EDT
clatd-1.4-2.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-539259c3b2
Comment 17 Fedora Update System 2016-08-30 03:06:13 EDT
clatd-1.4-2.el5 has been submitted as an update to Fedora EPEL 5. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-80d9bf670b
Comment 18 Fedora Update System 2016-08-30 03:06:19 EDT
clatd-1.4-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-6a8f0b2dcf
Comment 19 Fedora Update System 2016-08-30 03:06:25 EDT
clatd-1.4-2.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-01a13b182f
Comment 20 Fedora Update System 2016-08-30 22:45:57 EDT
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
Comment 21 Fedora Update System 2016-08-30 23:52:06 EDT
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
Comment 22 Fedora Update System 2016-08-31 08:57:18 EDT
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
Comment 23 Fedora Update System 2016-08-31 08:57:23 EDT
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
Comment 24 Fedora Update System 2016-08-31 09:48:20 EDT
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
Comment 25 Fedora Update System 2016-08-31 09:49:28 EDT
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
Comment 26 Fedora Update System 2016-09-05 13:53:40 EDT
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.
Comment 27 Fedora Update System 2016-09-09 12:53:52 EDT
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.
Comment 28 Fedora Update System 2016-09-15 17:48:07 EDT
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.
Comment 29 Fedora Update System 2016-09-15 23:15:46 EDT
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.
Comment 30 Fedora Update System 2016-09-16 00:47:13 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.