Bug 785619
Summary: | Review Request: lutok - Lightweight C++ API library for Lua | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Julio Merino <julio> |
Component: | Package Review | Assignee: | Michel Lind <michel> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | michel, notting, package-review |
Target Milestone: | --- | Flags: | michel:
fedora-review+
gwync: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | lutok-0.1-1.fc16 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2012-02-08 03:01:37 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
Julio Merino
2012-01-30 01:25:45 UTC
Welcome back, Julio! Do you have a test Fedora installation (whether virtualized or on a physical machine) that you can use to keep up with package maintenance? The spec looks straightforward; I'll start the review later today. Some short notes, we'll do the full review once this is taken care of. - license field should be just "License: BSD". see the short name column in http://fedoraproject.org/wiki/Licensing#Good_Licenses - will you be packaging this for Fedora only, or also for RHEL? Unless you're targeting RHEL version 5 or below, you don't need to declare the BuildRoot, and you don't need to clean it at the beginning of %install: http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag - likewise, %clean section can be removed unless you're targeting RHEL 6 or below. - %setup defaults to "-n %{name}-%{version}". Unless you do something unusual here you can just do %setup -q (to think about it, we don't have a Lua package naming guideline yet. Not sure if this should be lutok, lua-lutok, or lua-tok. The Python precedent is that packages starting with "py" don't need to be prefixed with python-, so in lieu of an explicit guideline the name seems OK for now) - the INSTALL file describes a 'make check' step. I take it without ATF the tests don't do anything, but since they run just fine anyway, could you add a check section under %install, as such: %check make check that way once ATF is packaged you don't have to change this package much. Since you're the author of ATF as well, do you want to submit it for inclusion as well? - you're installing documentation twice -- %doc list-of-files copy the files to %{_defaultdocdir}/%{name}-%{version}/ and you also have files in %{_defaultdocdir}/%{name}. The former is canonical, just remove the latter as part of %install, or update the build script to make doc installation a separate step - consider packaging examples and HTML docs in a separate -doc subpackage - devel subpackage should Require: %{name} = %{version}-%{release} (it should match the main package) Hello Michel, Thanks for the review. Some answers to your questions, in no particular order: Yes, I do have a Fedora VM installation that I intend to keep. In fact, I have had it for a long time already and use it regularly for the development of Lutok, ATF, etc. The reason is that Fedora includes a more up-to-date GCC and header files than my other build platforms (NetBSD, OS X)... which does wonders in catching programming errors :-) (I.e. more warnings, strict validation of explicit includes, etc.) No, I'm not targeting RHEL, so I have removed the extra boilerplate. Regarding the tests, yes, I am planning to package ATF later. I didn't do so yet because I wanted to get started with something simpler. The package for ATF will be trickier and potentially-controversial due to file layout issues, so I'd rather sort this out later once I've got the basics of packaging straight. I have created a separate doc package to include the html files and the examples. Lastly, I have fixed the documentation issue by preventing the Makefile from installing the documents. I have also changed the package to install its documentation into a "name-version" directory instead of just "name" to prevent having both "doc/lutok" and "doc/lutok-0.1". (I was confused about the behavior of %doc, hence why I had listed the documents twice in the previous spec version.) The spec and srpm files have been updated. Please take another look and thanks in advance! Hi Julio, (note: you forgot to update the SRPM, but the spec looks really close now) - %{_libdir}/liblutok.so.0 should be in the main package, not in -devel. only *.so should go to devel - The html and examples files are still ending up in the main package. Try installing them to %{_defaultdocdir}/%{name}-doc-%{version} instead %doc behavior is a bit confusing indeed. I'm pretty sure files in %{_defaultdocdir}/%{name}-%{version} automatically get registered as part of the package, and files tagged as %doc without absolute paths get copied there. - BuildRoot declaration, and cleanup in %install, can be removed - %files devel should have %{_includedir}/lutok/ _without_ the trailing *. With, you're telling RPM to own the files but not the directory itself Hmm, this is weird. I had addressed some of the comments you mention in your last mail (like getting rid of the buildroot), but somehow I screwed up when uploading the new versions of the files to the ftp site... Anyway! Fixed the doc issues by putting the files in a lutok-doc-0.1 directory as you suggest. Also added a missing Group: Documentation to the doc package. Also hopefully fixed the rest of issues mentioned. Updated the spec and srpm files (hopefully correctly this time!). Only one tiny issue left, which you can fix when importing the package -- the -doc subpackage should depend on the main package just like -devel: Requires: %{name} = %{version}-%{release} (since after all it does not make sense to have documentation for a non-matching version of the package. Also, from experience, when -doc does not require the main package, you can end up with stray -doc subpackages after removing the rest) APPROVED. Let me know your Fedora account system (FAS) username and I'll do the sponsorship, and then request the SCM repo for lutok as described here: http://fedoraproject.org/wiki/Package_SCM_admin_requests And do let me know when ATF is ready for packaging! * TODO Review [90%] - [X] Names [2/2] - [X] Package name - [X] Spec name - [X] Package version [2/2] http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Versioning - [X] Version number http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Version_Tag - [X] Release tag http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Release_Tag http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages - [X] Meets [[http://fedoraproject.org/wiki/Packaging/Guidelines][guidelines]] - [X] Source files match upstream ✗ sha1sum lutok-0.1.tar.gz ../SOURCES/lutok-0.1.tar.gz 2d56bdd27eedcb7cea26fea3ad1bb258c248b9d0 lutok-0.1.tar.gz 2d56bdd27eedcb7cea26fea3ad1bb258c248b9d0 ../SOURCES/lutok-0.1.tar.gz - [X] [[http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries][No bundled libraries]] - [X] License [4/4] - [X] License is Fedora-approved - [X] No licensing conflict - [X] License field accurate - [X] License included iff packaged by upstream - [X] rpmlint [2/2] - [X] on src.rpm lutok.src: W: invalid-url Source0: http://lutok.googlecode.com/files/lutok-0.1.tar.gz HTTP Error 404: Not Found harmless, rpmlint somehow cannot handle Google Code URLs - [X] on x86_64.rpm lutok-devel.x86_64: W: no-documentation 4 packages and 0 specfiles checked; 0 errors, 1 warnings. Harmless, really. - [X] Language & locale [2/2] - [X] Spec in US English - [X] Spec legible - [X] Build [3/3] - [X] Koji results http://koji.fedoraproject.org/koji/taskinfo?taskID=3762278 - [X] BRs complete - [X] Directory ownership - [-] Spec inspection [7/8] - [X] ldconfig for libraries - [X] No duplicate files - [X] File permissions - [X] Filenames must be UTF-8 - [X] no BuildRoot ([[https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag][except if targeting RHEL5]]) - [X] Macro usage consistent - [-] Documentation [2/3] - [X] If large docs, separate -doc - [X] %doc files are non-essential - [ ] requires main package - [X] Development [4/4] - [X] Headers in -devel - [X] If versioned .so's, unversioned in -devel - [X] -devel, -static requires main - [X] No .la (In reply to comment #6) > APPROVED. Let me know your Fedora account system (FAS) username and I'll do the > sponsorship, and then request the SCM repo for lutok as described here: Found your username (jmmv) and I've done the sponsorship. Welcome again :) New Package SCM Request ======================= Package Name: lutok Short Description: Lightweight C++ API library for Lua Owners: jmmv Branches: f16 InitialCC: Git done (by process-git-requests). Thanks for the review! I have submitted the package into git and issued builds for f17 and f16. lutok-0.1-1.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/lutok-0.1-1.fc16 lutok-0.1-1.fc16 has been pushed to the Fedora 16 stable repository. Package Change Request ====================== Package Name: atf New Branches: f17 Owners: jmmv Package Change Request ====================== Package Name: lutok New Branches: f17 Owners: jmmv Oops... my previous comment listed a mismatched package name. I presume this will be caught by whichever scripts handle this... but otherwise filing a new request here in the hope that it overrides the previous one. Git done (by process-git-requests). |