Bug 199682

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. :)