Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Review Request: postgresql_autodoc - PostgreSQL AutoDoc Utility https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200630 ------- Additional Comments From devrim@xxxxxxxxxxxxxxxxx 2006-08-19 16:25 EST ------- 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. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA contact for the bug, or are watching the QA contact. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review