Bug 455187 - Review Request: erlang-pgsql - Erlang PostgreSQL interface
Review Request: erlang-pgsql - Erlang PostgreSQL interface
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-13 14:45 EDT by Robert Scheck
Modified: 2008-09-10 02:52 EDT (History)
6 users (show)

See Also:
Fixed In Version: 0-1.20080825svn
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-31 06:35:08 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
lemenkov: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Robert Scheck 2008-07-13 14:45:54 EDT
Spec URL: http://labs.linuxnetz.de/bugzilla/erlang-pgsql.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/erlang-pgsql-0.0.0-
0.20080709svn.src.rpm
Library that gives possibility to Erlang programs to connect PostgreSQL 
databases by plain tcp and execute simple SQL statements.

As there are only a few erlang related packages within Fedora currently, I'm
adding the people maintaining such packages there on Cc. Feel free to remove
yourself if you feel bothered by me.
Comment 1 Peter Lemenkov 2008-08-17 12:47:49 EDT
I'll review it.
Comment 2 Peter Lemenkov 2008-08-20 05:51:32 EDT
TODO items:

* Version field should be 0 instead of 0.0.0
* Instead of using "svn co" and later removing svn files, you should use "svn export"
* You should add verbatim commands used to produce tarball, not only one (svn co) - see this spec, for example:

http://cvs.fedoraproject.org/viewvc/rpms/flashrom/devel/flashrom.spec?view=markup

* There is rev. 691 in svn. Would you mind to update your package?

* Using only checkout date in versionin is insufficient at all. You should use svn revision (and date, if you wish) instead.

* Main package should contain doc/short-desc as well.

* To avoid building empty debuginfo-subpackage you should add 

%define debug_package %{nil}

at top of your spec-file. See this spec as an example:

http://peter.fedorapeople.org/nagios-plugins-check_sip.spec

* I'm in doubts of naming scheme for devel-subpackage. Actually we can use erlang modules in development w/o sources :). Maybe it would be better to name it src instead of devel? Just my thoughts, anyway...

* You should use -p switch for "install" command, in order to preserve timestamps. Frankly speaking in this case (checkout from VCS) there is not so much sense, but lately, when official tarball may be introduced, it would have more meaning.
Comment 3 Robert Scheck 2008-08-25 15:17:41 EDT
(In reply to comment #2)
> * Version field should be 0 instead of 0.0.0
> * Instead of using "svn co" and later removing svn files, you should use "svn
> export"
> * You should add verbatim commands used to produce tarball, not only one (svn
> co) - see this spec, for example:

Uh, it must have been late. Of course, I'll change this, thanks for pointing.

> * There is rev. 691 in svn. Would you mind to update your package?

I can do that, it's just a minor bugfix.

> * Using only checkout date in versionin is insufficient at all. You should use
> svn revision (and date, if you wish) instead.

Sorry, but the MUST is the other way round DATEsvn is a must, while your DATEsvnREVISION suggestion is optional. So I will stay with the current one.
Keep in mind, that the upstream development is more than slow, I can cound
the commits over the last year on one hand or so; see also:

[20:59:35] < rsc> hrm. My reviewer dislikes the skipping of the revision in %release, so I'm currently only doing DATEsvn rather DATEsvnREV as he expects.
[21:00:04] < tibbs> Your reviewer is entitled to their opinion, I suppose.
[21:00:49] < tibbs> But the guidelines just say "followed by up to 16 (ASCII) alphanumeric characters of your choosing".
[21:01:08] < tibbs> If you choose to put nothing or just "svn", that is quite up to you.
[21:02:33] < tibbs> Our intent was not to legislate anything beyond the presence of the date.

> * Main package should contain doc/short-desc as well.

The content of doc/short-desc seems useless to me. As far as I read, HOWTO file
contains this content as well, so I didn't add the file and will keep this.

> * To avoid building empty debuginfo-subpackage you should add 
> %define debug_package %{nil}
> at top of your spec-file. See this spec as an example:

Accepted.

> * I'm in doubts of naming scheme for devel-subpackage. Actually we can use
> erlang modules in development w/o sources :). Maybe it would be better to name
> it src instead of devel? Just my thoughts, anyway...

I'm currently only using what the current packages are doing. AFAIK there is
no scheme for erlang until now. If I'm switching, the other packages also have
to do so before.

> * You should use -p switch for "install" command, in order to preserve
> timestamps. Frankly speaking in this case (checkout from VCS) there is not so
> much sense, but lately, when official tarball may be introduced, it would have
> more meaning.

I won't use -p once we've something official, currently seems useless to me.

Are you able to life with the "no"s I've stated above? Please give me a short
note if so. As you maybe understand, I'm not happy with flipping some bits or
lines all the time during a review while it is still discussed or similar. Of
course this only referes to the current points.
Comment 4 Peter Lemenkov 2008-08-25 16:06:56 EDT
(In reply to comment #3)

> > * Using only checkout date in versionin is insufficient at all. You should use
> > svn revision (and date, if you wish) instead.
> 
> Sorry, but the MUST is the other way round DATEsvn is a must, while your
> DATEsvnREVISION suggestion is optional. So I will stay with the current one.
> Keep in mind, that the upstream development is more than slow, I can cound
> the commits over the last year on one hand or so; see also:

Ok. 

> > * Main package should contain doc/short-desc as well.
> 
> The content of doc/short-desc seems useless to me. As far as I read, HOWTO file
> contains this content as well, so I didn't add the file and will keep this.

Likewise.


> > * I'm in doubts of naming scheme for devel-subpackage. Actually we can use
> > erlang modules in development w/o sources :). Maybe it would be better to name
> > it src instead of devel? Just my thoughts, anyway...
> 
> I'm currently only using what the current packages are doing. AFAIK there is
> no scheme for erlang until now. If I'm switching, the other packages also have
> to do so before.

I think we should, probably, discuss it via fedora-devel maillist. There are more libraries for interpreted languages coming, there similar issues would arise.

> > * You should use -p switch for "install" command, in order to preserve
> > timestamps. Frankly speaking in this case (checkout from VCS) there is not so
> > much sense, but lately, when official tarball may be introduced, it would have
> > more meaning.
> 
> I won't use -p once we've something official, currently seems useless to me.

Ok.

> As you maybe understand, I'm not happy with flipping some bits or
> lines all the time during a review while it is still discussed or similar. Of
> course this only referes to the current points.

Erlang packaging guidelines are still missing. Are you interested in creating ones? There is a dedicated SIG for erlang:

http://fedoraproject.org/wiki/Erlang

but still no guidelines.
Comment 5 Robert Scheck 2008-08-26 16:05:55 EDT
(In reply to comment #4)
> I think we should, probably, discuss it via fedora-devel maillist. There are
> more libraries for interpreted languages coming, there similar issues would
> arise.

I don't care whether -devel or -src or whatelse, but we should be consistent
for all packages. I'll rename to whatever gets defined or discussed, I'm lacking
a bit time to drive such a discussion the next time.

> Erlang packaging guidelines are still missing. Are you interested in creating
> ones? There is a dedicated SIG for erlang:
> 
> http://fedoraproject.org/wiki/Erlang
> 
> but still no guidelines.

Sorry, I just need PostgreSQL-Support for ejabberd at work - not more and not
less, so I'm maintaining the package, but I'm clueless regarding Erlang and all
what has to do with.

Updated files are here now (should include everything from yesterday):
Spec URL: http://labs.linuxnetz.de/bugzilla/erlang-pgsql.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/erlang-pgsql-0-1.20080825svn.src.rpm
Comment 6 Peter Lemenkov 2008-08-29 08:09:30 EDT
REVIEW:

+/- rpmlint is not silent:

[petro@Sulaco SPECS]$ rpmlint ../RPMS/ppc/erlang-pgsql-*
erlang-pgsql.ppc: E: no-binary
erlang-pgsql.ppc: E: only-non-binary-in-usr-lib
erlang-pgsql-devel.ppc: W: no-documentation
erlang-pgsql-devel.ppc: E: only-non-binary-in-usr-lib
2 packages and 0 specfiles checked; 3 errors, 1 warnings.
[petro@Sulaco SPECS]$ 

However these messages should be safely ignored.

+ The package is named according to the Package Naming Guidelines .
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines .
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license.
+ File, containing the text of the license, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package must matches the upstream source

[petro@Sulaco SOURCES]$ svn export -r 691 https://svn.process-one.net/ejabberd-modules/pgsql/trunk erlang-pgsql-0.new
A    erlang-pgsql-0.new
A    erlang-pgsql-0.new/Emakefile
A    erlang-pgsql-0.new/build.bat
A    erlang-pgsql-0.new/doc
A    erlang-pgsql-0.new/doc/HOWTO
A    erlang-pgsql-0.new/doc/short-desc
A    erlang-pgsql-0.new/INSTALL
A    erlang-pgsql-0.new/src
A    erlang-pgsql-0.new/src/pgsql_util.erl
A    erlang-pgsql-0.new/src/pgsql.erl
A    erlang-pgsql-0.new/src/pgsql_proto.erl
A    erlang-pgsql-0.new/src/pgsql_tcp.erl
A    erlang-pgsql-0.new/EPLICENSE
A    erlang-pgsql-0.new/build.sh
A    erlang-pgsql-0.new/ebin
Exported revision 691.
[petro@Sulaco SOURCES]$ tar xfz erlang-pgsql-0.tar.gz 
[petro@Sulaco SOURCES]$ diff -ru erlang-pgsql-0 erlang-pgsql-0.new/
[petro@Sulaco SOURCES]$ 

+ The package successfully compiles and builds into binary rpms on at least one supported architecture (ppc).
+ All build dependencies are listed in BuildRequires.
- The package must own all directories that it creates. 
  You should add  %dir %{_libdir}/erlang/lib/pgsql-%{version}/{ebin,src} also

+ The package does not contain any duplicate files in the %files listing.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
+ The package consistently uses macros, as described in the macros section of Packaging Guidelines .
+ The package contains code, or permissable content.
+ Everything, the package includes as %doc, does not affect the runtime of the application.
-/+ Header files are in a -devel package. However they're not a header, actually. Not a blocker, anyway.
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT.
+ All filenames in rpm packages are valid UTF-8.

Consider adding missing directories to %dir (btw, huge amount of already packaged apps does not list every created directory as %dir, so seems that this is not a critical blocker) and this package is 

APPROVED.
Comment 7 Robert Scheck 2008-08-29 12:26:07 EDT
Peter, I'll check that the missing directories are owned before importing.
Thanks for your review.



New Package CVS Request
=======================
Package Name: erlang-pgsql
Short Description: Erlang PostgreSQL interface
Owners: robert
Branches: EL-4 EL-5 F-8 F-9
InitialCC: 
Cvsextras Commits: no
Comment 8 Kevin Fenzi 2008-08-30 16:49:05 EDT
cvs done.
Comment 9 Robert Scheck 2008-08-31 06:35:08 EDT
Thanks for CVS. Peter, I can't see any missing/orphaned directories, even
after installing the package and looking twice.


Package: erlang-pgsql-0-1.20080825svn.fc8 Tag: dist-f8-updates-candidate Status: complete
Package: erlang-pgsql-0-1.20080825svn.fc9 Tag: dist-f9-updates-candidate Status: complete
Package: erlang-pgsql-0-1.20080825svn.fc10 Tag: dist-f10 Status: complete
81 (erlang-pgsql): Build on target fedora-5-epel succeeded.
Comment 10 Fedora Update System 2008-08-31 06:35:54 EDT
erlang-pgsql-0-1.20080825svn.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/erlang-pgsql-0-1.20080825svn.fc9
Comment 11 Fedora Update System 2008-08-31 06:36:00 EDT
erlang-pgsql-0-1.20080825svn.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/erlang-pgsql-0-1.20080825svn.fc8
Comment 12 Fedora Update System 2008-08-31 07:18:13 EDT
erlang-pgsql-0-2.20080825svn.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/erlang-pgsql-0-2.20080825svn.fc8
Comment 13 Fedora Update System 2008-08-31 07:18:39 EDT
erlang-pgsql-0-2.20080825svn.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/erlang-pgsql-0-2.20080825svn.fc9
Comment 14 Fedora Update System 2008-09-10 02:52:00 EDT
erlang-pgsql-0-2.20080825svn.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 15 Fedora Update System 2008-09-10 02:52:17 EDT
erlang-pgsql-0-2.20080825svn.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

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