Bug 573448
Summary: | Review request: TinyCDB | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Adrien Bustany <adrien-xx-redhatbz> |
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
Status: | CLOSED DUPLICATE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | 12 | CC: | fedora-package-review, itamar, martin.gieseking, notting, supercyper1 |
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: | 2010-07-12 03:45:44 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: |
Description
Adrien Bustany
2010-03-14 21:28:06 UTC
Adrien, is this your first package? I can't find you in the Fedora Account System. If so, you have to be sponsored to become a packager (see https://fedoraproject.org/wiki/PackageMaintainers/Join). To find one, add FE-NEEDSPONSOR to the Blocks field above. A couple of quick comments on your package: - The summary should not mention the package name. Simply replace it by "A simple utility for creating and reading constant data bases" - According to the source code headers and the website, TinyCDB is Public Domain software, so change the license from "GPL" to "Public Domain". - Add short comments above Patch0 and Patch1 describing what they do - The lines of the description text should not exceed 80 characters. Split them properly. - File libcdb.so must be placed in the devel package. - You must call ldconfig in %post and %postun (see https://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries) - Replace the man page suffixes ".gz" with "*", e.g. %{_mandir}/man1/cdb.1* since packages shouldn't rely on a specific compression format. - The version/revision number is missing in the changelog entry: * Sun Mar 14 2010 Adrien Bustany <abustany> - 0.77-1 Hi Martin, thanks for the comprehensive review. I actualized the SPEC file and SRPM. I didn't increment the release number, should have I done it ? I didn't do it because I didn't really "improve" the SPEC file, but rather fixed beginner's bugs. I do have a FAS account, under the same mail address as my bugzilla. But I'm not a packager yet. So I added the block you told me. Cheers Adrien (In reply to comment #2) > I didn't increment the release number, should have I done it ? Hi Adrien, yes, you should bump the release number every time you offer a new/different spec file. It doesn't matter what kind of changes have been made. Otherwise it could become confusing if several reviewers downloaded and comment on different versions of the same release. I had a closer look at the package and there are still some things to do: - First of all, put the file libcdb.so.1 back to the base package. Only the symlink libcdb.so should go to devel. - You must ensure that CFLAGS is set to %{optflags} during the build process. In the Makefile delete line CFLAGS = -O, and add CFLAGS="%{optflags}" to the "make" lines in the %build section - I suggest to create a single patch for the Makefile instead of splitting it in two parts. - All binaries should be linked against shared libraries if possible, and shouldn't use the static variants. Fortunately, the Makefile contains a rule to build cdb-shared, which we could rename to cdb. Thus, change the %build section to (long lines clipped here): make %{?_smp_mflags} PREFIX="%{_prefix}" ... make shared %{?_smp_mflags} PREFIX="%{_prefix}" ... mv cdb-shared cdb - Finally, always check your RPMs with rpmlint in order to find potential problems. After applying the above changes, I get only spelling warnings that could be ignored: $ rpmlint /var/lib/mock/fedora-12-x86_64/result/*.rpm tinycdb.src: W: spelling-error %description -l en_US cdb -> db, cab, cob tinycdb.src: W: spelling-error %description -l en_US passwd -> passed tinycdb.src: W: spelling-error %description -l en_US gdbm -> DBMS, Gd, GDP tinycdb.src: W: spelling-error %description -l en_US dbm -> db, dim, dam tinycdb.src: W: spelling-error %description -l en_US ndbm -> DBMS, Nd, numb tinycdb.src: W: spelling-error %description -l en_US runtime -> run time, run-time, untimely tinycdb.x86_64: W: spelling-error %description -l en_US passwd -> passed tinycdb.x86_64: W: spelling-error %description -l en_US gdbm -> DBMS, Gd, GDP tinycdb.x86_64: W: spelling-error %description -l en_US dbm -> db, dim, dam tinycdb.x86_64: W: spelling-error %description -l en_US ndbm -> DBMS, Nd, numb tinycdb.x86_64: W: spelling-error %description -l en_US runtime -> run time, run-time, untimely tinycdb-devel.x86_64: W: spelling-error %description -l en_US cdb -> db, cab, cob tinycdb-devel.x86_64: W: spelling-error %description -l en_US passwd -> passed tinycdb-devel.x86_64: W: spelling-error %description -l en_US gdbm -> DBMS, Gd, GDP tinycdb-devel.x86_64: W: spelling-error %description -l en_US dbm -> db, dim, dam tinycdb-devel.x86_64: W: spelling-error %description -l en_US ndbm -> DBMS, Nd, numb tinycdb-devel.x86_64: W: spelling-error %description -l en_US runtime -> run time, run-time, untimely tinycdb-devel.x86_64: W: no-documentation 4 packages and 0 specfiles checked; 0 errors, 18 warnings. Hmm, is there a policy for where to put symlinks to .so files ? I didn't find anything in the packaging guidelines... Anyway, so put back the .so.1 file where it belongs, and fixed hopefully all your remarks. I had actually run rpmlint, but on the spec file, not the generated rpms... I also merged all the patches in one. I'll write to the author to see if we can get that upstream. SPEC: http://mymadcat.com/~madcat/packages/SPECS/tinycdb.spec SRPM: http://mymadcat.com/~madcat/packages/SRPMS/tinycdb-0.77-2.fc12.src.rpm Note: rpmlint here whines about a non stripped object, is this normal ? I searched for "strip" in the packaging guidelines but found nothing :/ (In reply to comment #4) > Hmm, is there a policy for where to put symlinks to .so files ? I didn't find > anything in the packaging guidelines... Yes, see here for example: https://fedoraproject.org/wiki/Packaging/Guidelines#DevelPackages I'll have a look at the rest later today. (In reply to comment #4) > Note: rpmlint here whines about a non stripped object, is this normal ? No, the debuginfo should be automatically stripped by rpmbuild (via find-debuginfo.sh). If that doesn't happen properly, there is usually something wrong. In this case, the file permissions of libcdt.so.1 aren't properly set. find-debuginfo.sh only strips executable files. Thus, replace the 644 by 755 in line 140 of the Makefile: install -m 755 -t $(DESTDIR)$(libdir) $(SHAREDLIB) Thanks, that fixed it! SPECS: http://mymadcat.com/~madcat/packages/SPECS/tinycdb.spec SRPM: http://mymadcat.com/~madcat/packages/SRPMS/tinycdb-0.77-3.fc12.src.rpm Cheers my suggestion is to port it to use gnu-auto-tools It would of course be better, I'd have to see that with the maintainer. Meanwhile, my patches, though not graceful, are working... Two further remarks: :) - man3/cdb.3* contains the API documentation, thus it belongs to -devel - please add a blank line between the changelog revisions BTW, I'm not a sponsor, so I can't formally review this package. Adrien, Do you still interested in this package? We need this package be reviewed soon. In order to get sponsored[1], you need to submit some other Review Request along with this package and do some preview review[2] to other Review Request. [1]https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group [2]http://fedoraproject.org/wiki/Package_Review_Process Chen, I'm affraid I won't have enough spare time to properly work on this package, feel free to take it if you want Regards Adrien (In reply to comment #12) > Chen, > > I'm affraid I won't have enough spare time to properly work on this package, > feel free to take it if you want > > Regards > > Adrien Thanks for contributing to fedora, feel free to apply co-maintainer for all of my packages[1] after you get sponsored some time later :) [1] https://admin.fedoraproject.org/pkgdb/users/packages/supercyper *** This bug has been marked as a duplicate of bug 612023 *** |