[Bug 1302876] Review Request: clatd - CLAT / SIIT-DC Edge Relay implementation for Linux

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1302876



--- Comment #8 from Lubomir Rintel <lkundrak@xxxxx> ---
* 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).

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]