Bug 200630 - Review Request: postgresql_autodoc - PostgreSQL AutoDoc Utility
Summary: Review Request: postgresql_autodoc - PostgreSQL AutoDoc Utility
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Toshio Kuratomi
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2006-07-29 10:38 UTC by Devrim GUNDUZ
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-07-05 18:59:03 UTC
Type: ---
Embargoed:
toshio: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)
Patch to add prompt for password (2.46 KB, patch)
2006-12-28 22:26 UTC, Toshio Kuratomi
no flags Details | Diff

Description Devrim GUNDUZ 2006-07-29 10:38:29 UTC
Spec URL: http://developer.postgresql.org/~devrim/rpms/other/postgresql_autodoc/postgresql_autodoc.spec
SRPM URL: http://developer.postgresql.org/~devrim/rpms/other/postgresql_autodoc/postgresql_autodoc-1.25-2.src.rpm
Description: This is a utility which will run through PostgreSQL system
tables and returns HTML, Dot, Dia and DocBook XML which
describes the database.

Comment 1 Devrim GUNDUZ 2006-07-29 10:42:21 UTC
Please ignore the rpmlint warning about the missing documentation; because there
is no document inside tarball, even the license, todo, install, etc.

Comment 2 Toshio Kuratomi 2006-08-05 20:38:00 UTC
Please don't guess what the license of a package is.  Falsely representing the
license may expose you to legal liability.  It will almost certainly make the
package's author upset.  If there is no license on the upstream distribution,
then the package is not free (either free as in freedom or as in beer) but is
instead a  fully copyrighted work which you have no license to distribute or
modify.  If you think this is not what the author intended then you have to ask
the author to clarify the licensing terms.  The best way they can do that (from
a packager's perspective) is by putting a license file into the package.

In this case, the upstream author has enclosed license terms.  Look inside the
source code for it.

Comment 3 Devrim GUNDUZ 2006-08-08 12:59:23 UTC
Comment from original author of postgresql_autodoc:

"I would like to know what text in the license made you, or Red Hat's
lawyers, think it was restricted so that I can change it suitably."

So?

Comment 4 Toshio Kuratomi 2006-08-08 18:41:06 UTC
I'm confused.  Does the original author think the license is GPL?  If so, the
license at the top of postgresql_autodoc.pl needs to be changed to GPL.

If the author is asking what is "restricted" about his license, then the answer
is _nothing_.  (Well, it's original BSD instead of new BSD.  But that just makes
it incompatible with the GPL; it's still open source.)

The problem I'm mentioning in Comment #2 is that the License in your spec file
says GPL which is contrary to the license in the source.  So you are
misrepresenting the license that the author has chosen for his work.

Comment 6 Toshio Kuratomi 2006-08-19 19:54:55 UTC
MD5Sums:
ef45084bb065b11def33ef7885ee694b  postgresql_autodoc-1.25-3.src.rpm
f61071a23f6b34f948bbf799de91e8e4  postgresql_autodoc-1.25.tar.gz
72f9c48a19b9a0d2999f1274e0e4398d  postgresql_autodoc.spec

Blockers:
* Package installs to %{_datadir}/pgsql but it neither owns the directory nor
  depends on anything which requires it.  This may be a bug in the postgresql
  packaging.  Care to ask Tom Lane if the postgresql package rather than
  postgresql-server should own %{_datadir}/pgsql?  I don't know the purpose of
  the directories well enough to judge.
* Package does not own %{_datadir}/pgsql/postgresql_autodoc

Cosmetic:
* Perl packages typically have virtual provides detailing what perl
  dependencies they are providing.  The prefered way to Require perl modules
  is through this virtual provide method.  So instead of
    BuildRequires: perl-HTML-Template
    BuildRequires: perl-Pg-DBD
  you want to have:
    BuildRequire: perl(DBD::Pg)
    BuildRequires: perl(HTML::Template)
* Using %{?dist} in the release makes upgrades across Fedora Core releases
  work more or less seamlessly.  Consider adding %{?dist} to the end of your
  Release: tag.
* The package does not come with a detached license file.  You should ask
  upstream to include one next time they release a tarball.  (Since the license
  is included as part of the source code and this is a script so it is in the
  installed package, this is not a blocker.  But it is convenient for end-users
  to have this.)
* When manually installing files in the spec file you should try to preserve
  the file timestamps.  This can be done with cp -p in your

Questions:
* If I'm reading the source correctly, this package will only work with
  postgresql, not other db's that use the perl DBI interface.  But the
  Requires picked up by rpm do not include perl(DBD::Pg).  Should there
  be a Requires: perl(DBD:Pg) in the spec?
* Running the program just errors for me.  Any clues?
    $ postgresql_autodoc  -d orchard --password='XXXXX'
    Can't call method "finish" on an undefined value at
/usr/bin/postgresql_autodoc line 1203.
* It appears that the only method for providing a password to use when
  connecting to the database is via the commandline.  This is insecure as it
  allows another user to see the password with something as simple as the
  "ps" command.  It would be a very good idea to ask upstream for other methods
  of sending the password: prompt, config file, etc.

Good:
* Source matches upstream
* Package follows naming guidelines. (The _ comes from the upstream project).
* spec file name matches the package name.
* The license is original BSD and matches in SOURCE and spec file.
* The spec file is readable.
* No locales so %find_lang is not present.
* Not a library package.
* Not relocatable.
* Does not contain duplicate files.
* Package has a proper %clean section.
* Code, not content.
* Macros are used consistently.
* No %doc affects the application runtime.
* Not a GUI application.
* Does not own directories or files owned by someone else.
* No scriptlets.
* Builds in mock.
* Permissions are okay.

Notes:
* rpmlint outputs:
    $ rpmlint postgresql_autodoc-1.25-3.*
    W: postgresql_autodoc no-documentation
  Which is ignorable because the package doesn't provide any documentation
  at this time.


Comment 7 Devrim GUNDUZ 2006-08-19 20:25:05 UTC
Hello,

> Blockers:
> * Package installs to %{_datadir}/pgsql but it neither owns the directory nor
>   depends on anything which requires it.  This may be a bug in the postgresql
>   packaging.  Care to ask Tom Lane if the postgresql package rather than
>   postgresql-server should own %{_datadir}/pgsql?  I don't know the purpose of
>   the directories well enough to judge.

After thinking a bit, I realized that it is my bad: I've changed the dependency.

> * Package does not own %{_datadir}/pgsql/postgresql_autodoc

Fixed.

> Cosmetic:
> * Perl packages typically have virtual provides detailing what perl
>   dependencies they are providing.  The prefered way to Require perl modules
>   is through this virtual provide method.  So instead of
>     BuildRequires: perl-HTML-Template
>     BuildRequires: perl-Pg-DBD
>   you want to have:
>     BuildRequire: perl(DBD::Pg)
>     BuildRequires: perl(HTML::Template)

Fixed.

> * Using %{?dist} in the release makes upgrades across Fedora Core releases
>   work more or less seamlessly.  Consider adding %{?dist} to the end of your
>   Release: tag.

Fixed.

> * The package does not come with a detached license file.  You should ask
>   upstream to include one next time they release a tarball.  (Since the 
> license
>   is included as part of the source code and this is a script so it is in the
>   installed package, this is not a blocker.  But it is convenient for
> end-users to have this.)

Ok, let's keep it for the future.

> * When manually installing files in the spec file you should try to preserve
>   the file timestamps.  This can be done with cp -p in your

Done.

> Questions:
> * If I'm reading the source correctly, this package will only work with
>   postgresql, not other db's that use the perl DBI interface.  But the
>   Requires picked up by rpm do not include perl(DBD::Pg).  Should there
>   be a Requires: perl(DBD:Pg) in the spec?

Yeah, fixed.

> * Running the program just errors for me.  Any clues?
>     $ postgresql_autodoc  -d orchard --password='XXXXX'
>     Can't call method "finish" on an undefined value at
> /usr/bin/postgresql_autodoc line 1203.

Works for me:

# postgresql_autodoc -d test -u postgres --password test  -l
/usr/share/pgsql/postgresql_autodoc/
        (in cleanup) dbd_st_destroy called twice! at /usr/bin/postgresql_autodoc
line 199.
DBI handle 0x9bad4c4 has uncleared implementors data at
/usr/bin/postgresql_autodoc line 199.
    dbih_clearcom (sth 0x9bad4c4, com 0x9e2ec38, imp DBD::Pg::st):
       FLAGS 0x100113: COMSET IMPSET Warn PrintError PrintWarn
       PARENT DBI::db=HASH(0x9baca20)
       KIDS 0 (0 Active)
       IMP_DATA undef
       NUM_OF_FIELDS -1
       NUM_OF_PARAMS 0
Producing test.dia from /usr/share/pgsql/postgresql_autodoc//dia.tmpl
Producing test.dot from /usr/share/pgsql/postgresql_autodoc//dot.tmpl
Producing test.html from /usr/share/pgsql/postgresql_autodoc//html.tmpl
Producing test.neato from /usr/share/pgsql/postgresql_autodoc//neato.tmpl
Producing test.xml from /usr/share/pgsql/postgresql_autodoc//xml.tmpl
Producing test.zigzag.dia from /usr/share/pgsql/postgresql_autodoc//zigzag.dia.tmpl

> * It appears that the only method for providing a password to use when
>   connecting to the database is via the commandline.  This is insecure as it
>   allows another user to see the password with something as simple as the
>   "ps" command.  It would be a very good idea to ask upstream for other methods
>   of sending the password: prompt, config file, etc.

AFAIK it can use environmental variable for the password stuff.

Thanks for the review. The new spec and srpm will be up shortly.

Comment 9 Toshio Kuratomi 2006-09-16 01:48:02 UTC
As communicated on IRC, this looks good except for having to specify the
password on the commandline.  Can you poke upstream to see if they can provide a
way to prompt for the password interactively?  That's really the most secure way
and since this isn't likely to be run from a cron job, it's probably the best
way to handle it for this program.

Comment 10 Devrim GUNDUZ 2006-12-06 06:20:43 UTC
(Hi Toshio,

In reply to comment #9)
> As communicated on IRC, this looks good except for having to specify the
> password on the commandline.  Can you poke upstream to see if they can provide a
> way to prompt for the password interactively?  

I will poke upstream, but if we call this as a "feature", not a bug, what else
is needed for approval?

Regards, Devrim


Comment 11 Toshio Kuratomi 2006-12-28 22:26:29 UTC
Created attachment 144501 [details]
Patch to add prompt for password

Comment 12 Toshio Kuratomi 2006-12-28 22:35:31 UTC
Previous patch is untested.  It uses Term::ReadKey to prompt for a password if
the user specifies --password on the commandline.

You'll need to add a BuildRequires for perl(Term::ReadKey).  Not sure if rpm
will pick up the matching Requires: automatically as I haven't tested this.

The spec file is now picking up duplicate files in the %files section:
  %{_datadir}/pgsql/%{name}
  %{_datadir}/pgsql/%{name}/*.tmpl

You should only have the first of those two lines as rpm will find files inside
the directory recursively.

I'd say the next step is to see if the patch I proposed works for you, see if
upstream is amenable to including it, and then release an rpm with it included.

Comment 13 Kevin Fenzi 2007-06-01 06:16:42 UTC
I assume this package is under review by Toshio? 
Setting the fedora-review flag here. If I am in error, please reset. 

Is there any progress on this package? 

Comment 14 Devrim GUNDUZ 2007-06-03 07:46:19 UTC
(I had forgotten to update this bug.)

Yes, there is a progress. The patch that Toshio wrote is in upstream now. So,
all issues seem to be solved:

New Spec:
http://developer.postgresql.org/~devrim/rpms/other/postgresql_autodoc/postgresql_autodoc.spec

New SRPM:
http://developer.postgresql.org/~devrim/rpms/other/postgresql_autodoc/postgresql_autodoc-1.30-1.src.rpm

Regards, Devrim

Comment 15 Toshio Kuratomi 2007-06-09 00:17:51 UTC
NEEDSWORK

Cosmetic:
* BuildRequires: perl-TermReadKey is better as:
    BuildRequires: perl(Term::ReadKey)
* Requires: perl-TermReadKey is not needed as rpm picks up the dependency
  automatically.

Blocker:
* The build is putting the wrong value in the program for the templates
  directory so postgresql_autodoc fails to work. Changing your configure line
  to:
    %configure --datadir=%{_datadir}/pgsql
  should make it happy.


Comment 16 Devrim GUNDUZ 2007-06-09 01:15:09 UTC
Hello,

(In reply to comment #15)
> NEEDSWORK
> 
> Cosmetic:
> * BuildRequires: perl-TermReadKey is better as:
>     BuildRequires: perl(Term::ReadKey)
> * Requires: perl-TermReadKey is not needed as rpm picks up the dependency
>   automatically.
> 
> Blocker:
> * The build is putting the wrong value in the program for the templates
>   directory so postgresql_autodoc fails to work. Changing your configure line
>   to:
>     %configure --datadir=%{_datadir}/pgsql
>   should make it happy.

Fixed:

New Spec:
http://developer.postgresql.org/~devrim/rpms/other/postgresql_autodoc/postgresql_autodoc.spec

New SRPM:
http://developer.postgresql.org/~devrim/rpms/other/postgresql_autodoc/postgresql_autodoc-1.30-2.src.rpm

Regards, Devrim 



Comment 17 Toshio Kuratomi 2007-06-09 01:39:52 UTC
MD5Sums:
064c22add40c29b9e265d9384dbb493e  postgresql_autodoc-1.30-2.src.rpm
37fbeb1ca25e27af90dad5898c97eb33  postgresql_autodoc.spec

APPROVED

Comment 18 Devrim GUNDUZ 2007-06-09 01:45:52 UTC
New Package CVS Request
=======================
Package Name: postgresql_autodoc
Short Description: PostgreSQL AutoDoc Utility
Owners: devrim
Branches: FC-6 F-7 EL 4 EL 5
InitialCC: devrim

Comment 19 Jason Tibbitts 2007-06-09 04:03:49 UTC
CVS done.

Comment 20 Ruben Kerkhof 2007-07-05 17:39:44 UTC
Devrim, can you close this one?

Comment 21 Devrim GUNDUZ 2007-07-05 18:59:03 UTC
Sorry, closing.


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