IMO I think the spec is invalid.

https://www.resiprocate.org/files/pub/sipX/sipxtapi-3.3.0~test10.tar.gz

is not the final 3.3.0 release.

Rather, I have tracked the upstream project in a github fork to adapt the build system for packaging.

If the review identifies more changes needed in the spec file or upstream build system, those will be implemented before the final 3.3.0 tarball is created.  The real 3.3.0 tarball will then be ready for Fedora.

sipXtapi is not the same as reSIProcate.  They both provide an independent SIP stack implementation.  reSIProcate includes a conferencing library, librecon.  The conferencing library uses the media framework from sipxtapi.  That is why this tarball is on the reSIProcate web server.

Christopher, the spec I linked to is a template, it is meant to be parsed by autotools, it inserts the version number, package name, etc.  The template is kept in upstream git, the final generated spec file is produced automatically when generating a tarball.

The generated spec file is inside the tarball.  It builds cleanly with:

rpmbuild -tb sipxtapi-3.3.0~test10.tar.gz

I've placed a standalone copy of the latest spec file here:

  https://github.com/dpocock/sipxtapi-rpm/blob/master/sipxtapi.spec

and the latest tarball is available here:

  http://www.resiprocate.org/files/pub/sipX/

Yes, because you give us a template, we can also refuse to review your package, who knows exactly the content is.

First glimpse at spec:

1. Drop

Vendor: SIPfoundry
Packager: Daniel Pocock <daniel.au>

2. It's better to add --disable-static to the configure.

3. Why providing a libs subpackage? Can't it be merged into main package?

4. I don't hope you include "test" in the version field.

5. I'm sorry in the past years I never use Group tag, and you use Group: Development/Libraries in 2 different subpackages, I think it's not perfect, recommend dropping it.

The template only inserts the @VERSION@ at build time.  We use a template to generate the spec file so that RPM users will always be supported automatically with every upstream tarball.  Most RPM users are very happy with this approach. 

Anyhow, I've now made all the changes you suggested and created another standalone spec file for you.  Can you please confirm this is OK to go into Fedora now?

Also, the ~test suffix is used on the tarball because it has not been tagged upstream yet.

We are waiting to make sure that the tarballs work for Fedora users before tagging and confirming the 3.3.0 release.

Once you confirm it is OK, the 3.3.0 tag will be created and that can be the first version uploaded into Fedora

This is unacceptable.

Provide a tested (!) src.rpm at a direct download location. Point at it in the "SRPM URL" fields of this ticket. You may use your fedorapeople.org web space for hosting the src.rpm and the spec file.

The spec file does not follow the packaging guidelines in various places. It even refers to a -libs subpackage that isn't built by the spec file.

Run rpmlint (or rpmlint -I for more helpful output) on the src.rpm *and* all
built rpms. Feel free to ignore obvious false positives in the report, but fix
anything else. Preferably add a comment here about whether/when you think what
rpmlint reports is correct or incorrect.

Where is the new spec?

1. If it's the one mentioned above, please remember buming the release to higher one such as 1-->2.

And, release tag is invalid.

2. %package devel
Requires: %{name}-libs%{?_isa} = %{version}-%{release}
Summary: Header files for %name

Of course, this is not a headers only package.

Suggestion:

%package           devel
Summary:           Development files for %{name}
Requires:          %{name}-libs%{?_isa} = %{version}-%{release}

%description       devel
The %{name}-devel package contains libraries and header files for
developing applications that use %{name}.

3. Remove

BuildRequires: gcc-c++

4. Please don't use such:

%name-%version

use

%{name}-%{version}

instead.

5. License: LGPL

is wrong, should be

License: LGPLv2

There's more without looking too long...


> Requires: openssl >= 0.9.8

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
and %{?_isa} would be missing anyway.


> BuildRequires: cppunit-devel

There are unit tests available? Any of them suitable for a %check section?


> Requires: %{name}-libs%{?_isa} = %{version}-%{release}

-libs is a wrong leftover here.


> %package apidoc

Typically, we call them -doc not -apidoc:
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation


> %{_datarootdir}

The package must not own /usr/share.

I've fixed those issues too, the -libs thing had been fixed but hadn't all been pushed to github

Can somebody please comment on the correct way to install the docs?

Which of these should contain the docs:

/usr/share/doc/sipxtapi/
/usr/share/doc/sipxtapi-doc-%{version}/

I notice it is currently installing to the former, if I have to use the latter, what is the preferred way of doing that in the spec file?

In general, docs get installed where _you_ install them or where the software package installs them.

However, if you use %doc in a %files list, it does two different things depending on what files you apply it to. For local files in the build dir, it copies them to

  %{_docdir}/%{name}-%{version}

whereas when applying %doc to absolute paths, it just marks files as being Documentation files (e.g. for RPM queries or --excludedocs installations).


For Fedora 20, things have changed, and %doc will copy to

  %{_docdir}/%{name}

without the trailing version. Check out:

  https://fedoraproject.org/wiki/Changes/UnversionedDocdirs


When using %doc for the files of a subpackage, %{name} will be the subpackage name. So, in a -doc subpackage, %doc installs into:

  %{_docdir}/%{name}-doc-%{version}

Unversioned for F20, of course.

Some packagers don't like the multiple doc dirs that are the result of using %doc in subpackages. They install everything themselves during %install into a single directory (versioned or not), and files below %_docdir are marked %doc automatically, too.

HTH

I've now revised the spec file again and released a new test tarball, it is available as an SRPM this time:

http://download.sipxtapi.org/files/pub/sipX/

$ rpmlint sipxtapi-3.3.0~test12-5.src.rpm
sipxtapi.src: W: spelling-error Summary(en_US) codecs -> codes, coders, code's

 - codec/codecs is the correct term

sipxtapi.src:53: W: macro-in-comment %check

 - the section is deliberately commented, the comments explain why

1 packages and 0 specfiles checked; 0 errors, 2 warnings.

I strongly advice you to refresh your spec and srpm at the same time.

Issues:
=======
- Sources used to build the package match the upstream source, as provided in
  the spec URL.

- Package does not contain any libtool archives (.la)
  Note: sipxtapi : /usr/lib/sipxtapi/codecs/codec_g722.la sipxtapi :
  /usr/lib/sipxtapi/codecs/codec_gsm.la sipxtapi :
  /usr/lib/sipxtapi/codecs/codec_l16.la sipxtapi :
  /usr/lib/sipxtapi/codecs/codec_pcmapcmu.la sipxtapi :
  /usr/lib/sipxtapi/codecs/codec_tones.la
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries
- Large documentation must go in a -doc subpackage.
  Note: Documentation size is 1310720 bytes in 237 files.
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation

License:
=======
Found:
     "BSD (4 clause) ISC", "GPL (v2 or later)", "Unknown or generated", "BSD
     (4 clause)", "*No copyright* zlib/libpng", "zlib/libpng", "ISC", "BSD (3
     clause)". 1389 files have unknown license. 

And:
=======

sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXport.so.2.0.0 /lib/libm.so.6
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXsdp.so.1.0.0 /lib/libm.so.6
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXmedia.so.1.0.0 /lib/libgsm.so.1
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXmediaProcessing.so.1.0.0 /lib/libssl.so.10
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXmediaProcessing.so.1.0.0 /lib/libcrypto.so.10
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXmediaProcessing.so.1.0.0 /lib/libpcre.so.1
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXmediaProcessing.so.1.0.0 /lib/libdl.so.2
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXmediaProcessing.so.1.0.0 /lib/libpthread.so.0
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXmediaProcessing.so.1.0.0 /lib/libgsm.so.1
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXmediaProcessing.so.1.0.0 /lib/librt.so.1
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXmediaProcessing.so.1.0.0 /lib/libresolv.so.2
sipxtapi.i686: W: unused-direct-shlib-dependency /usr/lib/libsipXmediaProcessing.so.1.0.0 /lib/libm.so.6

Also:
========

sipxtapi: /usr/lib/sipxtapi/codecs/codec_g722.so
sipxtapi: /usr/lib/sipxtapi/codecs/codec_gsm.so
sipxtapi: /usr/lib/sipxtapi/codecs/codec_l16.so
sipxtapi: /usr/lib/sipxtapi/codecs/codec_pcmapcmu.so
sipxtapi: /usr/lib/sipxtapi/codecs/codec_tones.so

Cannot build the package yet and would need to fix it first:

| ERROR: SRPM file name was invalid.
| Message: 'Illegal character '~' encountered.'

https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Common_Character_Set_for_Package_Naming

It builds fine on F19

I understand the ~ is not supported on older Fedora versions and RHEL6

Nonetheless, the ~ is used to denote pre-release tarballs such as this one.  The proper release (without a tilde in the name) will be made shortly, but before we release, we need to know if any essential changes are needed to support Fedora.

When it is officially uploaded into Fedora, we will upload the final release tarball

Do you have an F19 or rawhide environment where you can test the tarball?

> It builds fine on F19

No, it doesn't. Perhaps a plain rpmbuild succeeds, but we're using Mock and/or build servers. Plague rejects the src.rpm. I've changed the src.rpm and submitted another build job.

  Name: sipxtapi
  Version: 3.3.0

  %global tartopdir %{name}-%{version}~test12

  Source: http://download.sipxtapi.org/files/pub/sipX/%{tartopdir}.tar.gz

  %setup -q -n %{tartopdir}

Have you submitted a scratch-build in the Fedora Build System (koji) yet?

$ rpmls -p sipxtapi-doc-3.3.0-5.x86_64.rpm|grep ^d
drwxr-xr-x  /usr/share/doc/sipxtapi/sipXcallLib
drwxr-xr-x  /usr/share/doc/sipxtapi/sipXmediaAdapterLib
drwxr-xr-x  /usr/share/doc/sipxtapi/sipXmediaLib
drwxr-xr-x  /usr/share/doc/sipxtapi/sipXportLib
drwxr-xr-x  /usr/share/doc/sipxtapi/sipXsdpLib
drwxr-xr-x  /usr/share/doc/sipxtapi/sipXtackLib

Directory /usr/share/doc/sipxtapi is missing in this package.
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

$ rpm -qpR sipxtapi-doc-3.3.0-5.x86_64.rpm|grep -v rpm
$


> drwxr-xr-x  /usr/share/doc/sipxtapi-devel-3.3.0
> -rw-r--r--  /usr/share/doc/sipxtapi-devel-3.3.0/COPYING

You don't need to duplicate the license file in the -devel package, because that package depends on the base package already, which includes the same file:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing

drwxr-xr-x  /usr/share/doc/sipxtapi-3.3.0
-rw-r--r--  /usr/share/doc/sipxtapi-3.3.0/COPYING


$ rpmls -p sipxtapi-3.3.0-5.x86_64.rpm |grep \.a$
-rwxr-xr-x  /usr/lib64/sipxtapi/codecs/codec_g722.la
-rwxr-xr-x  /usr/lib64/sipxtapi/codecs/codec_l16.la
-rwxr-xr-x  /usr/lib64/sipxtapi/codecs/codec_pcmapcmu.la
-rwxr-xr-x  /usr/lib64/sipxtapi/codecs/codec_tones.la

Does it use these libtool archives at run-time? (for example, if it uses an old libtool dlopen instead of the normal dlopen)

Else delete them. It's covered in an unrelated section here:
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


So much about the test-builds.

[...]


> License: LGPL

LGPLv2 - https://fedoraproject.org/wiki/Licensing:Main#SoftwareLicenses

If you could submit a comment here in the review ticket with two valid "Spec URL:" and "SRPM URL:" fields, you could run the "fedora-review" tool and point it at this ticket.

In the source code there are files with a different non-LGPL license header. What is known about that?


> BuildRequires: gsm-devel

Build log says:

  checking for libgsm >= 1.0.10... not found
  Codec iLBC was disabled
  Codec G.726 was disabled

Package doesn't depend on libgsm either.


> BuildRequires: xerces-c-devel

Seems unused, too. Please verify.


> configure: WARNING: no wx-config found - wxWidgets disabled

?


> W: macro-in-comment %check

"rpmlint -i …" gives a hint. Since you don't want a %check section, either delete it completely, or when commenting it out use a double-%: #%%check
That disables the macro.


> %defattr(-, root, root, -)

Is _really_ not needed anymore since RPM 4.4. ;)

Spec URL: https://github.com/dpocock/sipxtapi-rpm/raw/master/sipxtapi.spec

SRPM URL: http://www.resiprocate.org/files/pub/sipX/sipxtapi-3.3.0~test12-6.src.rpm

Created attachment 784745 [details]
licensecheck.txt done by fedora-review tool

"fedora-review -b 961237" complains about a few things, not limited to the src.rpm containing a spec file different from the separately linked spec file.

The license headers it finds:

  *No copyright* zlib/libpng
  BSD (3 clause)
  BSD (4 clause)
  BSD (4 clause) ISC
  GPL (v2 or later)
  ISC
  Unknown or generated
  zlib/libpng

[...]

> - Package does not contain duplicates in %files.
>   Note: warning: File listed twice: /usr/share/doc/sipxtapi/sipXcallLib
>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles

Indeed. Change the following from

  %files doc
  %doc %{_docdir}/%{name}/*
  %{_docdir}/%{name}/

to:

  %files doc
  %{_docdir}/%{name}/

What you wanted would be:

  %files doc
  %{_docdir}/%{name}/*
  %dir %{_docdir}/%{name}

Files below %_docdir are marked %doc automatically, btw.

[...]

Rpmlint (installed packages)
----------------------------
# rpmlint sipxtapi-doc sipxtapi sipxtapi-devel
sipxtapi-doc.x86_64: W: spelling-error %description -l en_US softphones -> soft phones, soft-phones, sousaphones
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libssl.so.10
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libcrypto.so.10
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libpcre.so.1
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libdl.so.2
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libpthread.so.0
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libgsm.so.1
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/librt.so.1
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libresolv.so.2
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXsdp.so.1.0.0 /lib64/libm.so.6
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXmedia.so.1.0.0 /lib64/libgsm.so.1
sipxtapi.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsipXport.so.2.0.0 /lib64/libm.so.6
sipxtapi-devel.x86_64: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 13 warnings.
# echo 'rpmlint-done:'

On 09/08/13 09:38, bugzilla wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=961237
>
>
>
> --- Comment #20 from Michael Schwendt <bugs.michael> ---
> Created attachment 784745 [details]
>   --> https://bugzilla.redhat.com/attachment.cgi?id=784745&action=edit
> licensecheck.txt done by fedora-review tool
>
> "fedora-review -b 961237" complains about a few things, not limited to the
> src.rpm containing a spec file different from the separately linked spec file.
>
> The license headers it finds:
>
>   *No copyright* zlib/libpng

That appears to be a false positive
https://github.com/sipXtapi/sipXtapi/blob/master/sipXcallLib/examples/include/nss/zlib.h
contains a complete copyright and license info

libpng does not appear to be included in the tree, it is used by some of
the examples but is not a dependency of the binary packages.


>   BSD (3 clause)
>   BSD (4 clause)
>   BSD (4 clause) ISC

These all appear compatible with the stated LGPL-2 license except for BSD 4 clause

I checked the 4-clause BSD code

It is code that is covered by the UCB statement retrospectively
relicensing all their code as 3-clause, specifically:

ftp://ftp.cs.berkeley.edu/pub/4bsd/README.Impt.License.Change

>   GPL (v2 or later)
Some source files in the examples have a GPL license.  They are not
compiled or distributed in the binary RPMs, the package works without them.

I've added comments about all of these extra licenses to the spec file, can you please look at it in github and see if the comments are sufficient to explain this package?

https://github.com/dpocock/sipxtapi-rpm/blob/master/sipxtapi.spec

>   ISC
>   Unknown or generated
>   zlib/libpng
>
> [...]
>
>> - Package does not contain duplicates in %files.
>>   Note: warning: File listed twice: /usr/share/doc/sipxtapi/sipXcallLib
>>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles
> Indeed. Change the following from
>
>   %files doc
>   %doc %{_docdir}/%{name}/*
>   %{_docdir}/%{name}/
>
> to:
>
>   %files doc
>   %{_docdir}/%{name}/

Fixed

> What you wanted would be:
>
>   %files doc
>   %{_docdir}/%{name}/*
>   %dir %{_docdir}/%{name}
>
> Files below %_docdir are marked %doc automatically, btw.
>
> [...]
>
> Rpmlint (installed packages)
> ----------------------------
> # rpmlint sipxtapi-doc sipxtapi sipxtapi-devel
> sipxtapi-doc.x86_64: W: spelling-error %description -l en_US softphones -> soft
> phones, soft-phones, sousaphones
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libssl.so.10
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libcrypto.so.10
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libpcre.so.1
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libdl.so.2
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libpthread.so.0
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libgsm.so.1
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/librt.so.1
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXmediaProcessing.so.1.0.0 /lib64/libresolv.so.2
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXsdp.so.1.0.0 /lib64/libm.so.6
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXmedia.so.1.0.0 /lib64/libgsm.so.1
> sipxtapi.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libsipXport.so.2.0.0 /lib64/libm.so.6
> sipxtapi-devel.x86_64: W: no-documentation
> 3 packages and 0 specfiles checked; 0 errors, 13 warnings.
> # echo 'rpmlint-done:'
>

spelling-error seems odd.  If we can't use "softphone", then words like
"software" should become "soft-ware" perhaps

I've suppressed spelling-error and no-documentation

Do the other issues definitely need to be fixed upstream or can the package be accepted despite the warnings?

The other issues require further work on the autotools build system.  It is quite complicated because there are many custom macros and multiple library projects.  It appears to just link some libs (e.g. libssl) for anything, and it needs to be changed to link those libs selectively when building libs that depend on them.

> *No copyright* zlib/libpng

That refers to the  sipXportLib/include/xmlparser/tinystr.*  files. The fedora-review tool saves a long list licensecheck.txt with the full paths.


> spelling-error seems odd

Don't worry too much about them. It's normal that there may be false positives.


> the warnings?

They are just warnings, not MUST-fix errors. Also see:
https://fedoraproject.org/wiki/Common_Rpmlint_issues

(In reply to Michael Schwendt from comment #22)
> > *No copyright* zlib/libpng
> 
> That refers to the  sipXportLib/include/xmlparser/tinystr.*  files. The
> fedora-review tool saves a long list licensecheck.txt with the full paths.


I ran fedora-review and it gave me an empty licensecheck.txt - maybe I'm not running it correctly

Anyhow, I've looked at those files, it is clear who the authors are, even though they didn't write the copyright line in the normal way.

I understand that packagers can't modify licenses for obvious reasons, but I will look at this as an upstream issue and arrange to have the author names inserted more explicitly above their license block and zlib listed in the license comments.  The authors clearly identify themselves in their documentation/web site, under Contributors:
http://www.grinninglizard.com/tinyxmldocs/index.html

The zlib license itself is OK, it is GPL compatible:
http://www.gnu.org/licenses/license-list.html#ZLib
and therefore the package is OK as an LGPLv2 license

Is that the only other thing I need to deal with upstream to satisfy license/copyright check?

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.