Bug 201551
Summary: | Review Request: db4o | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Paul F. Johnson <paul> |
Component: | Package Review | Assignee: | Dominik 'Rathann' Mierzejewski <dominik> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | ||
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-08-09 15:42:15 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
Paul F. Johnson
2006-08-07 11:04:50 UTC
OK, I'll review this as soon as I can figure out how to assign this bug to myself. Here's a formal review: MUST items OK: * package meets naming guidelines * source files match upstream $ md5sum db4o-5.5-mono.tar.gz 4aea1da3b96a2b92c9ac69ee2ccb9a63 db4o-5.5-mono.tar.gz * specfile is properly named, is cleanly written and uses macros consistently * dist tag is present * build root is correct * license field matches the actual license * license is open source-compatible * latest version is being packaged * BuildRequires are proper * rpmlint errors can be ignored $ rpmlint -i db4o-5.5-1.x86_64.rpm E: db4o no-binary The package should be of the noarch architecture because it doesn't contain any binaries. E: db4o only-non-binary-in-usr-lib There are only non binary files in /usr/lib so they should be in /usr/share. * final provides are sane mono(Db4oMonoTest) = 0.0.0.0 mono(Db4oTools) = 0.0.0.0 mono(db4o) = 5.5.1.0 db4o = 5.5-1 * no traditional shared libraries are present * 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 * %check is not present * no scriptlets present * code, not content * %docs are not necessary for the proper functioning of the package * no headers * no pkgconfig files * no libtool .la droppings * no locales * not a GUI app * not a web app MUST items BAD: * package doesn't meet packaging guidelines - {_prefix}/%{_lib} vs. %{_libdir}, See http://fedoraproject.org/wiki/Packaging/Mono#head-ae42c4b5de20e082855e2f5151542730ef40f15b http://fedoraproject.org/wiki/Packaging/Mono#head-932fb76878e1f34706c9962336a5f23fcc277af2 - installs shipped precompiled binaries, See http://fedoraproject.org/wiki/Packaging/Mono#head-1d3454f431ec1c8761eb46582e7f66bbddc1fd08 * source URL returns 404 $ wget http://www.db4o.com/community/db4o-5.5-mono.tar.gz --00:31:44-- http://www.db4o.com/community/db4o-5.5-mono.tar.gz => `db4o-5.5-mono.tar.gz' Resolving www.db4o.com... 213.203.204.172 Connecting to www.db4o.com|213.203.204.172|:80... connected. HTTP request sent, awaiting response... 404 Not Found 00:31:44 ERROR 404: Not Found. * final requires contain duplicates mono(Db4oTools) = 0.0.0.0 mono(System) = 1.0.5000.0 mono(db4o) = 5.5.1.0 mono(mscorlib) = 1.0.5000.0 mono-core i.e. mono(System) and mono(mscorlib) are provided by mono-core * documentation takes up 75% of the installed size, -docs subpackage is necessary SHOULD items: * License text included in package (License text not included upstream) Also, I don't like the use of sed where tr -d would suffice. E: db4o no-binary The package should be of the noarch architecture because it doesn't contain any binaries. I'll change that. E: db4o only-non-binary-in-usr-lib There are only non binary files in /usr/lib so they should be in /usr/share. Mono packages aren't sane when it comes to this and until mono is fixed, this one can be ignored * documentation takes up 75% of the installed size, -docs subpackage is necessary Not a problem - I did that with XaraLX i.e. mono(System) and mono(mscorlib) are provided by mono-core * documentation takes up 75% of the installed size, -docs subpackage is necessary It needs mono-core as a R and mono-devel as a BR. * package doesn't meet packaging guidelines - {_prefix}/%{_lib} vs. %{_libdir}, See Permitted for the time being - have a look at the mono packaging guidelines * Also, I don't like the use of sed where tr -d would suffice. I'm not sure that's a blocker though I'll fix the docs and noarch and upload a new spec file. Thanks for the review up to now. I don't know which mono packaging guidelines you've been reading, but those I pointed you to (from the Mono SIG page) say to use %{_libdir} and nothing else. They also say those rpmlint errors are to be ignored, so I don't understand why you want to make it a noarch package. Mono packages aren't supposed to be noarch, according to those guidelines. I've already said that. About Requires: mono-core - it's not necessary, as I've already said. Please read what I have written. Using sed instead of my preferred tr -d isn't a blocker, yes. "I don't know which mono packaging guidelines you've been reading, but those I pointed you to (from the Mono SIG page) say to use %{_libdir} and nothing else." See what happens when you build on x86_64 and you'll see why %{_prefix}/lib is permissable. Until such time that mono is fixed (that is, on x86 builds to /usr/lib and x86_64 builds to /usr/lib64) either upstream or for FC, the only 100% guaranteed method for others to be able to pick it up is to statically define where things go. This has been accepted in the short term (have a look at every other mono package registered to me and you'll see exactly the same hack is applied) "They also say those rpmlint errors are to be ignored, so I don't understand why you want to make it a noarch package. Mono packages aren't supposed to be noarch, according to those guidelines. I've already said that." Normally on a mono package, the executable is in /usr/lib/<package_name> with a symlink back to %{_bindir} and so rpmlint moans. What has happened here is I was in a rush to get out to get to work and have misread what you've said. Requires : mono-core, point taken. Spec URL: http://www.knox.net.nz/~nodoid/db4o.spec Addresses #2 Not uploaded a new src.rpm - there is no difference other than the spec file. Ok, upon further reading I see that you're right about %{_libdir}, though the guidelines say it is a temporary solution. However, you still haven't addressed my concerns about installing a precompiled binary. In particular, /usr/lib/mono/gac/db4o/*/db4o.dll (installed) is binary identical to db4o-5.5/dll/db4o.dll (shipped). The guidelines http://fedoraproject.org/wiki/Packaging/Mono#head-1d3454f431ec1c8761eb46582e7f66bbddc1fd08 say explicitly that "All packages must build from source". And I'd like you to name the docs package db4o-doc, not db4o-docs, as this naming seems to be more common among extras packages. This package is built from the source. It's simple enough to fix this problem, just %exclude it. That said, I have unearthed a slightly more sinister problem... Spec URL: http://www.knox.net.nz/~nodoid/db4o.spec Fixes the problems in #7 though Spec URL: http://www.knox.net.nz/~nodoid/db4o.spec SRPM URL: http://www.knox.net.nz/~nodoid/db4o-5.5-5.src.rpm I've removed the gacutil from the package as it's broken and added a devel (with .pc file) This now means that mcs foo.cs -pkg:db4o can now be used in much the same way as gtk-sharp needs to be used The tarball includes the .pc file Looks almost perfect, except for these new rpmlint warnings: W: db4o-devel no-documentation The package contains no documentation (README, doc, etc). You have to include documentation files. It can be ignored. W: db4o-doc non-standard-group Development/Documentation The value of the Group tag in the package is not valid. Valid groups are: [...]"Documentation"[...] I think you should call it just that. :) Oh, and you can also add %define debug_package %{nil} so that the empty debug package isn't generated. Something like that is found in other mono packages, too, and follows the packaging guidelines: http://fedoraproject.org/wiki/Packaging/Debuginfo#head-29573c4f61c3a4698b2d07c7e73cfa194785f257 Running the built Db4oMonoTest.exe seems fine, too. Fix the group tag, disable debuginfo generation, and it's APPROVED. Good work. Spec URL: http://www.knox.net.nz/~nodoid/db4o.spec SRPM URL: http://www.knox.net.nz/~nodoid/db4o-5.5-7.src.rpm Fixes #10 I've even added R pkg-config to the devel requires! (In reply to comment #11) > I've even added R pkg-config to the devel requires! Unfortunately the pkg-config program comes in a package called pkgconfig so you need an R: pkgconfig rather than an R: pkg-config There's a typo now: %define debug_packahe %{nil} Also, please put Requires: pkgconfig in a separate line. Spec URL: http://www.knox.net.nz/~nodoid/db4o.spec Fixes the above Is there a reason for having the pkgconfig on another line? From what I can see, there is nothing in the docs about doing it that way (and I certainly haven't on other packages) Having each explicit dependency in a separate line makes it easier to see what is required. I, for one, had some trouble finding your addition at first glance. That said, it's not a blocker, so the current version is APPROVED. Thanks for such a wonderful first review (on your part). Keep up the good work and I'll see you on IRC :-D The package imported into cvs still has the wrong pkg-config dependency for the -devel package, which will make this package uninstallable. |