Not an official review as I'm not yet sponsored
Mock build for development i386 is successfull with some warnings as
warning: dereferencing type-punned pointer will break strict-aliasing rules

MUST Items:
     - MUST: rpmlint shows error 
       E: bitlbee configure-without-libdir-spec 
       A configure script is run without specifying the libdir. configure
options must be augmented with something like --libdir=%{_libdir}.
     - MUST: dist tag is present
     - MUST: The package is named according to the Package Naming Guidelines.
     - MUST: The spec file name matching the base package bitlbee, in the
format bitlbee.spec
      - MUST: This package meets the Packaging Guidelines.
      - MUST: The package is licensed with an open-source compatible license GPL.
      - MUST: The License field in the package bitlbee.spec file matches the
actual license file COPYING in tarball.
      - MUST: The sources used to build the package matches the upstream source,
as provided in the spec URL. md5sum is correct.
      - MUST: This package owns all directories that it creates. 
      - MUST: This package did not contain any duplicate files in the %files
listing.
      - MUST: This package  have a %clean section, which contains rm -rf
$RPM_BUILD_ROOT.
      - MUST: This package used macros.
      - MUST: Document files are included like INSTALL README.
      - MUST: This Package include a bitlbee.desktop file, and that file is 
installed with desktop-file-install in the %install section successfully.
      - MUST: Package is calling ldconfig on postun post successfully.
      * Source URL is present.
      * BuildRoot is correct BuildRoot:       
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
      * BuildRequires is correct

What You Need to Do:-
      * Add --libdir=%{_libdir) to %configure
      * Add subpackage -doc as document files are more in base package.
      

The gcc warnings because of dereferencing type-punned pointer have to be handled 
by upstream and IIRC they are even OpenSSL not BitlBee related. Most Extras and 
Core packages are having such warnings, because the world is using older gccs.

To your first point at the todo list: No, no - never, sorry. You didn't look 
into the tarball neither you called ./configure --help by hand. Finally a `grep 
-ri libdir bitlbee-1.0.3` returns nothing. For me there is no reason only to 
make rpmlint happy by applying unneccessary and even unused parameters.

To your second point at the todo list: I can't aggree with you when reading the 
Fedora (Extras) Packaging Guidelines. There is neither "a lot of documentation" 
nor is runtime (when installing/updating/erasing) affected by handling exactly 
16 files consuming maximal 150 KB of space.

Waiting for another opinion before changing anything. XML documentation could
be skipped as there is very less similar in /usr/share/doc/*/ of my system. 
Doing this would reduce %{_docdir} to 100 KB which would just host AUTHORS, 
CHANGES, COPYING, CREDITS, FAQ, README and user-guide.txt. On the other hand, I 
can't absolutely see why such basic files (except the 35 KB of user-guide.txt) 
should be moved into a -doc subpackage...

thanks for your comments. anyway i am still learning thru' reviewing other packages.

Blocker: Package doesn't honor RPM_OPT_FLAGS.
Everything is compiled using -O3.

Verified and accepted. Worked out a working solution which also should get part
of the next BitlBee release. Applying suggested patch from http://bugs.bitlbee.
org/bitlbee/ticket/171 for now to resolve the issue until 1.0.4 is available.

Pushing -3 now. Further comments, hints and feedback?

This (longish) message during ./configure worries me:

---------------------------
No detection code exists for OpenSSL. Make sure that you have a complete
install of OpenSSL (including devel/header files) before reporting
compilation problems.

Also, keep in mind that the OpenSSL is, according to some people, not
completely GPL-compatible. Using GnuTLS or NSS is recommended and better
supported by us. However, on many BSD machines, OpenSSL can be considered
part of the operating system, which makes it GPL-compatible.

For more info, see: http://www.openssl.org/support/faq.html#LEGAL2
                    http://www.gnome.org/~markmc/openssl-and-the-gpl.html

Please note that distributing a BitlBee binary which links to OpenSSL is
probably illegal. If you want to create and distribute a binary BitlBee
package, you really should use GnuTLS or NSS instead.

Also, the OpenSSL license requires us to say this:
 *    "This product includes software developed by the OpenSSL Project
 *    for use in the OpenSSL Toolkit. (http://www.openssl.org/)"
---------------------------

The prototypical casual observer in me asks, "Wouldn't it be better to compile
this against gnutls?"  If there are compelling reasons to compile against
openssl, I suspect FE-LEGAL should be blocked by this bug as well.

IMHO neither GnuTLS nor NSS are providing the same like OpenSSL, otherwise we 
wouldn't ship OpenSSL within Core, right? What is the (possible) legal problem 
you are seeing? At least from my personal understanding any binary linking to 
OpenSSL could be illegal when the local law conflicts with encryption. For me 
this sounds similar like at the mp3 stuff...

I'm building per default using OpenSSL, because every Fedora Core user has this 
package installed and it is used by many applications (rpm -e --test openssl).
If possible, I want to cause less new dependencies and I also don't want to 
force FE users to install another package/library when the already available
one does the same.

Blocking FE-Legal now, waiting for official response because of the "probably 
illegal", even when I can't see any real reason. Somebody of the legal people
has to remove this blocking when it's resolved.

Is there a technical reason for selecting OpenSSL over gnutls ? Even the bitlbee
developers recommend using gnutls (docs/README). 

Its seems a trival change and seems to be holding up a review for no good
reason. IMHO.

I will do the review on this one. 

----------------------------------------

Review for release 2:
* RPM name is OK
* Source bitlbee-1.0.3.tar.gz is the same as upstream
* This is the latest version
* Builds fine in mock
* rpmlint looks OK
* File list looks OK
* Config files of bitlbee looks OK


Notes:

Once we have a resolve on the OpenSSL issue. This should be good to go. 

Well, I still can't see why I should depend on gnutls when openssl is available 
per default. As from my understanding a bitlbee binary linking to openssl is not 
more or less probably illegal rather a wget binary linking to openssl (there are 
many examples more, just try a rpm -e --test openssl). Also said, bitlbee and 
wget are both licensed under GNU GPL and I absolutely can't see any difference 
between them. Or did Red Hat do some probably illegal things for Fedora Core?

(In reply to comment #10)
> Well, I still can't see why I should depend on gnutls when openssl is available 
> per default. As from my understanding a bitlbee binary linking to openssl is not 
> more or less probably illegal rather a wget binary linking to openssl (there are 
> many examples more, just try a rpm -e --test openssl). Also said, bitlbee and 
> wget are both licensed under GNU GPL and I absolutely can't see any difference 
> between them. Or did Red Hat do some probably illegal things for Fedora Core?

GPL software linking against OpenSSL should include an exception in the license
to allow this. For instance, the wget README file, it says:

  In addition, as a special exception, the Free Software Foundation
  gives permission to link the code of its release of Wget with the
  OpenSSL project's "OpenSSL" library (or with modified versions of it
  that use the same license as the "OpenSSL" library), and distribute
  the linked executables.  You must obey the GNU General Public License
  in all respects for all of the code used other than "OpenSSL".  If you
  modify this file, you may extend this exception to your version of the
  file, but you are not obligated to do so.  If you do not wish to do
  so, delete this exception statement from your version.

Yes, indeed. However, should OpenSSL be the default if there are alternatives
that can be used and when the developers themselves recommend gnutls. Why go the
path of legal grey area when there is a prefectly acceptable alternative? 

The configure scripts don't even detect for OpenSSL! It just assumes you are
using OpenSSL if you do not specify gnutls or nss. 

You still haven't provided a techincal reason to use OpenSSL. "Becuase they did
too" isn't really a reason. 

There is no need for this to depend on OpenSSL, its only blocking FE-LEGAL and
holding up the review.

Okay Michael, I changed what you were complaining about and pushed -4. Otherwise
I won't get BitlBee into Fedora Extras before Windows Vista ;-) Waiting for FE-
ACCEPT now...

oops. I hadnt seen this request, so i had started my own submission.

Robert, please see bug 204884, where I made my package of bitlbee, having missed
yours. I added one patch to fix an accept() call warning on x86_64. I also just
used openssl since everyone has that installed. My configure also has some
different arguments then yours. I am not sure why you need to make those perl
calls. I didn't seem to need that.

I used condrestart for xinetd, instead of just blindly starting it. I'll check
your  source rpm's xinetd file to see if you only bind to 127.0.0.1 as well, and
install and compile it to see how it works on my system compared to the package
I had made. 

Can bitlbee actually write to its config dir which you chown as "daemon"? I will
do more testing for your package later today. It's time this package moves forward.

As a seperate upstream bug, I think bitlbee's proxy use is not working, but that
also needs more testing on my end.

I'll review your x86_64 patch and add when there's a reference to a upstream 
bug report having the patch also applied (and maybe reviewed by upstream).

Regarding openssl vs. gnutls please read the comments above. I'm not interested 
in gnutls but to make Michael happy, I'm using it - for the same non-technical 
reason you provide, too.

Using condrestart for xinetd is accepted and will be added. I'm only binding to 
127.0.0.1 and the package itself works for me about a year as you can see from 
my changelog ;-)

This also should answer the question regarding "daemon"; yes, bitlbee can write 
to /var/lib/bitlbee, because it's set in the xinetd file. You're simply doing 
exactly the same, but using the user "nobody" for. But it would be interesting 
to know whether "daemon" or "nobody" is better...

I think, I'll push a new package when you've the testing completed and I got 
more input.

Oh, I forgot to talk about the perl calling: You're using /usr/bin/install 
rather /usr/bin/make in %install, so it doesn't matter when you're breaking 
anything and fixing none of them. The %configure, which is a macro, is also 
useless, because the configure script of bitlbee doesn't recognize any of
these given parameters (just have a look into it).

I no longer have time to review this. Sorry, putting back out in the wild.
Hopefully its picked up soon. 

I'll happilly take it on for approval. It does not matter much to me who
maintains the package.

I didn't use __install or __rm because that's the FE preferred method. It is the
same reason I use %configure, and not ./configure, despite bitlbee not
supporting all options.  make vs %{__make} I am not sure what the official FE
policy is, I find both being used.

I noticed my package did not properly install the help files. They are not
available. Your package does correctly install the help file. 

I am not sure why you Require: xmlto, as it seems to be this is not used at all.
I cannot find any calls to xmlto/xmlif in the building, and the xmlto package
only provides two binaries, no libraries.

I am not sure I understand your comments on install/make and breaking/not fixing
things.

My spec file also contained the post/postun/preun entries, which you should
copy. It properly adds the bitlbee service so that programs as 'ntsysv' can
enable/disable these (xinetd) services properly. Also add the appropriate
Requires (see my spec file).

As for which user to use. I guess neither daemon nor nobody is supposed to own
files. We could add a user bitlbee to run as instead? This user would not need
Fedora registration, as it is completely irrelevant what uid/gid it would get.

The accept() issue has been reported by me to upstream:
http://bugs.bitlbee.org/bitlbee/ticket/200

If you create a new package, I will do some more testing again.
I noticed my version did not correctly support any of the proxy settings, but I
am not sure if that is a building issue (missing certain libraries?) or whether
this is a generic upstream issue.

Why should I enable 'ntsysv' variant of bitlbee? When reading bitlbee.conf, I 
think, I shouldn't do that (on the other hand: How to (re-)name two identical 
services to avoid confusions in /usr/sbin/ntsysv?):

##  Inetd -- Run from inetd (default)
##  Daemon -- Run as a stand-alone daemon -- EXPERIMENTAL! BitlBee is not yet
##    stable enough to serve lots of users from one process. Because of this
##    and other reasons, the use of daemon-mode is *STRONGLY* discouraged,
##    don't even *think* of reporting bugs when you use this.

I need "those perl calls" you mentioned in comment #14, to do a %makeinstall 
rather a couple of "install -mXYZ" like you are doing. Finally it saves nothing, 
so we could switch over when this makes you even more happy.

Hmm...xmlto really seems to be required no longer, it's just a hangover from 
older times.

Regarding the proxy stuff, could it be a problem openssl vs. gnutls? Personally, 
I would be happy if there's a technical reason to prefer openssl over gnutls ;-)

Ah, adding an own user for bitlbee is a very good idea!

*** Bug 204884 has been marked as a duplicate of this bug. ***

Paul...ping?

Did you create a new package for me to try out with the fixes? Can you give me a url. Don't forget to 
increase the release number. 

Paul, you didn't answer my questions regarding ntsysv, perl calls and proxy 
stuff from comment #19. Until these things aren't clarified, I'll build no new 
package.

chkconfig --add bitlbee will just make sure the service is "registered" with the
management system that programs such as ntsysv use to enable/disable services.
From the man page:

       --add name

              This option adds a new  service  for  management  by  chkconfig.
              When  a new service is added, chkconfig ensures that the service
              has either a start or a kill entry in  every  runlevel.  If  any
              runlevel  is missing such an entry, chkconfig creates the appro-
              priate entry as specified by the  default  values  in  the  init
              script.  Note  that default entries in LSB-delimited ’INIT INFO’
              sections take precedence  over  the  default  runlevels  in  the
              initscript.

install -m ... vs %makeinstall and perl. I personally like the install -m lines
over perl replacements of Makefiles. But whatever you decide is fine.

I still need to do more testing with the proxy stuff. I wouldn't stall the
package on that though.

Hey...next time, please read the whole 'man chkconfig' and not only the part 
you're looking for:

chkconfig provides a simple command-line tool for maintaining the /etc/rc[0-6].d 
directory hierarchy by relieving system administrators of the task of directly 
manipulating the numerous symbolic links in those directories. [...]

chkconfig also can manage xinetd scripts via the means of xinetd.d configuration 
files. Note that only the on, off, and --list commands are supported for xinetd.
d services. [...]

And regarding reload vs. condrestart, there's no good reason to use condrestart 
over reload especially when looking to other xinetd files in Fedora Extra CVS:
http://cvs.fedora.redhat.com/viewcvs/*checkout*/devel/proftpd/proftpd.spec?
root=extras
http://cvs.fedora.redhat.com/viewcvs/*checkout*/devel/uw-imap/uw-imap.spec?
root=extras

Nevertheless, ever sent a "killproc xinetd -HUP" when xinetd was not running? It 
absolutely doesn't matter and guessing that's the reason why uw-imap and proftpd 
just do it. I'll strictly follow the Fedora Extras "examples" staying there for 
years now, especially when there's no official or inofficial guideline.

condrestart still seems the proper way of doing it for xinetd. 

1) don't try to send signals to non-existing processes
2) don't try and start something that isn't running
3) we don't want to return errors in exit codes when failing to signal something
non-existing

Is there any reason why *not* to use condrestart?

Perhaps proftpd and uw-imapd send the signals because they also have a
standalone mode without being run from inetd? 

I did not know about chkconfig not needing the --add command. Sorry about that.

I would still like to see a new package to test. Or if you want to wait on the
proxy testing, that's fine too, but that might take me a day or two to get
around to.

BTW, "/sbin/service xinetd reload > /dev/null 2>&1 || :" will catch the return 
errors and exit codes. But I'll use condrestart, as I was wrong with the meaning 
when the scriptlet is executed (thought on upgrade rather on erasing).

Finally, I would be very happy, if you could do the proxy testing, as it doesn't 
matter for me to wait for another days, now...

Paul, did you finish the proxy testing?

I am still testing it. I ended up not trusting the old copy of the otrproxy I
used for testing, so I ended up upgrading that (and turning it into an FE package).
I should hopefully finish testing tomorrow.

Paul: If you are officially reviewing this package, you should reassign it to 
yourself, and change the blocker from FE-NEW (163776) to FE-REVIEW (163778).

Robert, at this point I can't commit to extensive testing with otrproxy and
britlbee. At this point, either one could be responsible for not working properly.
So i will drop by claim for now that britlbee is broken, and hopefully I can
test it out when I have some more time.

Can you add my accept patch for x86_64 ? I've uploaded it to:
ftp://ftp.xelerance.com/bitlbee/bitlbee-1.0.3-accept.patch

I would still prefer a %configure call, just in case upstream starts adding
support for it. There is no reason not to do so (and makes rpmlint happy)

Kevin: bugzilla does not allow me to re-assign this bug to me. Perhaps because
of a change of user/email within bugszilla. I'll ask around.

Looks like you are not in the 'fedorabugs' group. 

See: 
http://fedoraproject.org/wiki/Extras/Contributors#GetAFedoraAccount
(look at 4.c to request fedorabugs access)

Paul, upstream will never add support for %configure. I already had a longer 
discussion with them. There is absolutely no switch to auto{conf,make} until 
hell freezes completely...and I can't see any reason why to make a not fully 
developed rpmlint happy using stuff that isn't used and thus IMHO wrong.

I pushed -5 to the same directory a few minutes ago reflecting all purposed 
changes from this bug report.

Good:

- package meets naming guidelines
- package meets packaging guidelines
- license (GPL) OK, text in %doc, matches source
- source matches upstream
- spec in american english
- compiles and builds on one arch at least.
- no forbidden buildrequires included
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- doesn't own any files/dirs that are already owned by other packages

Minor:
- rpmlint clean except a warning on not using %configure macro, but
  upstream only provides limited non-auto{conf|make} support, which
  is specified on the configure line.

APPROVED

20335 (bitlbee): Build on target fedora-development-extras succeeded.
20336 (bitlbee): Build on target fedora-6-extras succeeded.
20337 (bitlbee): Build on target fedora-5-extras succeeded.