Bug 199682 - Review Request: postgresql-dbi-link - Partial implementation of the SQL/MED portion of the SQL:2003 specification
Review Request: postgresql-dbi-link - Partial implementation of the SQL/MED p...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-07-21 08:14 EDT by Devrim GUNDUZ
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-25 19:59:06 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
petersen: fedora‑cvs+


Attachments (Terms of Use)
patch to spec file (1.40 KB, patch)
2007-02-20 22:08 EST, Kevin Fenzi
no flags Details | Diff

  None (edit)
Description Devrim GUNDUZ 2006-07-21 08:14:40 EDT
Spec URL: http://developer.postgresql.org/~devrim/rpms/other/dbi-link/dbi-link.spec

SRPM URL: http://developer.postgresql.org/~devrim/rpms/other/dbi-link/dbi-link-1.0.0-2.src.rpm

Description: If you've ever wanted to join PostgreSQL tables from other data you
can access via Perl's DBI, this is your project.

You can add speed and accuracy to your ETL processes by treating
any data source you can reach with DBI as a  PostgreSQL table.
Comment 1 Kevin Fenzi 2007-01-12 23:43:55 EST
Your Spec link seems to be 404, but I can get the spec from your src.rpm. 

I'd be happy to review this. 
Expect a full review in a bit here. 
Comment 2 Kevin Fenzi 2007-01-13 00:01:25 EST
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
See below - License
See below- License field in spec matches
See below- License file included in package
OK - Spec in American English
OK - Spec is legible.
See below - Sources match upstream md5sum:
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
See below - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK (i386/x86_64) - Should build on all supported archs
See below - Should have subpackages require base package with fully versioned 
depend.
See below - Should have dist tag
OK - Should package latest version

Issues:

1. Not a blocker, but you should consider very strongly using
the %{?dist} tag.

2. You have the license as GPL in the spec, but the pgfoundry
page says "BSD". The included copyright.txt file appears to be the
orig BSD license with advertising clause.

3. Suggest you provide a full download url for the Source0 line.
In fact, now I can't find a download link for the 1.0.0 release.
Only the new 2.0beta1 seems to be available. Perhaps thats the
prefered version to package now?

4. This package should own:
%{_datadir}/pgsql/%{name}
directory.

5. rpmlint says:

W: dbi-link no-%build-section

You could add a empty %build to fix this. I seem to remember some
other problems with specs with no %build section, so you should
probibly add it anyhow. Shouldn't hurt.

6. Should change the Requires for the dbi-link-test subpackage from:
Requires:       %{name}
to
Requires:       %{name} = %{version}-%{release}
Comment 3 Devrim GUNDUZ 2007-01-13 18:47:51 EST
Hi,

Thanks for the review. I have a newer spec file already, for 2.0beta1. I applied
some fixes to it per this review. Per author, it will go stable next week. Until
then we can push this.

Here are the spec file and SRPM:

http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link.spec
http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link-2.0beta1-1.src.rpm

Regards, Devrim
Comment 4 Kevin Fenzi 2007-01-13 19:59:54 EST
ok, I see you changed the package name, and postgresql-dbi-link might make more 
sense... I am changing the summary of this review to match for tracking 
purposes. 

1. ok. dist tag now present. 
2. ok. license is correctly BSD. 
3. ok. The source link looks good now.
4. ok. No longer an issue with this version. 
5. ok. empty build section added. 
6. ok. subpackage now requires the version-release of the main package.  

New issue:

The naming doesn't seem to match what it should be for pre-release packages. 
See: 
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-
d97a3f40b6dd9d2288206ac9bd8f1bf9b791b22a

I think it should be: 
postgresql-dbi-link-2.0-0.1.beta1
Comment 5 Devrim GUNDUZ 2007-01-14 08:19:20 EST
Hi,

(In reply to comment #4)
> New issue:
> 
> The naming doesn't seem to match what it should be for pre-release packages. 
> See: 
> http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-
> d97a3f40b6dd9d2288206ac9bd8f1bf9b791b22a
> 
> I think it should be: 
> postgresql-dbi-link-2.0-0.1.beta1

Ok, but that would not match tarball name.

My proposal: Let's leave it as it is now. dbi-link will go stable in next week
or so, and we will remove beta tag then.

Regards, Devrim
 

Comment 6 Kevin Fenzi 2007-01-14 13:37:09 EST
In reply to comment #5: 

But your current 2.0beta1-1 package is rpm newer than 2.0-1 will be. 
Upgrades will not work. 

% rpmdev-vercmp 
Epoch1 :0
Version1 :2.0beta1
Release1 :1
Epoch2 :0
Version2 :2.0
Release2 :1
0:2.0beta1-1 is newer

This is why the nameing guidelines require the nameing they do for pre release 
packages. The current name doesn't meet the guidelines and won't work right 
for upgrades. 
Comment 7 Devrim GUNDUZ 2007-01-14 14:22:33 EST
Hi,

(In reply to comment #6)
<snip>
> The current name doesn't meet the guidelines and won't work right 
> for upgrades. 

Ok then. I updated the spec file:

http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link.spec

...and the SRPM is here:

http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link-2.0-0.1.beta1.src.rpm

Regards, Devrim
Comment 8 Kevin Fenzi 2007-01-14 23:59:44 EST
The package from comment #7 looks good. 

I don't see any further blockers, so this package is APPROVED. 

Don't forget to close this review request as NEXTRELEASE once it's been 
imported and built. 
Comment 9 Mamoru TASAKA 2007-01-16 11:13:26 EST
Well, as this package came up for discussion, I checked
this package.

* As commented in fedora-extras-list, please remove
  test/oracle (I confirmed that you did it).
* Why does this package have "perl => 5.8.5, postgresql-devel >= 8.0"
  for BuildRequires?
  From your spec file, all rpmbuild has to do is just "install"
  or "cp", so no other packages than mimimal buildroot environment
  are needed
* Files/directories entry
---------------------------------------
%files
%defattr(-,root,root,-)
%doc copyright.txt  IMPLEMENTATION.txt README.txt TODO.txt ROADMAP.txt
%{_datadir}/%{name}/

%files test
%defattr(-,root,root,-)
%doc README.txt
%{_datadir}/%{name}/test/csv
%{_datadir}/%{name}/test/mysql
%{_datadir}/%{name}/test/postgresql
------------------------------------------------
   Well, this is wrong......

   Writing as
------------------------------------------------
%{_datadir}/%{name}/
------------------------------------------------
   is interpretted as the directory %{_datadir}/%{name}/ and
   all files/directories under %{_datadir}/%{name}/.
   So, for example, all test files are also included in
   main package, too.

   Please fix the file/directory entry so that there are
   no duplicate entries.

   And... why is README.txt installed in both packages?
 
Comment 10 Mamoru TASAKA 2007-01-16 11:14:41 EST
Sorry, however I must reopen this bug.
Comment 11 Kevin Fenzi 2007-01-16 11:24:55 EST
Ugh. Sorry about missing those things. ;( 
It seems I didn't re-run my full checklist on the new 2.0beta1 package...
oracle and excel and such were not in the 1.0 package that was orig submitted. 

I'll make another run just to doublecheck... Can you fix all the issues in 
comment #9?
Comment 12 Kevin Fenzi 2007-01-16 11:42:44 EST
The only additional item I can see from re-running my full checklist on the 
version in CVS is that the 'perl-YAML' Requires is not needed. rpm picks up 
that dependency just fine.


Comment 13 Devrim GUNDUZ 2007-01-17 12:00:11 EST
(In reply to comment #12)
> The only additional item I can see from re-running my full checklist on the 
> version in CVS is that the 'perl-YAML' Requires is not needed. rpm picks up 
> that dependency just fine.

Done.

Regards, Devrim
Comment 14 Devrim GUNDUZ 2007-01-17 12:04:57 EST
Hi,

(In reply to comment #9)
> Well, as this package came up for discussion, I checked
> this package.
> 
> * As commented in fedora-extras-list, please remove
>   test/oracle (I confirmed that you did it).

Yes, done in 0.2.

> * Why does this package have "perl => 5.8.5, postgresql-devel >= 8.0"
>   for BuildRequires?
>   From your spec file, all rpmbuild has to do is just "install"
>   or "cp", so no other packages than mimimal buildroot environment
>   are needed

That's my bad -- Removed that in 0.3.

> * Files/directories entry
> ---------------------------------------
> %files
> %defattr(-,root,root,-)
> %doc copyright.txt  IMPLEMENTATION.txt README.txt TODO.txt ROADMAP.txt
> %{_datadir}/%{name}/
> 
> %files test
> %defattr(-,root,root,-)
> %doc README.txt
> %{_datadir}/%{name}/test/csv
> %{_datadir}/%{name}/test/mysql
> %{_datadir}/%{name}/test/postgresql
> ------------------------------------------------
>    Well, this is wrong......
> 
>    Writing as
> ------------------------------------------------
> %{_datadir}/%{name}/
> ------------------------------------------------
>    is interpretted as the directory %{_datadir}/%{name}/ and
>    all files/directories under %{_datadir}/%{name}/.
>    So, for example, all test files are also included in
>    main package, too.
> 
>    Please fix the file/directory entry so that there are
>    no duplicate entries.

Fixed in new version.

>    And... why is README.txt installed in both packages?

Yeah, ok. Removed.

Thanks for the review. I'll submit the new spec and SRPM shortly.

Regards, Devrim

Comment 16 Kevin Fenzi 2007-01-17 12:49:19 EST
Looking at the package in comment #15: 

- You probibly shouldn't have the versions in the Requires for postgresql-
server and perl. Since this package will be for fc5/fc6/devel, all of those 
releases have newer versions than listed in the requires. 

- You now need to own
%{_datadir}/%{name}/ 
in the main package, you can add a: 
%dir %{_datadir}/%{name}
in the files section of the main package to do this. 

- The test subpackage needs to own: 
%{_datadir}/%{name}/test 
directory as well. 
%dir %{_datadir}/%{name}/test
in files there should do that. 

I don't see anything else aside from those 3 items... 
Comment 17 Mamoru TASAKA 2007-01-17 14:15:57 EST
One more issue.

* Timestamps
------------------------------
install -m 644 *.sql %{buildroot}/%{_datadir}/%{name}/
------------------------------
Please use "install -p -m 644" to keep timestamps.
Comment 19 Devrim GUNDUZ 2007-01-22 02:32:34 EST
Removing this package from Extras, because the author did not want Oracle
portions to be removed.

So I'm closing this bug.

Regards, Devrim
Comment 20 David Fetter 2007-01-27 16:27:46 EST
I believe this fixes the earlier issues.

http://fetter.org/~shackle/postgresql-dbi-link.spec

http://fetter.org/~shackle/postgresql-dbi-link-2.0.0-1.src.rpm
Comment 21 Kevin Fenzi 2007-01-27 21:34:18 EST
Just to fill in some additional information here: 

- David is the upstream author of this package. (from comment #20). 
- The Oracle (and excel) dependencies were due to examples of how to use this
package, there is no real dependency on them in the package itself. 
- David is fine with Devrim maintaining the Fedora version of this package now. 
- The 2.0.0 version was released upstream, including moving those test scripts
to a 'examples' directory, and having them be shipped as doc files. 

That said, looking at the package in comment #20: 

1. You should use the %{?dist} tag: 
http://www.fedoraproject.org/wiki/Packaging/DistTag

2. You might change the 
%doc %{_datadir}/%{name}/examples
to just
%doc examples
Although I guess it's just a matter of taste where the example files are... 

I don't see anything else thats a blocker with the package in comment #20.

Comment 22 Mamoru TASAKA 2007-01-28 13:04:00 EST
Then reopen.

By the way, usually the dependency for perl module
should be written like "perl(DBI)" or so.
However, if it needs version-specific dependency,
I don't know how to write it.

(So if there is no way to write version dependency
 in perl(XXX) format, I leave the format as current
 spec file has)
Comment 23 Mamoru TASAKA 2007-01-28 13:05:11 EST
And again once back to FE-REVIEW
Comment 24 Kevin Fenzi 2007-01-28 14:07:00 EST
Thanks Mamoru. I meant to move it back to review, but forgot to do so. 

Devrim: Can you address the issues in comments #21 and #22?
Comment 25 Devrim GUNDUZ 2007-02-20 00:31:46 EST
Sorry for the delay -- I just forgot this. I just added the dist tag. The
comment 2 in #21 broke package build...
Here is the new spec:
http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link.spec
Here is the new SRPM:
http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link-2.0.0-2.src.rpm

Regards, Devrim 
Comment 26 Kevin Fenzi 2007-02-20 22:07:01 EST
in reply to comment #25 

You need to remove the cp statements that move the doc files in order for that
to work. I'll attach a diff so you can see what I mean. 

I think it's cleaner to have those examples there. 
Comment 27 Kevin Fenzi 2007-02-20 22:08:12 EST
Created attachment 148462 [details]
patch to spec file
Comment 28 Devrim GUNDUZ 2007-02-21 01:38:57 EST
Hi,

Thanks for the review.

Here is the new spec:
http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link.spec
Here is the new SRPM:
http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link-2.0.0-3.src.rpm

Regards, Devrim
Comment 29 Kevin Fenzi 2007-02-21 18:56:04 EST
ok, that version from comment #28 looks good to me, so I would APPROVE it (again). 

Mamoru: Could you look over the package in comment #28 and confirm that I
haven't missed anything?

Comment 30 Kevin Fenzi 2007-03-09 00:32:28 EST
Sorry for the delay here. 

I don't see anything blocking here now... so I will go ahead and APPROVE this
package (again). Don't forget to close this bug once it's been imported and built. 
Comment 31 Devrim GUNDUZ 2007-03-09 05:17:29 EST
New Package CVS Request
=======================
Package Name: postgresql-dbi-link
Short Description: Partial implementation of the SQL/MED portion of the SQL:2003
specification
Owners: devrim@CommandPrompt.com
Branches: FC-5 FC-6
InitialCC: devrim@CommandPrompt.com
Comment 32 Devrim GUNDUZ 2007-03-09 05:18:52 EST
Hmm. If possible, I'd like to see this package in EL-4 and EL-5, too.
Comment 33 Jens Petersen 2007-03-09 08:11:24 EST
Fedora cvs and branches added.

Sorry could you please put in a separate request for the EPEL branches this time.
Comment 34 Jens Petersen 2007-03-09 09:09:04 EST
Nevermind, added the EPEL branches too. :)

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