Bug 785619 - Review Request: lutok - Lightweight C++ API library for Lua
Review Request: lutok - Lightweight C++ API library for Lua
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-29 20:25 EST by Julio Merino
Modified: 2013-10-19 10:42 EDT (History)
3 users (show)

See Also:
Fixed In Version: lutok-0.1-1.fc16
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-02-07 22:01:37 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
michel: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Julio Merino 2012-01-29 20:25:45 EST
Spec URL: ftp://ftp.netbsd.org/pub/NetBSD/misc/jmmv/lutok/lutok.spec
SRPM URL: ftp://ftp.netbsd.org/pub/NetBSD/misc/jmmv/lutok/lutok-0.1-1.fc17.src.rpm
Description:

Hello!  A few years ago, I packaged bmake and mk-files and submitted them for inclusion into Fedora. The packages passed the review and I got sponsored, but then I changed my work environment, I ceased to use such packages and thus stopped maintaining the packages and apparently lost my sponsorship.

I would like now to package some additional software, starting with Lutok for which I'm submitting a review request here. This is a simple library to access Lua from C++. My previous experiences with spec files did not include subpackages nor libraries, so I'm expecting that the spec file will not be perfect on the first try!  Note that I have made an attempt to comply with the packaging guidelines that seemed relevant.

Lastly, Lutok is my own creation.  I have interest in submitting this package for two reasons.  First because having binary packages available in one of the major Linux distribution will ensure that users can get easy access to my little library.   (I certainly don't expect them going through the hassle of building it from a source package.)  And second because Lutok is a direct dependency of another package I want to submit, and thus I have to start with this one ;-)

Please take the time to review this package and send me any improvements you would like to see, both in the spec file and in the source itself!
Comment 1 Michel Alexandre Salim 2012-02-02 06:00:13 EST
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.
Comment 2 Michel Alexandre Salim 2012-02-02 08:42:26 EST
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)
Comment 3 Julio Merino 2012-02-02 11:21:07 EST
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!
Comment 4 Michel Alexandre Salim 2012-02-03 08:59:38 EST
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
Comment 5 Julio Merino 2012-02-03 10:17:45 EST
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!).
Comment 6 Michel Alexandre Salim 2012-02-04 10:21:11 EST
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
Comment 7 Michel Alexandre Salim 2012-02-04 10:23:15 EST
(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 :)
Comment 8 Julio Merino 2012-02-06 18:34:09 EST
New Package SCM Request
=======================
Package Name: lutok
Short Description: Lightweight C++ API library for Lua
Owners: jmmv
Branches: f16
InitialCC:
Comment 9 Jon Ciesla 2012-02-06 19:14:58 EST
Git done (by process-git-requests).
Comment 10 Julio Merino 2012-02-07 22:01:37 EST
Thanks for the review! I have submitted the package into git and issued builds for f17 and f16.
Comment 11 Fedora Update System 2012-02-07 22:07:30 EST
lutok-0.1-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/lutok-0.1-1.fc16
Comment 12 Fedora Update System 2012-02-25 03:37:15 EST
lutok-0.1-1.fc16 has been pushed to the Fedora 16 stable repository.
Comment 13 Julio Merino 2012-05-06 11:13:11 EDT
Package Change Request
======================
Package Name: atf
New Branches: f17
Owners: jmmv
Comment 14 Julio Merino 2012-05-06 11:16:36 EDT
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.
Comment 15 Jon Ciesla 2012-05-06 16:57:30 EDT
Git done (by process-git-requests).

Note You need to log in before you can comment on or make changes to this bug.