Bug 198758

== Not an official review as I'm not yet sponsored ==
   Mock build for rawhide i386 is Failed. I need to add 
BuildReuires: perl-XML-Parser
   After that also i got errors as
+ /usr/lib/rpm/redhat/find-lang.sh
/var/tmp/gnome-phone-manager-0.7-1.fc6-root-mockbuild gnome-phone-manager
No translations found for gnome-phone-manager in
/var/tmp/gnome-phone-manager-0.7-1.fc6-root-mockbuild
error: Bad exit status from /var/tmp/rpm-tmp.77473 (%install)

* MUST Items:
      - rpmlint shows no error. 
      - dist tag is present.
      - The package is named according to the Package Naming Guidelines.
      - The spec file name matching the base package gnome-phone-manager, in the
format gnome-phone-manager.spec.
      - This package meets the Packaging Guidelines.
      - The spec file for the package MUST be legible.
      - The package is licensed with an open-source compatible license GPL.
      - This package includes License file COPYING.
      - This source package includes the text of the license in its own file,and
that file, containing the text of the license for the package is included in %doc.
      - The sources used to build the package matches the upstream source,
as provided in the spec URL. md5sum is correct (951471bf5d6fe93fe550c60b6bdf58f9)
      - This package did NOT successfully compiled and built into binary rpms
for i386 architecture.
      - This package did not containd any ExcludeArch.
      - This package did NOT handled locales properly. This is done by using the
%find_lang macro. Not used %{_datadir}/locale/*.
      - This package  have a %clean section, which contains rm -rf
$RPM_BUILD_ROOT.
      - This package used macros.
      - Document files are included like README, NEWS, COPYING, AUTHORS
      - Package did NOT contained any .la libtool archives.

Also,
      * Source URL is present and working.
      * BuildRoot is correct BuildRoot:       
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
      * BuildRequires is correct
  

(In reply to comment #1)
> == Not an official review as I'm not yet sponsored ==
>    Mock build for rawhide i386 is Failed. I need to add 
> BuildReuires: perl-XML-Parser

That should be: 

BuildReuires: perl(XML::Parser)

>    After that also i got errors as
> + /usr/lib/rpm/redhat/find-lang.sh
> /var/tmp/gnome-phone-manager-0.7-1.fc6-root-mockbuild gnome-phone-manager
> No translations found for gnome-phone-manager in
> /var/tmp/gnome-phone-manager-0.7-1.fc6-root-mockbuild
> error: Bad exit status from /var/tmp/rpm-tmp.77473 (%install)

That's usually a sign of needing:

BuildRequires: gettext

> 
> * MUST Items:
>       - rpmlint shows no error. 
>       - dist tag is present.
>       - The package is named according to the Package Naming Guidelines.
>       - The spec file name matching the base package gnome-phone-manager, in the
> format gnome-phone-manager.spec.
>       - This package meets the Packaging Guidelines.
>       - The spec file for the package MUST be legible.
>       - The package is licensed with an open-source compatible license GPL.
>       - This package includes License file COPYING.
>       - This source package includes the text of the license in its own file,and
> that file, containing the text of the license for the package is included in %doc.
>       - The sources used to build the package matches the upstream source,
> as provided in the spec URL. md5sum is correct (951471bf5d6fe93fe550c60b6bdf58f9)
>       - This package did NOT successfully compiled and built into binary rpms
> for i386 architecture.
>       - This package did not containd any ExcludeArch.
>       - This package did NOT handled locales properly. This is done by using the
> %find_lang macro. Not used %{_datadir}/locale/*.

It's a good idea to list things that need fixing separately from the rest of the
review checklist as that's clearer and easier to read.

>       - This package used macros.

But did it use them *consistently*? That's what the review guidelines are asking
to be checked.

>       - Document files are included like README, NEWS, COPYING, AUTHORS

Did you look to see if there are any other document files in the package that
might be included, or whether any of the included files don't have anything
useful to end users of the package?

>       - Package did NOT contained any .la libtool archives.
> 
> Also,
>       * Source URL is present and working.
>       * BuildRoot is correct BuildRoot:       
> %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
>       * BuildRequires is correct

No, they're not. The package failed to build in mock because of the missing
buildreqs of perl(XML::Parser) and gettext.

(In reply to comment #2)
> (In reply to comment #1)
> > == Not an official review as I'm not yet sponsored ==
> >    Mock build for rawhide i386 is Failed. I need to add 
> > BuildReuires: perl-XML-Parser
> 
> That should be: 
> 
> BuildReuires: perl(XML::Parser)
> 
> >    After that also i got errors as
> > + /usr/lib/rpm/redhat/find-lang.sh
> > /var/tmp/gnome-phone-manager-0.7-1.fc6-root-mockbuild gnome-phone-manager
> > No translations found for gnome-phone-manager in
> > /var/tmp/gnome-phone-manager-0.7-1.fc6-root-mockbuild
> > error: Bad exit status from /var/tmp/rpm-tmp.77473 (%install)
> 
> That's usually a sign of needing:
> 
> BuildRequires: gettext
> 
> > 
> > * MUST Items:
> >       - rpmlint shows no error. 
> >       - dist tag is present.
> >       - The package is named according to the Package Naming Guidelines.
> >       - The spec file name matching the base package gnome-phone-manager, in the
> > format gnome-phone-manager.spec.
> >       - This package meets the Packaging Guidelines.
> >       - The spec file for the package MUST be legible.
> >       - The package is licensed with an open-source compatible license GPL.
> >       - This package includes License file COPYING.
> >       - This source package includes the text of the license in its own file,and
> > that file, containing the text of the license for the package is included in
%doc.
> >       - The sources used to build the package matches the upstream source,
> > as provided in the spec URL. md5sum is correct
(951471bf5d6fe93fe550c60b6bdf58f9)
> >       - This package did NOT successfully compiled and built into binary rpms
> > for i386 architecture.
> >       - This package did not containd any ExcludeArch.
> >       - This package did NOT handled locales properly. This is done by using the
> > %find_lang macro. Not used %{_datadir}/locale/*.
> 
> It's a good idea to list things that need fixing separately from the rest of the
> review checklist as that's clearer and easier to read.
 Will remember that.
> 
> >       - This package used macros.
> 
> But did it use them *consistently*? That's what the review guidelines are asking
> to be checked.
 Did i missed somthing to check in SPEC?
> 
> >       - Document files are included like README, NEWS, COPYING, AUTHORS
> 
> Did you look to see if there are any other document files in the package that
> might be included, or whether any of the included files don't have anything
> useful to end users of the package?
     I didn't get you?
> 
> >       - Package did NOT contained any .la libtool archives.
> > 
> > Also,
> >       * Source URL is present and working.
> >       * BuildRoot is correct BuildRoot:       
> > %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> >       * BuildRequires is correct
> 
> No, they're not. The package failed to build in mock because of the missing
> buildreqs of perl(XML::Parser) and gettext.
> 
 I forgot that i added perl(XML::Parser) not AUTHOR.

(In reply to comment #3)
> (In reply to comment #2)
> > >       - This package used macros.
> > 
> > But did it use them *consistently*? That's what the review guidelines are asking
> > to be checked.
>  Did i missed somthing to check in SPEC?

I don't know; I haven't looked at the spec. The review guidelines ask you to
check that macros are used consistently, not just that they are used. So you're
looking for things like:

 * Using both $RPM_BUILD_ROOT and %{buildroot}
 - both expand to the same value; use one or the other but not both in the same
spec file

 * Using %{__rm} but not %{__make} etc.
 - if the packager is using macros for some commands (e.g. %{__rm}), they should
use them in all cases where a macro is available; alternatively, they could just
use "rm" instead of "%{__rm}"; the key is consistency

> > >       - Document files are included like README, NEWS, COPYING, AUTHORS
> > 
> > Did you look to see if there are any other document files in the package that
> > might be included, or whether any of the included files don't have anything
> > useful to end users of the package?
>      I didn't get you?

Supposing the package contains docs:
README, NEWS, COPYING, AUTHOR

Did you check to see if there are any other useful documents in the tarball that
might be useful to end users? There might be some other .txt files, a docs
directory full of useful HTML files, perhaps a TODO file? A reviewer should be
looking at a package in much the same way as they would do if they were
packaging something themselves, looking to see what would be sensible to include.

Also on the subject of documentation, sometimes the documentation provided in
tarballs isn't very useful. So sometimes you may see a "NEWS" file that just
says to look in the "ChangeLog" file. There is no point in including such a
"NEWS" file in the package. That's something a reviewer should be looking at too.

> > >       * BuildRequires is correct
> > 
> > No, they're not. The package failed to build in mock because of the missing
> > buildreqs of perl(XML::Parser) and gettext.
> > 
>  I forgot that i added perl(XML::Parser) not AUTHOR.

There's also the missing gettext buildreq that caused the package build to fail
in mock.

Thnaks for your comment Paul.
 This Package requires todo following things
1) Add
BuildRequires: perl(XML::Parser),gettext

2)Package is using mixed macros as explained by Paul in
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=198758#c4
Either use %{buildroot} or $RPM_BUILD_ROOT

3)I think with README, NEWS, COPYING, AUTHOR these file TODO is also important
so that let the user know what things AUTHOR is interested to implement in
future versions.

OK issues adressed:

Spec URL: http://www.df.lth.se/~triad/krad/fc/gnome-phone-manager.spec
SRPM URL: http://www.df.lth.se/~triad/krad/fc/gnome-phone-manager-0.7-2.src.rpm

As for macro consistency, I think that was all OK from the beginning...

There is at least one instance where you use 'rm', and then '%{__rm}'.  The
guidelines require that you use one or the other form consistently. ("MUST")

Also, the package fails to build under mock:
/usr/bin/ld: warning: libplds4.so, needed by
/usr/lib/gcc/x86_64-redhat-linux/4.1.1/../../../../lib64/libedataserver-1.2.so,
not found (try using -rpath or -rpath-link)
/usr/bin/ld: warning: libplc4.so, needed by
/usr/lib/gcc/x86_64-redhat-linux/4.1.1/../../../../lib64/libedataserver-1.2.so,
not found (try using -rpath or -rpath-link)
/usr/bin/ld: warning: libnspr4.so, needed by
/usr/lib/gcc/x86_64-redhat-linux/4.1.1/../../../../lib64/libedataserver-1.2.so,
not found (try using -rpath or -rpath-link)
/usr/bin/ld: warning: libssl3.so, needed by /usr/lib64/libcamel-1.2.so.0, not
found (try using -rpath or -rpath-link)
/usr/bin/ld: warning: libsmime3.so, needed by /usr/lib64/libcamel-1.2.so.0,
not found (try using -rpath or -rpath-link)
/usr/bin/ld: warning: libnss3.so, needed by /usr/lib64/libcamel-1.2.so.0, not
found (try using -rpath or -rpath-link)
/usr/lib64/libcamel-1.2.so.0: undefined reference to
`NSS_InitReadWrite'

(Full log attached.)  I suspect this is due to nspr & nss not being
buildrequires'ed, from the names of the libraries being referenced.

Additionally, are the scriptlets necessary?  The .desktop file does not have a
MimeType entry (see wiki: ScriptletSnippets).  In any case, desktop-file-utils
should not be required for the scriptlets and the scriptlets should be
tolerant of the desktop-file-utils not being installed.

After installing, I find I have two entries for the app in my menus.

X package meets naming and packaging guidelines (release tag).
X specfile is properly named, is cleanly written and uses macros consistently.
+ Package URL is browsable.
+ dist tag is present.
+ build root is correct.
+ license field matches the actual license.
+ license is open source-compatible. License text included in package.
+ source files match upstream:
951471bf5d6fe93fe550c60b6bdf58f9  gnome-phone-manager-0.7.tar.bz2
951471bf5d6fe93fe550c60b6bdf58f9  gnome-phone-manager-0.7.tar.bz2.srpm
+ latest version is being packaged.
X BuildRequires are proper.
X package builds in mock (devel/x86_64).
+ rpmlint is silent.
X final provides and requires are sane:
== provides
gnome-phone-manager = 0.7-2.fc5
== requires
 /bin/sh
X desktop-file-utils
 libICE.so.6()(64bit)
 libORBit-2.so.0()(64bit)
 libSM.so.6()(64bit)
 libX11.so.6()(64bit)
 libart_lgpl_2.so.2()(64bit)
 libatk-1.0.so.0()(64bit)
 libbluetooth.so.1()(64bit)
 libbonobo-2.so.0()(64bit)
 libbonobo-activation.so.4()(64bit)
 libbonoboui-2.so.0()(64bit)
 libbtctl.so.2()(64bit)
 libc.so.6()(64bit)
 libc.so.6(GLIBC_2.2.5)(64bit)
 libc.so.6(GLIBC_2.3.4)(64bit)
 libc.so.6(GLIBC_2.4)(64bit)
 libcairo.so.2()(64bit)
 libdl.so.2()(64bit)
 libebook-1.2.so.5()(64bit)
 libedataserver-1.2.so.7()(64bit)
 libgconf-2.so.4()(64bit)
 libgdk-x11-2.0.so.0()(64bit)
 libgdk_pixbuf-2.0.so.0()(64bit)
 libglade-2.0.so.0()(64bit)
 libglib-2.0.so.0()(64bit)
 libgmodule-2.0.so.0()(64bit)
 libgnokii.so.2()(64bit)
 libgnome-2.so.0()(64bit)
 libgnome-keyring.so.0()(64bit)
 libgnomebt.so.0()(64bit)
 libgnomecanvas-2.so.0()(64bit)
 libgnomeui-2.so.0()(64bit)
 libgnomevfs-2.so.0()(64bit)
 libgobject-2.0.so.0()(64bit)
 libgthread-2.0.so.0()(64bit)
 libgtk-x11-2.0.so.0()(64bit)
 libm.so.6()(64bit)
 libpango-1.0.so.0()(64bit)
 libpangocairo-1.0.so.0()(64bit)
 libpangoft2-1.0.so.0()(64bit)
 libpopt.so.0()(64bit)
 libpthread.so.0()(64bit)
 libxml2.so.2()(64bit)
 libz.so.1()(64bit)
+ no shared libraries are present.
+ package is not relocatable.
+ owns the directories it creates.
+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ %clean is present.
X %check is not present, but there are no tests
X non-sane scriptlets present.
+ code, not content.
+ documentation is small, so no -docs subpackage is necessary.
+ %docs are not necessary for the proper functioning of the package.
+ no headers.
+ no pkgconfig files.
+ no libtool .la droppings.
X desktop file installs correctly.
+ not a web app.

Created attachment 133370 [details]
mock x86_64/devel build.log

The errors from linking libedataserver and libcamel, which both comes
from the evolution-data-server RPM package, is technically out of scope
for this package as far as I can tell.

gnome-phone-manager does not use either nspr or the rest of the deps
directly, only indirectly through evolution-data-server.
The problem is that the RPM for evolution-data-server does not bring
in its dependencies.

I think it is a problem with evolution-data-server on x64 not 
picking up its own dependencies correctly... I was under the impression 
that this did not occur in mock on i386?

(I'm working on the rest of the issues.)

New version with all but the above mentioned issue fixed:

Spec URL: http://www.df.lth.se/~triad/krad/fc/gnome-phone-manager.spec
SRPM URL: http://www.df.lth.se/~triad/krad/fc/gnome-phone-manager-0.7-3.src.rpm

I am however uncertain about the %check thing, I cannot find this in the
review guidelines or elsewhere, can you give me a pointer to information?
There is perhaps something all new I need to learn here...

Requested changes have been made to spec file.

Looking back over the guidelines, there doesn't appear to be an explict
dependency  on a %check section.  (Normally it's a good idea to have something
of the form "%check\nmake test". See, e.g., almost any perl module package spec
file.)  As this package has no test suite, it doesn't make much sense for me to
insist on it :)

The package builds in mock(5/x86_64) after I added "BuildRequires: 
desktop-file-utils" to the spec.  Add it and I'll approve...

Fixed it:

Spec URL: http://www.df.lth.se/~triad/krad/fc/gnome-phone-manager.spec
SRPM URL: http://www.df.lth.se/~triad/krad/fc/gnome-phone-manager-0.7-4.src.rpm

APPROVED

Ping?  Looks like this was imported and built OK, can we close the bug? :)

It doesn't build on devel because (I think) Evolution Data Server
has changed its interface. I need to communicate with upstream and
perhaps write patches first... :-/

Ok, so let's open up another bug against this component directly about the devel
issues and close out this one.

OK builds nicely in FC5 some problem in devel related to
changes in E-D-S. Would open a bug for it on gnome-phone-manager
but it haven't appeared in Bugzilla yet so can't. Thanks to Chris
for the review and all!