Bug 226568

Package Change Request
======================
Package Name: xmlto
Updated Fedora Owners: ovasik

Can it be possible to ask "fedora-cvs?" without formal review?

(In reply to comment #2)
> Can it be possible to ask "fedora-cvs?" without formal review?

This is possible for packages that are under "Merge Review", because they are already included in Fedora.

About the package:

- Why is xsltproc required via a path instead of using libxslt? If libxslt cannot be used, please explain with a comment in the spec.

- It is recommended not to use %makeinstall, is there a reason to use it? It builds fine here with:

make install DESTDIR=$RPM_BUILD_ROOT

- parallel make is not used:
make %{?_smp_mflags}

- Is the touch command still needed? If it is, please explain why in the spec file or fix it upstream:
touch doc/xmlto.xml doc/xmlif.xml

- License Tag should be GPLv2+ according to the header in xmlto.in
- License is in the tarball, but not in %doc:
COPYING

- There is more documentation missing:
README ChangeLog AUTHORS NEWS, maybe FAQ if it is planned to add more to this file in the future

- Btw. fedorahosted provides a way to distribute releases without using the scm, but I do not know how to upload something there:
https://fedorahosted.org/releases/

If you want and permissions are open in cvs, I can fix at least the issues that do not need a comment from you. I will then perform a full review later.

Please note comment: 3.

Thanks for check of spec file sanity...

(In reply to comment #3) 
> About the package:
> 
> - Why is xsltproc required via a path instead of using libxslt? If libxslt
> cannot be used, please explain with a comment in the spec.

Fixed, probably accidently introduced when using spec file from tarball ... up to F-9 libxslt requirements were present, fix now...

> - It is recommended not to use %makeinstall, is there a reason to use it? It
> builds fine here with:

No reason, fixed.

> - parallel make is not used:
> make %{?_smp_mflags}

Fixed

> - Is the touch command still needed? If it is, please explain why in the spec
> file or fix it upstream:
> touch doc/xmlto.xml doc/xmlif.xml

Not sure why they were touched - introduced between 0.18-6 and 0.18.13 (FC-5 and FC-6) without explanation in ChangeLog ... will remove it

> - License Tag should be GPLv2+ according to the header in xmlto.in

Fixed

> - License is in the tarball, but not in %doc:
> COPYING
> 
> - There is more documentation missing:
> README ChangeLog AUTHORS NEWS, maybe FAQ if it is planned to add more to this
> file in the future

No need for FAQ at the moment, will think about it for future releases.

> - Btw. fedorahosted provides a way to distribute releases without using the
> scm, but I do not know how to upload something there:
> https://fedorahosted.org/releases/

Ok, thanks for suggestion, I'll probably add sources under fedorahosted svn in future and fedorahosted.org/releases for tarballs - xmlto 0.0.21 is already there... https://fedorahosted.org/releases/x/m/xmlto/
 
> If you want and permissions are open in cvs, I can fix at least the issues that
> do not need a comment from you. I will then perform a full review later.

I guess just listing issues here should be enough, I'll fix it myself, thanks.

Objections so far mentioned in comment #3 fixed in rawhide build xmlto-0.0.21-3.fc11

(In reply to comment #5)
> > - Is the touch command still needed? If it is, please explain why in the spec
> > file or fix it upstream:
> > touch doc/xmlto.xml doc/xmlif.xml
> 
> Not sure why they were touched - introduced between 0.18-6 and 0.18.13 (FC-5
> and FC-6) without explanation in ChangeLog ... will remove it

It was to make sure the man pages were rebuilt against docbook-style-xsl.  The upstream tarball shipped pre-built man pages.

rpmlint output:
xmlto.spec:61: E: files-attr-not-set
- The %doc has to be below the %defattr

xmlto.i686: E: explicit-lib-dependency libxslt

- OK

xmlto-tex.i686: W: no-documentation
- OK
4 packages and 1 specfiles checked; 2 errors, 1 warnings.

License is still not ok:

xmlto.in contains a GPLv2+ header, butxmlif.l and xmlif.c do not contain any license information
Are they really GPLv2+, too? They seem to be individual programs to me. Can you get the original author to verify the license and add license headers then?
Here is a howto for GPL licensing:
http://www.gnu.org/licenses/gpl-howto.html

On the page of xmlif http://www.catb.org/~esr/xmlif/ is mentioned, that the xmlif is shipped as part of Tim Waugh's xmlto as well - and in the tarball is GPLv2 COPYING file. So I guess GPLv2+ should be ok... 
%doc and attrs will be fixed - of course - just accident ;)

If you want to rely only on the COPYING file, then xmlif is licensed under the terms of any GPL license, see:

https://fedoraproject.org/wiki/Licensing/FAQ

Or read the COPYING file:

| If the Program does not specify a version number of this License, you may 
| choose any version ever published by the Free Software Foundation.

This may not be intended by upstream, therefore it is better to ask.

The correct license tag for xmlif is then GPL+.

So do you suggest to have License: GPLv2+ and GPL+ ? I'm ok with it, although I guess it is quite obscure ... Tim - as you probably have direct contact to xmlif author - what do you think?

I haven't really had any contact with Eric since xmlif was included.  I'm sure it's fine for you to just add that RPM tag, and send him an email asking what he intended in the mean time.

(In reply to comment #10)
> So do you suggest to have License: GPLv2+ and GPL+ ?

Yes, please also add a comment above the License tag saying that xmlto is GPLv2+ and xmlif is GPL+.

Regards,
Till

Ok, I already commited and built License: GPLv2+ and GPL+ to rawhide, I do plan next xmlto release in January (hopefully) - will add the comment about licenses in next rawhide build later.

(In reply to comment #13)
> Ok, I already commited and built License: GPLv2+ and GPL+ to rawhide, I do plan
> next xmlto release in January (hopefully) - will add the comment about licenses
> in next rawhide build later.

It is ok to add the comment without creating a new rawhide in build. It will make it easier to not forget about it. :-)
Btw. did you write Eric an e-mail? If you did not, I can do it.

Email to Eric sent and license comment added to spec file. I received one patch during Christmas break, so it was ok to make rawhide build.

Email to Eric - he is ok with GPLv2+, so will change it in next xmlto release:

Ondřej Vašík <ovasik>:
> Hello,
> I'm Fedora(and since 0.0.19 upstream) maintainer of xmlto.
> During review of xmlto
> (https://bugzilla.redhat.com/show_bug.cgi?id=226568) Till Maas brought
> up question about xmlif license. It seems that xmlif has GPL+ license,
> but xmlto has GPLv2+ license. Therefore I have to use License: GPL+ and
> GPLv2+ as the current license for xmlto. Are you ok with having xmlif
> under GPLv2+ to consolidate it with xmlto? 
> 
> Thanks in advance for answer.
> 
> Greetings,
>          Ondrej Vasik

Yes, go ahead.
-- 
                <a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

ping... shouldn't we finish that review? I guess all objections are solved now...

Sorry for the delay.

[OK] rpmlint output:
xmlto.i386: E: explicit-lib-dependency libxslt
- libxslt contains the binary xsltproc
xmlto-tex.i386: W: no-documentation
xmlto-xhtml.i386: W: no-documentation

[OK] Spec in %{name}.spec format

[OK] license allowed: GPLv2+
[OK] license matches shortname in License: tag
[OK] license in tarball and included in %doc: COPYING
[OK] package is code or permissive content:
{OK} patches sent to upstream and commented
[OK] Source0 is a working URL
{N/A} Sourceforge URL is Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
<OK> SourceX / PatchY prefixed with %{name}
[OK] Source0 matches Upstream:
12f297dc7051e4fef08339980f88a1dd  xmlto-0.0.22.tar.bz2

[OK] Package builds on all primary architectures:
http://koji.fedoraproject.org/koji/buildinfo?buildID=95375
[N/A] ExcludeArch bugs are filed and commented:
[OK] BuildRequires are complete (mock builds)
(OK) No file dependencies outside of /etc /bin /sbin /usr/bin /usr/sbin 

[N/A] %find_lang used for locales

[N/A] Every (sub)package containing libraries runs ldconfig
%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig
[N/A] .h (header) files are in -devel subpackage
[N/A] .a (static libraries) are in -static subpackage
[N/A] contains .pc (pkgconfig) files and has Requires: pkgconfig
(N/A) .pc files are in -devel subpackage
[N/A] contains .so.X(.Y) files and .so is in -devel
[N/A] -devel subpackage has Requires: %{name} = %{version}-%{release}
[N/A] .la files (libtool) are not included

[N/A] Has GUI and includes %{name}.desktop
[N/A] Follows desktop entry spec
[N/A] Valid .desktop Name
[N/A] Valid .desktop GenericName
[N/A] Valid .desktop Categories
[N/A] Valid .desktop StartupNotify
[N/A] .desktop file installed with desktop-file-install in %install
[N/A] Prefix: /usr not used (not relocatable)

[OK] Owns all created directories
[OK] no duplicates in %files
[OK] %defattr(-,root,root,-) is in every %files section
[OK] Does not own files or dirs from other packages
[OK] included filenames are in UTF-8

[OK] %clean is rm -rf %{buildroot} or $RPM_BUILD_ROOT 
[OK] %install starts with rm -rf %{buildroot} or $RPM_BUILD_ROOT 
[OK] Consistent macro usage

[N/A] large documentation is -doc subpackage
[OK] %doc does not affect runtime

{OK} no pre-built binaries (.a, .so*, executable)

{OK} well known BuildRoot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

{OK} PreReq not used
{OK} RPM_OPT_FLAGS honoured
{OK} Useful debuginfo generated
{OK} no duplication of system libraries
{OK} no rpath
{NOT OK} Timestamps preserved with cp and install
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

You can fix this with:
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"

{OK} Uses parallel make (%{?_smp_mflags})
{OK} Requires(pre,post) style notation not used
{OK} only writes to tmp /var/tmp $TMPDIR %{_tmppath} %{_builddir} (and %{buildroot} on %install and %clean)
{OK} no Conflicts
{OK} nothing installed in /srv
{GOOD ENOUGH} Changelog in allowed format
https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

For the 0.0.22-1 released the version-release was not added to the changelog
entry.


<GOOD ENOUGH> Architecture independent packages have: BuildArch: noarch
The tex and xhtml subpackage can nowadays be noarch:
https://fedoraproject.org/wiki/Features/NoarchSubpackages
But this is not enforced, so you do not need to make them noarch.

<OK> Sane Provides: and Requires:
{OK} Follows Naming Guidelines

Please add 'INSTALL="install -p"' to 'make install' to preserve timestamps and consider to use noarch subpackages. The install issue is minor, therefore this package is now APPROVED.

Also please consider removing the patches that are no longer used from the devel branch:
xmlto-libpaper.patch
xmlto-stringparam.patch
xmlto-xhtml1.patch
xmlto-xmllintoptions.patch

(make unused-patches shows you these patches)

Thanks for review! Patches removed, preserving timestamps added, noarch subpackages done and one wrong changelog entry fixed (just forgot to write version there ;) ) - built as xmlto-0.0.22-3.fc12. Closing RAWHIDE.