[Bug 1263941] Review Request: tayga - Simple out-of-kernel stateless NAT64 daemon
https://bugzilla.redhat.com/show_bug.cgi?id=1263941
Lubomir Rintel <lkundrak(a)v3.sk> changed:
What |Removed |Added
----------------------------------------------------------------------------
Flags|fedora-review? |fedora-review+
--- Comment #17 from Lubomir Rintel <lkundrak(a)v3.sk> ---
(In reply to Ingvar Hagelund from comment #14)
Hello, Lubomir. Thanks for taking the review :-)
Updated src.rpm:
https://ingvar.fedorapeople.org/tayga/tayga-0.9.2-3.fc23.src.rpm
Updated specfile: https://ingvar.fedorapeople.org/tayga/tayga.spec
(In reply to Lubomir Rintel from comment #12)
> 0.) License tag not correct
>
> > License: GPLv2
>
> It seems to be "GPLv2+" (there's an or later version clause).
Fixed. I have also asked upstream to add this to README or some other more
visible file. Atm, it's only visible in the manpage.
Well, it's visible in the comments in the source code, which is what actually
matters.
> 1.) Please add comments that would describe the upstreaming
status of the
> patches
>
> > Patch0: tayga-0.9.2_redhat_initscripts_and_systemd.patch
> > Patch1: tayga-0.9.2_cflags_override.patch
Done
> Have you send them upstream? Is there a mailing list or bug tracker
> reference?
Yes, they are sent upstream. I have not found any issue tracker nor mailing
list for tayga. I have asked upstream for this as well, but not received any
reply.
Thanks.
> 2.) Why do you turn off PIE on ppc64?
>
> > # PIE hardening seems to fail on ppc64
>
> If this is really the case, please add a more descriptive comment (error
> output).
After a bit of debugging, I found this not to be a problem with PIE, but a
difference on Red Hat's ppc64 and x86_64 koji builders. The ppc64 one does
not take "-z now" in LDFLAGS, though the x86_64 one does. Changing to
"-znow" seems to work, so I have removed the workaround.
Good job on this one, thanks.
> 3.) You're missing the %defattr tag
>
> It's not needed for current RPM, but seems like you're targetting old RHEL
> versions as well?
Fixed.
> 4.) You're installing a service with name of a templated
service:
>
> > ln -s %{_unitdir}/%{name}@.service
%{buildroot}%{_unitdir}/%{name}(a)example.service
>
> This makes no sense. systemd would probably just ignore that.
No, it treats it as a service.
> 'systemctl enable tayga@example' would make the link in the proper place;
> just drop that line.
Okay, dropped, but it would not be trivial for the non-seasoned systemd user
to know how to enable the service.
Yes, but how does this help a non-seasoned user?
Ideally upstream would should distribute the service files along with the
documentation on how to use them.
Should I add a README.RedHat with a note
explaining this?
Yes, I think that's okay.
Also, Will the %systemd_preun %{name}@.service remove both the unit
file and
any generated symlinks?
Of course not. The packages shouldn't touch /etc. If the user installs file in
/etc, he's responsible for dealing with it. That's the same regardless of
whether the service is templated or not.
> 5.) rpmlint complains:
>
> > tayga.x86_64: E: incorrect-fsf-address /usr/share/licenses/tayga/COPYING
> > The Free Software Foundation address in this file seems to be outdated or
> > misspelled. Ask upstream to update the address, or if this is a license file,
> > possibly the entire file with a new copy available from the FSF.
>
> This is something the upstream would need to fix; you may want to let them
> know. Obviously not a review blocker.
I did notice upstream about this as well.
Thank you.
The package looks good to me now.
APPROVED
--
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