Bug 193071

Summary: Review Request: ruby-sqlite3
Product: [Fedora] Fedora Reporter: David Lutterkort <lutter>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: hbrock, j
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-07-12 00:37:52 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 163779    

Description David Lutterkort 2006-05-24 21:36:17 UTC
Spec URL: http://people.redhat.com/dlutter/yum/spec/ruby-sqlite3.spec
SRPM URL: http://people.redhat.com/dlutter/yum/SRPMS/ruby-sqlite3-1.1.0-2.src.rpm
Description: 
Database driver to access SQLite v.3 databases from Ruby.

Comment 1 David Lutterkort 2006-05-24 22:15:54 UTC
I ran rpmlint on both the SRPM, and an i386 rpm built under rawhide; both
produce no output.

Comment 2 Jason Tibbitts 2006-06-02 19:59:28 UTC
A couple of comments:

The guidelines (which I think you wrote) use ruby_sitearchdir instead of
ruby_sitearch.  Not a big deal but I suppose we should try for consistency since
these first few packages will stand as examples.

You don't require a specific Ruby version.

You manually strip the .so, which is a bad idea because it breaks the debuginfo
package.  (It ends up empty.)  Everything is fine if you delete the call to
strip.  I'm guessing you saw an rpmlint warning about an unstripped binary;
making it executable is sufficient it fix that.

I wonder if we're any closer to getting fixed Ruby packages so that we can get
the guidelines ratified.

Comment 3 David Lutterkort 2006-06-06 00:21:45 UTC
I addressed all these problems with an updated package
  - the ruby_sitarch{,dir} happened because I started with the ruby specfile
template, but decided that I really want the macros to have the same name as the
entry in Config::CONFIG. I'll try and get the fedora-rpmdevtools template
updated once the guidelines have been finalized
  - requires now ruby >= 1.8 which is what the gemspec requires, too
  - no more stripping; thanks for the tip

Spec URL: http://people.redhat.com/dlutter/yum/spec/ruby-sqlite3.spec
SRPM URL: http://people.redhat.com/dlutter/yum/SRPMS/ruby-sqlite3-1.1.0-3.src.rpm

Comment 4 Jason Tibbitts 2006-06-29 18:28:48 UTC
Ther Ruby guidelines are ratified now; taking this for review (probably a bit
later today).

Comment 5 Jason Tibbitts 2006-07-06 04:08:39 UTC
My apologies; somehow this managed to slip my mind.

The first thing I notice is that the included test suite doesn't get run; it has
a dependency on FlexMock which of course isn't in Extras.  I don't think it
should block this package, but it would definitely be nice to get FlexMock into
extras so that the test suite can be turned on.

The gemspec file looks like a source of useful data; I wonder if we could use it
to generate a reasonable starting spec file.
 
The site{lib,arch} thing should be resolved now.  The guidelines say sitearchdir
and sitelib dir; is that we really wanted to go with?

There's no ruby(abi) requirement.

Interestingly, BR: ruby really is required; ruby-devel only pulls in ruby-libs.

Is the explicit sqlite requirement necessary?  rpm finds the libsqlite3.so.0
dependency on its own.

Review:
* package meets naming and packaging guidelines.
? specfile is properly named, is cleanly written and uses macros consistently.
X No ruby(abi) requirement.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* source files match upstream:
   56e47e1736bd50e2b71f11726ff9ecdd  sqlite3-ruby-1.1.0.tar.gz
* latest version is being packaged.
* BuildRequires are proper.
* package builds in mock (development, x86_64).
* rpmlint is silent.
* final provides and requires are sane:
   ruby(sqlite3)
   sqlite3_api.so()(64bit)
   ruby-sqlite3 = 1.1.0-3.fc6
  =
   libruby.so.1.8()(64bit)
   libsqlite3.so.0()(64bit)
   sqlite >= 3
* shared libraries are present, but internal to ruby.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* %clean is present.
O %check is present; dependencies not yet available for running test suite.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.
* not a GUI app.

Comment 6 David Lutterkort 2006-07-07 00:02:59 UTC
(In reply to comment #5)
> The gemspec file looks like a source of useful data; I wonder if we could use it
> to generate a reasonable starting spec file.

I tried that with http://people.redhat.com/dlutter/gem2spec.html which works
reasonably well. The thing that makes me hesitant about packaging gems are
outlined at http://fedoraproject.org/wiki/PackagingDrafts/RubyGems

> The site{lib,arch} thing should be resolved now.  The guidelines say sitearchdir
> and sitelib dir; is that we really wanted to go with?

I wanted to keep it close to the entry in Config::CONFIG those get set from. Do
you think the resulting macro names are too long ?

> There's no ruby(abi) requirement.

Oops.
 
> Is the explicit sqlite requirement necessary?  rpm finds the libsqlite3.so.0
> dependency on its own.

You are right - that was overkill

> Review:
> X No ruby(abi) requirement.
Fixed

Updated stuff:
Spec URL: http://people.redhat.com/dlutter/yum/spec/ruby-sqlite3.spec
SRPM URL: http://people.redhat.com/dlutter/yum/SRPMS/ruby-sqlite3-1.1.0-4.src.rpm




Comment 7 Jason Tibbitts 2006-07-09 03:26:29 UTC
(In reply to comment #6)

> I wanted to keep it close to the entry in Config::CONFIG those get set from. Do
> you think the resulting macro names are too long ?

I don't think the length is a problem; what is at issue is the fact that what's
in the current guidelines doesn't match what's being done here.  The first
approved Ruby package should follow the spec rather closely, I think.

So, if you want to keep it close to what's in Config::CONFIG, then the
guidelines don't reflect that and need to be changed, assuming it's not too late
to do that.

Finally, you use "%{__chmod}" instead of just plain "chmod", but don't use
"%{__rm}".  Any reason for using just one obsfucating macro?

Comment 8 David Lutterkort 2006-07-10 21:14:24 UTC
You are right about following the guidelines very closely. I have fixed the
ruby_site* macros to coincide with the guidelines (no dir at the end, Ville
Skyttae pointed out that all outher scripting languages use _sitelib, not
_sitelibdir, so let's stick with that)

As for using %{__chmod}, %{__rm} vs chmod, rm, I am open to either, though I am
never quite sure what the recommended one is, and whether it is correct to rely
on the $PATH of the user building the RPM.

Updated:
Spec URL: http://people.redhat.com/dlutter/yum/spec/ruby-sqlite3.spec
SRPM URL: http://people.redhat.com/dlutter/yum/SRPMS/ruby-sqlite3-1.1.0-5.src.rpm

Comment 9 Paul Howarth 2006-07-10 21:49:31 UTC
(In reply to comment #8)
> You are right about following the guidelines very closely. I have fixed the
> ruby_site* macros to coincide with the guidelines (no dir at the end, Ville
> Skyttae pointed out that all outher scripting languages use _sitelib, not
> _sitelibdir, so let's stick with that)
> 
> As for using %{__chmod}, %{__rm} vs chmod, rm, I am open to either, though I am
> never quite sure what the recommended one is, and whether it is correct to rely
> on the $PATH of the user building the RPM.

It's up to you but be consistent about it.

My personal preference is not to trust the user's $PATH and use either macros or
full paths for commands that don't have macros. I think from Jason's
"obsfucating macro" terminology that his preference lies the other way...


Comment 10 Jason Tibbitts 2006-07-11 00:58:48 UTC
If you really would prefer to type seven characters, five of which require the
shift key, instead of two for zero gain, then you're free to do so but you
should be consistent.  Personally my eyes start to bleed and my wrists start to
ache in empathy everytime someone uses a ton of needless macros.  However, even
the specfile templates aren't consistent about this.  (See, for example, the
python spec which uses "rm" and "%{__python}" on consecutive lines.)

In any case, the latest package looks good to me.  The only issue that hasn't
been addressed is that of the test suite; obviously we can't run it as is, but
now that we have functional guidelines, perhaps it will eventually be packaged
up and this package can then run its test suite.

APPROVED

Comment 11 Ville Skyttä 2006-07-11 06:52:42 UTC
(In reply to comment #10)
> python spec which uses "rm" and "%{__python}" on consecutive lines

The intention of using the macroized form for the python executable is to
provide some flexibility for local rebuilds in setups where multiple versions of
python are installed.  And upstream rpm (not in FC yet) uses that form for
computing sitelib and sitearch, we need to be in sync with that as long as those
variables are only conditionally defined in the template.  Ditto perl.

Comment 12 David Lutterkort 2006-07-12 00:37:52 UTC
Built successfully as job 12477