Bug 612174 - Review Request: eurephia - An advanced and flexible OpenVPN user authentication plug-in
Summary: Review Request: eurephia - An advanced and flexible OpenVPN user authenticati...
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
(Show other bugs)
Version: rawhide
Hardware: All Linux
medium
medium
Target Milestone: ---
Assignee: Mattias Ellert
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-07-07 13:42 UTC by David Sommerseth
Modified: 2016-05-22 23:30 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-11-11 14:24:38 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mattias.ellert: fedora-review+
limburgher: fedora-cvs+


Attachments (Terms of Use)

Description David Sommerseth 2010-07-07 13:42:07 UTC
Spec URL: http://people.redhat.com/dsommers/eurephia/eurephia.spec
SRPM URL: http://people.redhat.com/dsommers/eurephia/eurephia-1.0.0-1.fc12.src.rpm

Description: 
This plug-in enhances OpenVPN by adding user name and password
authentication in addition. An eurephia user account is a combination of
minimum one OpenVPN SSL certificate and a user name with a password
assigned. It is also possible to setup several eurephia user names to use
a shared OpenVPN certificate.

Remarks:
The following errors and warnings are reported by rpmlint, which I consider "false positives".  I'll give my arguments for that here.

* eurephia.spec:75: E: use-of-RPM_SOURCE_DIR
One OpenVPN specific file (openvpn-plugin.h) needs to be added extra.  I've just included that file as a plain text file and does a 'cp' into the source tree.  I'm using cp  %{_sourcedir}/openvpn-plugin.h to grab the file.  If there is a better way how to do this, I'm willing to move to that solution.  In the future I hope this requirement will go away, as I've asked the OpenVPN package maintainer to include openvpn-plugin.h in the openvpn package.

* eurephia.spec:79: W: configure-without-libdir-spec
The ./configure script is not an autotools script, just a wrapper script around cmake.  Thus, --libdir is not available.

* eurephia.src: W: spelling-error %description -l en_US iptables -> potables, portables, birdtables
iptables is a proper name, but not acknowledged by the spell check.

* eurephia-utils.x86_64: W: spelling-error %description -l en_US saltdecode -> salt decode, salt-decode, saltigrade
This package contains a binary named 'eurephia_saltdecode'.  This binary same is mentioned in the %description section.

Comment 1 David Sommerseth 2010-07-08 11:28:46 UTC
Fixed some odd building behaviour on F-13 and F-14 (sem_*() functions not found in librt) and added missing build dependency (openssl-devel).  Did successful builds on F-12, F-13, F-14.

SPEC URL: http://people.redhat.com/dsommers/eurephia/eurephia.spec
SRPM URL: http://people.redhat.com/dsommers/eurephia/eurephia-1.0.0-3.fc12.src.rpm

* Thu Jul  8 2010 David Sommerseth <davids@redhat.com> - 1.0.0-3
- Added patch to fix building in Koji/Fedora
- Added missing Group tags
- Added stricter cmake version requirement

* Thu Jul  8 2010 David Sommerseth <davids@redhat.com> - 1.0.0-2
- Added missing build dependency for openssl-devel

Comment 2 Mattias Ellert 2010-07-15 10:38:24 UTC
(In reply to comment #0)
> * eurephia.spec:75: E: use-of-RPM_SOURCE_DIR
> One OpenVPN specific file (openvpn-plugin.h) needs to be added extra.  I've
> just included that file as a plain text file and does a 'cp' into the source
> tree.  I'm using cp  %{_sourcedir}/openvpn-plugin.h to grab the file.  If there
> is a better way how to do this, I'm willing to move to that solution.  In the
> future I hope this requirement will go away, as I've asked the OpenVPN package
> maintainer to include openvpn-plugin.h in the openvpn package.

cp %{SOURCE1} .


One more comment: Your %setup unpacks the sources twice:

+ cd /home/ellert/rpmbuild/BUILD
+ rm -rf eurephia-1.0.0
+ /usr/bin/bzip2 -dc /home/ellert/rpmbuild/SOURCES/eurephia-1.0.0.tar.bz2
+ /bin/tar -xf -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ /usr/bin/bzip2 -dc /home/ellert/rpmbuild/SOURCES/eurephia-1.0.0.tar.bz2
+ /bin/tar -xf -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd eurephia-1.0.0

If you are using the -b (or -a) flag to %setup you should also use the -T flag to prevent the default setup invocation.

But -b0 is the default so instead of adding -T you could instead simply drop the -b0.


%prep 
%setup -q -n %{name}-%{version}%{?betatag:_%{betatag}}
%patch0 -p1
# This is needed to ship extra until openvpn-plugin.h becomes part of the OpenVPN RPM
cp %{SOURCE1} .

Comment 3 David Sommerseth 2010-07-15 11:48:49 UTC
Spec URL: http://people.redhat.com/dsommers/eurephia/eurephia.spec
SRPM URL:
http://people.redhat.com/dsommers/eurephia/eurephia-1.0.0-4.fc12.src.rpm

Thanks a lot for your review, Mattias.  I obviously failed to spot that one, sorry about that.  Your suggestion is implemented and it works as expected.  New src.rpm and spec file uploaded.

Comment 4 Mattias Ellert 2010-07-15 13:13:11 UTC
Fedora Review eurephia 2010-07-15

rpmlint output:

$ rpmlint eurephia-1.0.0-4.fc12.src.rpm eurephia-*1.0.0-4.fc12.x86_64.rpm 
eurephia.src: W: spelling-error %description -l en_US iptables -> potables, portables, birdtables
eurephia.src:91: W: configure-without-libdir-spec
eurephia.x86_64: W: spelling-error %description -l en_US iptables -> potables, portables, birdtables
eurephia-utils.x86_64: W: spelling-error %description -l en_US saltdecode -> salt decode, salt-decode, saltigrade
8 packages and 0 specfiles checked; 0 errors, 4 warnings.

(no fixes needed for those)


+ Package is named according to guidelines
+ Specfile is named after the package
+ Package license tag (GPLv2) is a Fedora approved license
+ The stated license matches the license statements in the sources
+ The license file (LICENSE.txt) is included as %doc
+ Specfile written in legible English
+ Source matches upstream:

$ md5sum eurephia-1.0.0.tar.bz2 srpm/eurephia-1.0.0.tar.bz2 
395040dd170e156a8f0e6d3150e0ea1e  eurephia-1.0.0.tar.bz2
395040dd170e156a8f0e6d3150e0ea1e  srpm/eurephia-1.0.0.tar.bz2

+ Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2321726
+ BuildRequires are sane
+ No locales
+ No libraries in default library path
+ No bundled libraries

- Orphaned directories not owned by any package:
  /usr/lib64/eurephia
  /usr/share/eurephia
  /usr/share/eurephia/xslt

+ No duplicate files
+ Permissions are sane and %files have %defattr

+ Specfile uses macros consistently (though the %{name} macro could be
  used more often

+ Contains code
+ %doc is not runtime essential
+ No header files
+ No static libraries

- Intra-package dependencies do not use fully qualified versions

+ No libtool archives
+ Package does not own other's directories
+ Installed files have valid UTF8 filenames

(unbalanced parentheses in the last changelog entry)

Comment 5 David Sommerseth 2010-07-15 14:06:08 UTC
Spec URL: http://people.redhat.com/dsommers/eurephia/eurephia.spec
SRPM URL:
http://people.redhat.com/dsommers/eurephia/eurephia-1.0.0-5.fc12.src.rpm

Thanks again, Mattias!  I've fixed the negative remarks and balanced the
changelog entry.

Comment 6 David Sommerseth 2010-07-15 14:11:53 UTC
Sorry!  I was too quick ... Something went wrong with the directory ownership.

Comment 7 David Sommerseth 2010-07-15 14:23:52 UTC
Spec URL: http://people.redhat.com/dsommers/eurephia/eurephia.spec
SRPM URL: http://people.redhat.com/dsommers/eurephia/eurephia-1.0.0-6.fc12.src.rpm

Using %dir properly now on the orphaned directories.

Comment 8 Mattias Ellert 2010-07-15 14:54:22 UTC
Don't hardcode the version number in the intra-package Requires - use macros. It will be so much simpler to update to newer versions that way. And a "fully qualified version" is %{version}-%{release} not only %{version}.

The eurephia-iptables package still has "Requires: eurephia" without version

The %{_libdir}/eurephia directory should be in the eurephia-sqlite3 package and not in the main eurephia package, since eurephia depends on eurephia-sqlite3 but not the other way.

The %{_datadir}/eurephia directory should be in the eurephia-admin package and not in the main eurephia package. The main package does not install files there, but the eurephia-admin does and the eurephia-admin has no dependency on the main package.

Comment 9 David Sommerseth 2010-07-15 15:12:21 UTC
Spec URL: http://people.redhat.com/dsommers/eurephia/eurephia.spec
SRPM URL: http://people.redhat.com/dsommers/eurephia/eurephia-1.0.0-7.fc12.src.rpm

Thanks again!

(In reply to comment #8)
> Don't hardcode the version number in the intra-package Requires - use macros.
> It will be so much simpler to update to newer versions that way. And a "fully
> qualified version" is %{version}-%{release} not only %{version}.

Ahh, I didn't know this.  Fixed!

> The eurephia-iptables package still has "Requires: eurephia" without version

Fixed!

> The %{_libdir}/eurephia directory should be in the eurephia-sqlite3 package and
> not in the main eurephia package, since eurephia depends on eurephia-sqlite3
> but not the other way.
> 
> The %{_datadir}/eurephia directory should be in the eurephia-admin package and
> not in the main eurephia package. The main package does not install files
> there, but the eurephia-admin does and the eurephia-admin has no dependency on
> the main package.    

Yeah, I actually expected you to say this.  I've changed it according to your requirements, and I see I will need to rethink this a little bit later on.

More database drivers will come in the future and  they will all go into %{_libdir}/eurephia.  A web-based admin utility is being planned, which will use %{_datadir}/eurephia/ as well.  So that's the reason for my way of thinking.

But when not looking into the future now, I do agree to your comments.

Comment 10 Mattias Ellert 2010-07-15 15:40:22 UTC
(In reply to comment #9)
> The %{_libdir}/eurephia directory should be in the eurephia-sqlite3 package and
> not in the main eurephia package, since eurephia depends on eurephia-sqlite3
> but not the other way.

This is still not resolved. You can still install only the eurephia-sqlite3 package - in which case nothing owns %{_libdir}/eurephia.

> More database drivers will come in the future and  they will all go into
> %{_libdir}/eurephia.  A web-based admin utility is being planned, which will
> use %{_datadir}/eurephia/ as well.  So that's the reason for my way of
> thinking.
> 
> But when not looking into the future now, I do agree to your comments.    

It is of course possible to keep the directory in the main package, but then you need to add requires on the main package to packages that install in the directory.


You have a typo (two equals) here:

Requires: eurephia-sqlite3 = = %{version}-%{release}

Comment 11 Mattias Ellert 2010-07-15 16:05:34 UTC
One more thing ...

You write

make VERBOSE=1

and not

make VERBOSE=1 %{?_smp_mflags}

Any reason for this? If parallel build is broken you should comment on this in the spec file. If it can be built in parallel you should do that. See:

https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

Comment 12 David Sommerseth 2010-07-15 16:17:58 UTC
Spec URL: http://people.redhat.com/dsommers/eurephia/eurephia.spec
SRPM URL:
http://people.redhat.com/dsommers/eurephia/eurephia-1.0.0-8.fc12.src.rpm

Thank you for your patience!  I hope I got it right this time.

(In reply to comment #10)
> (In reply to comment #9)
> > The %{_libdir}/eurephia directory should be in the eurephia-sqlite3 package and
> > not in the main eurephia package, since eurephia depends on eurephia-sqlite3
> > but not the other way.
> 
> This is still not resolved. You can still install only the eurephia-sqlite3
> package - in which case nothing owns %{_libdir}/eurephia.

Gee ... is it possible!  Sorry about that, my eyes slipped on this one.

> > More database drivers will come in the future and  they will all go into
> > %{_libdir}/eurephia.  A web-based admin utility is being planned, which will
> > use %{_datadir}/eurephia/ as well.  So that's the reason for my way of
> > thinking.
> > 
> > But when not looking into the future now, I do agree to your comments.    
> 
> It is of course possible to keep the directory in the main package, but then
> you need to add requires on the main package to packages that install in the
> directory.

Yeah, as this is under development, I'll spend some time thinking about it to
get the dependencies right.  The dependencies between -admin and -sqlite3
package will also go away in the future.  So I don't think it makes sense to
solve this right now.  It depends on which features the first next release will
get.

> You have a typo (two equals) here:
> 
> Requires: eurephia-sqlite3 = = %{version}-%{release}    

Ouch!  Fixed!


Regarding not using %{?_smp_mflags}.  That is intentional.  I have seen a few builds fail when doing parallel build.  So I wanted to avoid this for now, to figure out why it fails.  The project is quite small, so the benefit of parallel building is minimal right now.  But it's on my radar to fix this.

Comment 13 Mattias Ellert 2010-07-15 16:43:47 UTC
Package approved.

Comment 14 David Sommerseth 2010-07-15 16:55:47 UTC
New Package CVS Request
=======================
Package Name: eurephia
Short Description: An advanced and flexible OpenVPN user authentication plug-in 
Owners: dsommers
Branches: F-12 F-13
InitialCC:

Comment 15 Kevin Fenzi 2010-07-16 17:39:21 UTC
CVS done (by process-cvs-requests.py).

Comment 16 Fedora Update System 2010-07-16 19:24:46 UTC
eurephia-1.0.0-8.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/eurephia-1.0.0-8.fc12

Comment 17 Fedora Update System 2010-07-16 19:24:53 UTC
eurephia-1.0.0-10.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/eurephia-1.0.0-10.fc13

Comment 18 Mattias Ellert 2010-11-11 14:24:38 UTC
The updates above were pushed to stable long ago, but somehow the review request was left open... Closing.

Comment 19 David Sommerseth 2012-10-09 00:59:42 UTC
Package Change Request
======================
Package Name: eurephia
New Branches: el5 el6
Owners: dsommers

Comment 20 Gwyn Ciesla 2012-10-09 10:48:32 UTC
Git done (by process-git-requests).


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