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.
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.
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}
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
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
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
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.
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
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.
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?
Sorry, however I must reopen this bug.
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?
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.
(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
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
New spec file: http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link.spec New SRPM: http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link-2.0-0.3.beta1.src.rpm Could you please take a look at them? Regards, Devrim
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...
One more issue. * Timestamps ------------------------------ install -m 644 *.sql %{buildroot}/%{_datadir}/%{name}/ ------------------------------ Please use "install -p -m 644" to keep timestamps.
Hi, New spec file: http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link.spec New SRPM: http://developer.postgresql.org/~devrim/rpms/other/dbi-link/postgresql-dbi-link-2.0-0.4.beta1.src.rpm Regards, Devrim
Removing this package from Extras, because the author did not want Oracle portions to be removed. So I'm closing this bug. Regards, Devrim
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
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.
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)
And again once back to FE-REVIEW
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?
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
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.
Created attachment 148462 [details] patch to spec file
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
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?
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.
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 Branches: FC-5 FC-6 InitialCC: devrim
Hmm. If possible, I'd like to see this package in EL-4 and EL-5, too.
Fedora cvs and branches added. Sorry could you please put in a separate request for the EPEL branches this time.
Nevermind, added the EPEL branches too. :)