Bug 486570 (courier-authlib) - Review Request: courier-authlib - The Courier authentication library provides authentication services for other Courier applications.
Summary: Review Request: courier-authlib - The Courier authentication library provides...
Keywords:
Status: CLOSED DUPLICATE of bug 1016221
Alias: courier-authlib
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 208064 (view as bug list)
Depends On:
Blocks: FE-DEADREVIEW KyaPanel courier-imap
TreeView+ depends on / blocked
 
Reported: 2009-02-20 11:59 UTC by Itamar Reis Peixoto
Modified: 2017-01-27 15:37 UTC (History)
12 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2012-04-19 12:29:14 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Itamar Reis Peixoto 2009-02-20 11:59:32 UTC
Spec URL: http://ispbrasil.com.br/courier/courier-authlib.spec
SRPM URL: http://ispbrasil.com.br/courier/courier-authlib-0.62.2-1.fc11.src.rpm
Description: 

The Courier authentication library provides authentication services for
other Courier applications.

Comment 1 Hugo Cisneiros 2009-07-28 22:29:49 UTC
I was told that Allison (past spec creator) was kinda busy to continue with this so I'm helping. Took the spec, upgraded to the newer version of upstream release and used command macros like %{__rm}, %{__install} and so on instead of plain commands.

Spec URL: http://www.devin.com.br/fedora-devel/SPECS/courier-authlib.spec
SRPM URL: http://www.devin.com.br/fedora-devel/courier-authlib-0.62.4-1.fc11.src.rpm

Description: 
The Courier authentication library provides authentication services for
other Courier applications.

Koji scratch build for dist-f11:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1554498

Comment 2 Hugo Cisneiros 2009-07-29 00:29:19 UTC
New build, with minor fixes:

Spec URL: http://www.devin.com.br/fedora-devel/SPECS/courier-authlib.spec
SRPM URL:
http://www.devin.com.br/fedora-devel/courier-authlib-0.62.4-2.fc11.src.rpm

Description: 
The Courier authentication library provides authentication services for
other Courier applications.

Comment 3 Hugo Cisneiros 2009-07-30 21:38:01 UTC
I know this isn't completely right, but I did some review prior to taking the package. I hope someone sees this and get to approve :)

MUST Items:
- rpmlint must be run on every package. OK
courier-authlib.i586: W: conffile-without-noreplace-flag /etc/authlib/authdaemonrc.dist - Acceptable
courier-authlib.i586: W: non-standard-uid /var/spool/authdaemon courier - Acceptable                 
courier-authlib.i586: W: non-standard-gid /var/spool/authdaemon courier - Acceptable
courier-authlib.i586: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/courier-authlib - Acceptable
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

- Package named according to the Package Naming Guidelines. - OK
- The spec file name must match the base package %{name} - OK
- The package must meet the Packaging Guidelines. - OK
- The package must be licensed with a Fedora approved license and meet the Licensing Guidelines - OK
- The License field in the package spec file must match the actual license. - OK
- Package contains the text of license in its own file - OK (COPYING.GPL)
- The spec file must be written in American English. - OK
- The spec file for the package MUST be legible. - OK
- The sources used to build the package must match the upstream source, as provided in the spec URL. - OK
- The package MUST successfully compile and build into binary rpms on at least one primary architecture. - OK (All Archs on Scratch Koji)
- All build dependencies must be listed in BuildRequires - OK
- The spec file MUST handle locales properly. - OK (Not Needed)
- Every binary RPM package with shared library files must call ldconfig in %post and %postun. - OK
- A package must own all directories that it creates. - OK
- A Fedora package must not list a file more than once in the spec file's %files listings. - OK
- Permissions on files must be set properly. - OK
- Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). - OK
- Each package must consistently use macros. - OK
- The package must contain code, or permissable content. - OK
- Large documentation files must go in a -doc subpackage. - OK (Not Needed)
- If a package includes something as %doc, it must not affect the runtime of the application. - OK
- Header files must be in a -devel package. - OK
- Static libraries must be in a -static package. - OK (Not needed)
- Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). - OK (Not Needed)
- Packages with .so files without suffix go to -devel package - OK (Not needed)
- devel packages must require the base package using a fully versioned dependency - OK
- Packages must NOT contain any .la libtool archives. - OK (Not needed)
- Packages containing GUI applications must include a %{name}.desktop file - OK (Not needed)
- Packages must not own files or directories already owned by other packages. - OK
- At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). - OK
- All filenames in rpm packages must be valid UTF-8. - OK

SHOULD items:

- If license file not included, query upstream for it. - OK (Not needed)
- Description and Summary translations in the spec - OK (Not needed)
- The reviewer should test that the package builds in mock. - OK (All Archs on Scratch Koji)
- The package should compile and build into binary rpms on all supported architectures.- OK (All Archs on Scratch Koji)
- The reviewer should test that the package functions as described. - OK
- If scriptlets are used, those scriptlets must be sane. - OK
- Usually, subpackages other than devel should require the base package using a fully versioned dependency. - OK
- The placement of pkgconfig(.pc) files in -devel pkg - OK (Not needed)

Comment 4 Itamar Reis Peixoto 2009-07-30 21:50:44 UTC
I will review your package, do you like to take courier-imap too ?

https://bugzilla.redhat.com/show_bug.cgi?id=514105

Comment 5 Hugo Cisneiros 2009-07-31 01:41:22 UTC
Yes I can take it, but first I need this :)

Thanks.

Comment 6 Rafael Gomes 2009-08-17 00:33:05 UTC
any news?

Comment 7 Rafael Gomes 2009-09-06 14:20:08 UTC
Please don't forget this bug!

Comment 8 Jason Tibbitts 2009-09-11 15:39:27 UTC
*** Bug 208064 has been marked as a duplicate of this bug. ***

Comment 9 Toshio Ernie Kuratomi 2009-09-15 06:50:54 UTC
Not quite a complete review.  I still have some rpmlint information to verify.  But there's some stuff to fix or discuss so I might as well post what I have:

Good:                                                                           
* Builds in koji                                                                
* named according to the naming guidelines                                      
* licensed according to the GPLv3+ as specified in the spec file                
* license file included.                                                        
* spec file is legible                                                          
* source matches upstream                                                       
* No file listed twice                                                          
* Permissions set properly                                                      
* Cleans the buildroot                                                          
* Uses macros consistently                                                      
* Headers in -devel package                                                     
* No static libraries or libtool archives                                       
* Devel package Requires the base package                                       


NEEDSWORK:
* It looks like all the libauth* files in the %{_libdir}/%{name}/ directory
  are plugins, not shared libs.  No need to run ldconfig for the subpackages
  that just contain those types of shared objects.                          
* There's no libraries in %{_libdir}.  RPATH is being used to load the needed 
  libraries from %{_libdir}/%{name}/ (libcourierauth.so,                    
  libcourierauthcommon.so).  The Packaging Guidelines specify that this should
  be done with a file in %{_sysconfdir}/ld.so.conf.d/ instead:                
    http://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath        
  
  I also wonder why we don't just put libcourier* directly into %{_libdir}?                                                                 
* Nothing owns %{_libdir}/%{name} or %{_libexecdir}/%{name} or                
  %{_sysconfdir}/authlib I suggest the base package does this.                

* rpmlint 
courier-authlib.i586: W: conffile-without-noreplace-flag /etc/authlib/authdaemonrc.dist                                                                         
courier-authlib-ldap.i586: W: conffile-without-noreplace-flag /etc/authlib/authldaprc.dist
courier-authlib-mysql.i586: W: conffile-without-noreplace-flag /etc/authlib/authmysqlrc.dist
courier-authlib-pgsql.i586: W: conffile-without-noreplace-flag /etc/authlib/authpgsqlrc.dist

 - It looks like all of these *rc.dist files are just vanilla versions of the
   *rc config files.  Why do we distribute these at all?

[Some more rpmlint messages will be posted in a followup]

Comment 10 Rafael Gomes 2009-11-09 02:20:29 UTC
Any news?

Comment 11 Rex Dieter 2010-04-09 15:05:57 UTC
toshio?  I was about to review courier-imap, but it's blocking on this one. (I see you're assigned, but the fedora-review flag isn't set yet too?)

Comment 12 Toshio Ernie Kuratomi 2010-04-09 17:40:54 UTC
Hmm... I think I forgot that there was more rpmlint stuff and was waiting for itamar to update the package.  I'm happy if you take over Rex.  Itamar, is there a new package that fixes the previous comments?

Comment 14 Rex Dieter 2010-04-26 14:56:24 UTC
I can continue the review here.

Comment 15 Rex Dieter 2010-04-28 13:51:14 UTC
So, who's the submitter here?

Aldrey or Itamar? both? Are you going to co-maintain this?

Comment 16 Itamar Reis Peixoto 2010-04-28 13:54:46 UTC
(In reply to comment #15)
Aldrey, are you willing to continue ?

Comment 17 Aldrey Galindo 2010-04-28 18:07:09 UTC
Yes, so I made the corrections that needed to be seen.

Comment 18 Rafael Gomes 2010-05-05 03:39:12 UTC
Ping! Any news? This package is very important for a Brazilian project, please help us.

Thanks!

Comment 19 Giandomenico De Tullio 2011-04-10 07:58:24 UTC
"Time passes and eternity approaches", 

Any News!?

Comment 20 Rex Dieter 2011-09-02 14:22:16 UTC
OK, I'll pick the review up again (sorry for the terrible delay).

Comment 21 Rex Dieter 2011-09-02 14:41:24 UTC
$ rpmlint x86_64/*.rpm
courier-authlib.x86_64: W: conffile-without-noreplace-flag /etc/authlib/authdaemonrc.dist
courier-authlib.x86_64: W: non-conffile-in-etc /etc/ld.so.conf.d/courier-authlib.conf
courier-authlib.x86_64: W: non-standard-uid /var/spool/authdaemon courier
courier-authlib.x86_64: W: non-standard-gid /var/spool/authdaemon courier
courier-authlib.x86_64: W: no-manual-page-for-binary userdb-test-cram-md5
courier-authlib.x86_64: W: no-manual-page-for-binary pw2userdb
courier-authlib.x86_64: W: no-manual-page-for-binary authdaemond
courier-authlib.x86_64: W: no-manual-page-for-binary authenumerate
courier-authlib.x86_64: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/courier-authlib
courier-authlib-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/courier-authlib-0.63.0/authldaplib.c
courier-authlib-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/courier-authlib-0.63.0/.libs
courier-authlib-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/courier-authlib-0.63.0/.libs
courier-authlib-devel.x86_64: W: spelling-error %description -l en_US runtime -> run time, run-time, runtish
courier-authlib-devel.x86_64: W: no-manual-page-for-binary courierauthconfig
courier-authlib-ldap.x86_64: W: conffile-without-noreplace-flag /etc/authlib/authldaprc.dist
courier-authlib-mysql.x86_64: W: conffile-without-noreplace-flag /etc/authlib/authmysqlrc.dist
courier-authlib-pgsql.x86_64: W: conffile-without-noreplace-flag /etc/authlib/authpgsqlrc.dist
courier-authlib-pipe.x86_64: W: spelling-error %description -l en_US authpipe -> auth pipe, auth-pipe, authorship
courier-authlib-pipe.x86_64: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging
courier-authlib-pipe.x86_64: W: spelling-error %description -l en_US stdin -> stein, stain, stdio
courier-authlib-pipe.x86_64: W: spelling-error %description -l en_US stdout -> stout, std out, std-out
courier-authlib-pipe.x86_64: W: no-documentation
courier-authlib-userdb.x86_64: W: no-documentation
8 packages and 0 specfiles checked; 1 errors, 22 warnings.

These are all mostly harmless.


naming: ok

sources: ok
411a927d178f783a1e8fab9964ce0dd2  courier-authlib-0.63.0.tar.bz2

macros: NOT OK, MUST fix.  See:
http://fedoraproject.org/wiki/Packaging/Guidelines#Macros
in particular,
"Macro forms of system executables SHOULD NOT be used except when there is a need to allow the location of those executables to be configurable. For example, rm should be used in preference to %{__rm}, but %{__python} is acceptable."

init: SHOULD: update to include systemd unit file, instead of sysv init
http://fedoraproject.org/wiki/Packaging/Guidelines#Systemd

license: ok (though most sources only say "see COPYING", which is in general not ideal, should explicitly mention license/copyright)

file/dir ownership: ok

SHOULD: investigate why shlibs are in %_libdir/courier-authlib instead of %_libdir directly, so the ld.so.conf.d file could be removed


So, the only blocker I see so far, is the macro thing.

Comment 22 Rex Dieter 2011-09-02 14:45:05 UTC
MUST:  Oops, file/dir ownership still broken:

%{_libexecdir}/%{name}/
%{_libdir}/%{name}
are still unowned, I'd suggest changing

%{_libexecdir}/%{name}/*
to 
%{_libexecdir}/%{name}/

and add
%dir %{_libdir}/%{name}/
to main pkg.

Comment 23 Rex Dieter 2012-04-19 12:26:28 UTC
ping?  haven't seen any reponse from reporter in ... a while.

Comment 24 Rex Dieter 2012-04-19 12:29:14 UTC
meh, marking dead review, original reporter hasn't been heard from in years.

Comment 25 Zvi "Viz" Effron 2013-10-07 11:08:34 UTC
Spec URL: http://fedorapkgs.flippedperspective.com/SPECS/courier-authlib.spec
SRPM URL: http://fedorapkgs.flippedperspective.com/SRPMS/courier-authlib-0.66.0-1.fc19.src.rpm

Description: 

The Courier authentication library provides authentication services for
other Courier applications.

Fedora Account System Username: viz

Notes:

This is my first package, so I need a sponsor

I don't know whether reopening this bug or filing a new one is correct procedure, so I reopened this one.  If that's incorrect, we can close this one again and create a new one.

When I ran rpmlint I saw 6 errors.  One was an incorrect-fsf-address which I've already reported to upstream.  The other 5 are non-readable /etc/authlib/auth*rc 0660L.  I believe there are security reasons for not making these files world readable, especially for authmysqlrc, authpgsqlrc, and authldaprc, as these files contain passwords.

I intend to also pick up the courier-imap package (https://bugzilla.redhat.com/show_bug.cgi?id=514105) but figured I'd do courier-authlib first, as courier-authlib is a dependency for courier-imap.

Koji f19 scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6030607
Koji f20 scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6030575

Comment 26 Rex Dieter 2013-10-07 14:42:13 UTC
> I don't know whether reopening this bug or filing a new one is 
> correct procedure, so I reopened this one

better to open a new one, then the bz submitter is properly attributed to be you

Comment 27 Zvi "Viz" Effron 2013-10-07 17:58:56 UTC
New bug filed: https://bugzilla.redhat.com/show_bug.cgi?id=1016221

Comment 28 Christopher Meng 2013-10-07 22:54:56 UTC

*** This bug has been marked as a duplicate of bug 1016221 ***

Comment 29 Rex Dieter 2017-01-27 15:37:04 UTC
clearing review flag


Note You need to log in before you can comment on or make changes to this bug.