Bug 193071
Summary: | Review Request: ruby-sqlite3 | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | David Lutterkort <lutter> |
Component: | Package Review | Assignee: | Jason Tibbitts <j> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | 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
I ran rpmlint on both the SRPM, and an i386 rpm built under rawhide; both produce no output. 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. 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 Ther Ruby guidelines are ratified now; taking this for review (probably a bit later today). 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. (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 (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? 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 (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... 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 (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. Built successfully as job 12477 |