Bug 244908 - Review Request: olpc-utils - various utilities used by OLPC but not packaged anywhere else
Review Request: olpc-utils - various utilities used by OLPC but not packaged...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: John (J5) Palmieri
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-19 15:49 EDT by Rahul Sundaram
Modified: 2013-03-13 01:42 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-25 18:01:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
johnp: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Rahul Sundaram 2007-06-19 15:49:29 EDT
Spec URL: http://people.redhat.com/sundaram/olpc-utils.spec
SRPM URL: http://people.redhat.com/sundaram/olpc-utils-0.1-2.src.rpm
Description: 

Utilities used by OLPC:

  * olpc-keyboard - tools for mapping keys on the OLPC keyboards
  * dbench - benchmarking tool used to test the OLPC machines

This is my first package and I need a sponsor.
Comment 1 Patrice Dumas 2007-06-19 15:59:33 EDT
I don't think that dbench should be packaged in that package, 
but in a separate package.
Comment 2 Rahul Sundaram 2007-06-19 18:59:52 EDT
Splitting off dbench seemed a good thing to do. I will submit a separate review
for that shortly. Meanwhile here are the updated spec and SRPMS for olpc-utils.

http://people.redhat.com/sundaram/olpc-utils.spec
http://people.redhat.com/sundaram/olpc-utils-0.1-3.src.rpm

Thanks for the reviews. 
Comment 3 Rahul Sundaram 2007-06-19 19:14:36 EDT
dbench package review has been submitted at #244936 FYI. 
Comment 4 John (J5) Palmieri 2007-06-19 19:59:06 EDT
First pass

olpc-keyboard needs to be renamed olpc-utils everywhere
I need to add a COPYING file upstream

I'll tell you what, let me put a Makefile upstream and shoot you a new package
so that make and make install DESTDIR= works


Comment 5 John (J5) Palmieri 2007-06-19 20:59:40 EDT
New packages

http://people.freedesktop.org/~johnp/olpc-utils-0.10.tar.bz2
http://people.freedesktop.org/~johnp/olpc-utils-0.10.tar.bz2.md5

in the spec

%build
make

%install
make install DESTDIR=%{buildroot}
Comment 7 Parag AN(पराग) 2007-06-20 05:42:31 EDT
1 ) rpmlint on RPM reported
W: olpc-utils setup-not-quiet
Use the -q option to the %setup macro to avoid useless build output from
unpacking the sources.

you SHOULD replace current %prep with following
%prep
%setup -q

2) Everything else looks good except license text is not included in package. 
you SHOULD add following line
%doc COPYING in SPEC file.
Comment 8 John (J5) Palmieri 2007-06-20 16:06:06 EDT
Going through checklist
Comment 9 John (J5) Palmieri 2007-06-20 16:11:55 EDT
olpc-util review

      - MUST: rpmlint must be run on every package. The output should be posted
in the review.

W: olpc-utils no-url-tag
Please add a URL to the git tree

Url: http://dev.laptop.org/git?p=projects/olpc-utils;a=summary

      - MUST: The package must be named according to the Package Naming Guidelines.
Ok
      - MUST: The spec file name must match the base package %{name}, in the
format %{name}.spec unless your package has an exemption on Package Naming
Guidelines.
Ok
      - MUST: The package must meet the Packaging Guidelines.

Ok

      - MUST: The package must be licensed with an open-source compatible
license and meet other legal requirements as defined in the legal section of
Packaging Guidelines.

Ok (GPL) 

      - MUST: The License field in the package spec file must match the actual
license.
Ok

      - MUST: If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package must be included in %doc.

Fix add %doc COPYING to 

      - MUST: The spec file must be written in American English.
Ok
      - MUST: The spec file for the package MUST be legible. If the reviewer is
unable to read the spec file, it will be impossible to perform a review. Fedora
is not the place for entries into the Obfuscated Code Contest
(http://www.ioccc.org/).
Ok
      - MUST: The sources used to build the package must match the upstream
source, as provided in the spec URL. Reviewers should use md5sum for this task.
If no upstream URL can be specified for this package, please see the Source URL
Guidelines for how to deal with this.

Ok(we are upstream source)

      - MUST: The package must successfully compile and build into binary rpms
on at least one supported architecture.
Ok

      - MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch. Each architecture listed in ExcludeArch needs to have a bug filed
in bugzilla, describing the reason that the package does not compile/build/work
on that architecture. The bug number should then be placed in a comment, next to
the corresponding ExcludeArch line. New packages will not have bugzilla entries
during the review process, so they should put this description in the comment
until the package is approved, then file the bugzilla entry, and replace the
long explanation with the bug number. (Extras Only) The bug should be marked as
blocking one (or more) of the following bugs to simplify tracking such issues:
FE-ExcludeArch-x86, FE-ExcludeArch-x64, FE-ExcludeArch-ppc, FE-ExcludeArch-ppc64

Ok

      - MUST: All build dependencies must be listed in BuildRequires, except for
any that are listed in the exceptions section of Packaging Guidelines; inclusion
of those as BuildRequires is optional. Apply common sense.

Ok

      - MUST: The spec file MUST handle locales properly. This is done by using
the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
Ok (no translations)
      - MUST: Every binary RPM package which stores shared library files (not
just symlinks) in any of the dynamic linker's default paths, must call ldconfig
in %post and %postun. If the package has multiple subpackages with libraries,
each subpackage should also have a %post/%postun section that calls
/sbin/ldconfig. An example of the correct syntax for this is: 

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

Ok (No %{_libdir} libraries)
      - MUST: If the package is designed to be relocatable, the packager must
state this fact in the request for review, along with the rationalization for
relocation of that specific package. Without this, use of Prefix: /usr is
considered a blocker.
Ok (package not relocatable)

      - MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory. The exception to this are directories listed explicitly
in the Filesystem Hierarchy Standard
(http://www.pathname.com/fhs/pub/fhs-2.3.html), as it is safe to assume that
those directories exist.
Ok

      - MUST: A package must not contain any duplicate files in the %files listing.
Ok

      - MUST: Permissions on files must be set properly. Executables should be
set with executable permissions, for example. Every %files section must include
a %defattr(...) line.
Ok

      - MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
Ok

      - MUST: Each package must consistently use macros, as described in the
macros section of Packaging Guidelines.
Ok

      - MUST: The package must contain code, or permissable content. This is
described in detail in the code vs. content section of Packaging Guidelines.
Ok

      - MUST: Large documentation files should go in a -doc subpackage. (The
definition of large is left up to the packager's best judgement, but is not
restricted to size. Large can refer to either size or quantity)
Ok(no large docs)

      - MUST: If a package includes something as %doc, it must not affect the
runtime of the application. To summarize: If it is in %doc, the program must run
properly if it is not present.
Ok

      - MUST: Header files must be in a -devel package.
Ok(No libraries)

      - MUST: Static libraries must be in a -static package.
Ok (No static libraries)

      - MUST: Packages containing pkgconfig(.pc) files must 'Requires:
pkgconfig' (for directory ownership and usability).
Ok (No pkgconfig)

      - MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), then library files that end in .so (without suffix) must go in a
-devel package.
Ok (no libraries)
      - MUST: In the vast majority of cases, devel packages must require the
base package using a fully versioned dependency: Requires: %{name} =
%{version}-%{release} 
Ok(No devel)
      - MUST: Packages must NOT contain any .la libtool archives, these should
be removed in the spec.
Ok (No Libraries)
      - MUST: Packages containing GUI applications must include a
%{name}.desktop file, and that file must be properly installed with
desktop-file-install in the %install section. This is described in detail in the
desktop files section of Packaging Guidelines. If you feel that your packaged
GUI application does not need a .desktop file, you must put a comment in the
spec file with your explanation.
Ok (no .desktop file)
      - MUST: Packages must not own files or directories already owned by other
packages. The rule of thumb here is that the first package to be installed
should own the files or directories that other packages may rely upon. This
means, for example, that no package in Fedora should ever share ownership with
any of the files or directories owned by the filesystem or man package. If you
feel that you have a good reason to own a file or directory that another package
owns, then please present that at package review time.
Ok
      - MUST: At the beginning of %install, each package MUST run rm -rf
%{buildroot} (or $RPM_BUILD_ROOT). See Prepping BuildRoot For %install for details.
Ok
      - MUST: All filenames in rpm packages must be valid UTF-8.
Ok

SHOULD Items:
      - SHOULD: If the source package does not include license text(s) as a
separate file from upstream, the packager SHOULD query upstream to include it.
Ok
     
      - SHOULD: The reviewer should test that the package builds in mock.
Ok


Comment 10 John (J5) Palmieri 2007-06-20 16:19:26 EDT
APPROVED with conditions

Please add the %doc COPYING line to the %files section and add 

Url: http://dev.laptop.org/git?p=projects/olpc-utils;a=summary

to the headers.  Also you need a sponsor.  I'm still figuring out how to do that
since I can sponsor right now.
Comment 11 John (J5) Palmieri 2007-06-20 16:23:26 EDT
turns out he is already sponsored.
Comment 12 Patrice Dumas 2007-06-20 16:25:12 EDT
This package isn't acceptable even with the changes asked above.

%{_datadir}/olpc/ is unowned, this is a blocker.

Also I think that the macro olpc_utils_version is useless

The optflags are not used during compilation. This is ablocker.

The Makefile doesn't allow to use the macros like %{_datadir}
and so on, so I think that the best would be to do the install
'by hand', like

install -d %{buildroot}%{_datadir}/olpc/keycodes
....

Another reason would be because there is no possibility to
keep the timestamp during install, like

install -p --mode=0664 olpc-evdev %{buildroot}%{_datadir}/olpc/keycodes
Comment 13 John (J5) Palmieri 2007-06-21 14:57:37 EDT
Ah, both Parag and I missed the %{_datadir}/olpc ownership issue.  I can fix the
upstream tarball.
Comment 14 John (J5) Palmieri 2007-06-21 15:31:00 EDT
New package respects $(prefix), $(bindir), $(datadir), etc. and preserves
timestamps:

http://people.freedesktop.org/~johnp/olpc-utils-0.12.tar.bz2
http://people.freedesktop.org/~johnp/olpc-utils-0.12.tar.bz2.md5
Comment 16 John (J5) Palmieri 2007-06-22 18:59:00 EDT
still needs to own %dir %{_datadir}/olpc
Comment 17 John (J5) Palmieri 2007-06-22 19:04:24 EDT
rpmlint olpc-utils-0.14-1.fc7.src.rpm
E: olpc-utils no-cleaning-of-buildroot %clean
W: olpc-utils no-%clean-section

please add 

%clean
rm -rf %{buildroot}

and then post the new .src.rpm
Comment 19 John (J5) Palmieri 2007-06-22 19:11:20 EDT
Just for future refrence you should up the release after each change.

rpmlint passes
upstream tarball uses configure and respects %configure optflags
upstream tarball respects DISTDIR
%{_libdir}/olpc
and %{_libdir}/olpc/keycodes owned by package

All issues have been cleaned up

APPROVED - please continue with CVS access request
Comment 20 Patrice Dumas 2007-06-22 19:16:12 EDT
git failed:

$ git clone git://dev.laptop.org/git/projects/olpc-utils; cd olpc-utils
Initialized empty Git repository in /home/dumas/tmp/olpc-utils/.git/
fatal: The remote end hung up unexpectedly
fetch-pack from 'git://dev.laptop.org/git/projects/olpc-utils' failed.

But why don't you use
http://people.freedesktop.org/~johnp/olpc-utils-0.14.tar.bz2

on the line
make%{?_smp_mflags}
there should be a space:
config.status: creating Makefile
+ make-j3
/var/tmp/rpm-tmp.37841: line 50: make-j3: command not found
erreur: Mauvais status de sortie pour /var/tmp/rpm-tmp.37841 (%build)

rpmlint shows that a %clean section is missing:
E: olpc-utils no-cleaning-of-buildroot %clean
W: olpc-utils no-%clean-section

Since John is reading this, the following compiler warning should
be solved:
olpc-bios-sig.c: In function 'main':
olpc-bios-sig.c:35: warning: implicit declaration of function 'open'
olpc-bios-sig.c:48: warning: implicit declaration of function 'close'
olpc-bios-sig.c:58: warning: implicit declaration of function 'isdigit'

Looking at the man pages, seems to be missing:

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <ctype.h>

Also for John, unless I am wrong configure.ac is missing, this is
not acceptable...

There is also certainly a bug somewhere (but I cannot read the
missing configure.ac...):
configure: Configuring dbench
Comment 21 Patrice Dumas 2007-06-22 19:18:25 EDT
(In reply to comment #19)

> All issues have been cleaned up
> 
> APPROVED - please continue with CVS access request

John, I think you should also wait for somebody else to
approve, the package is still not in acceptable shape.
Comment 22 Rahul Sundaram 2007-06-22 19:37:33 EDT
1) The git instructions in the spec file has been fixed. J5 put that freedesktop
tarball as a temporary place to build the spec and is not really the upstream url. 

2) make file space has been fixed

3)% clean has already been fixed in the previous spec file

http://people.redhat.com/sundaram/olpc-utils.spec
http://people.redhat.com/sundaram/olpc-utils-0.14-2.fc7.src.rpm

Rest of the feedback is directed to J5 since they are source code fixes. 
Comment 23 Patrice Dumas 2007-06-22 19:53:05 EDT
The tarball regeneration still doesn't work for me, when I do it I
don't get the same files than what is in the tarball, I get
only:
COPYING  Makefile  olpc-bios-sig.c  olpc-evdev  setolpckeys.c
Comment 24 Rahul Sundaram 2007-06-22 20:03:54 EDT
As the spec file indicates you can consider the source within the SRPM as
authoritative for now till J5 gets to fix rest of upstream issues. Is there any
problems specific to the spec file left?
Comment 25 Patrice Dumas 2007-06-23 03:30:33 EDT
(In reply to comment #24)
> As the spec file indicates you can consider the source within the SRPM as
> authoritative for now till J5 gets to fix rest of upstream issues. 

Ok. No problem with that. At the same time, to me, not having the 
configure.ac to understand how the configure was generated is a 
blocker for a release in fedora.

> Is there any
> problems specific to the spec file left?

spec seems right to me. But it is possible to have upstream issues
that block a release in fedora, and in that case I think that not 
having configure.ac in upstream tarball (even if it is only in the
srpm, no problem with that) is a blocker.
Comment 26 John (J5) Palmieri 2007-06-23 12:30:12 EDT
Makefile.in is packaged (the tarball posted is old, you can get the tarball from
the srpm).  We don't ship with the configure.ac file.  ./configure is
authoritative.  The configure.ac can be found in the git tree which is mentioned
in the .spec file.

I can add the configure.ac if you want.  In any case if someone wants to take
the review, be my guest, but please be timely on it.  I only took it because no
one else was.
Comment 27 Patrice Dumas 2007-06-23 12:37:43 EDT
The configure.ac should also be in the tarball in the srpm. A
configure without configure.ac is like shipping a binary without
the sources, this is not acceptable except in very rare situation
(bootstrap, firmwares...).

The review request was posted 4 days ago. Some review requests
are years old.
Comment 28 Patrice Dumas 2007-06-23 12:40:36 EDT
I still can't get the configure.ac and Makefile.in from git. I am 
completly ignorant in git, so maybe I am doing something wrong:

$ git clone git://dev.laptop.org/projects/olpc-utils; cd olpc-utils; ls
Initialized empty Git repository in /home/dumas/tmp/olpc-utils/.git/
remote: Generating pack...
remote: Done counting 26 objects.
remote: Deltifying 26 objects...
remote:  100% (26/26) done
Indexing 26 objects...
remote: Total 26 (delta 12), reused 0 (delta 0)
 100% (26/26) done
Resolving 12 deltas...
 100% (12/12) done

COPYING  Makefile  olpc-bios-sig.c  olpc-evdev  setolpckeys.c
Comment 29 John (J5) Palmieri 2007-06-23 13:04:12 EDT
btw it is git clone git://dev.laptop.org/projects/olpc-utils  which had been
descussed over irc.  The extra git directory is used when using the git+ssh
protocol. Also the URL line leads you to the gitweb tree.

New upstream packages

http://people.freedesktop.org/~johnp/olpc-utils-0.15.tar.bz2
http://people.freedesktop.org/~johnp/olpc-utils-0.15.tar.bz2.md5

Fixes, olpc-bios-sig.c by including the correct headers
Adds configure.ac to the dist
Fixes the configure.ac message to say configuring olpc-utils instead of dbench

Also thoes files won't be there on fd.o forever which is why they are not the
listed source. 
Comment 31 Patrice Dumas 2007-06-23 13:33:09 EDT
For J5: 
Seems to me that AC_INIT should be:

AC_INIT([olpc-utils],[0.15])

And then in Makefile.in you could set
VERSION=@PACKAGE_VERSION@

Rahul:
Otherwise everything seems sane now, you should proceed
to the cvs request.
Comment 32 John (J5) Palmieri 2007-06-23 13:38:28 EDT
(In reply to comment #27)
> The review request was posted 4 days ago. Some review requests
> are years old.

This is the wrong way of looking at it.  I intend to write up my experience with
the whole process and moving 37+ packages into the Fedora infrastructure.  It
was mostly a good experience but there a lot of things that would have made a
contributer quit long ago.  I'll addmit I had problems with my review of this
package, though I think they could have been resolved a bit quicker by removing
some of the process.  More on that later.

Put it this way, if there are reviews in the queue for a year, Fedora loses. One
package taking 4 days doesn't seem much to you but considering it is 1/8th of my
ship deadline and add that over however many packages that need to go through
review and you get an idea of timelines.  Unbuntu is getting more contributers
than Debian these days because they took a process full of red tape and
simplified it.

I thank you for your input and making my upstream project a bit more robust and
all the work you have done on our other packages.  I'm not saying I deserve to
ship crappy packages, I'm just saying there are more efficent ways of getting to
this point that I will bring up at the next FESCo meating.
Comment 33 Rahul Sundaram 2007-06-23 13:46:21 EDT
Thanks to Parag, Patrice Dumas and J5 for the reviews. 

New Package CVS Request
=======================
Package Name: olpc-utils 
Short Description: Tools for mapping keys on the OLPC keyboards and checking
BIOS signature.
Owners: sundaram@redhat.com,johnp@redhat.com
Branches: olpc-2
InitialCC: 
Comment 34 Kevin Fenzi 2007-06-25 14:51:44 EDT
cvs done.
Comment 35 Rahul Sundaram 2007-06-25 18:01:50 EDT
My first package in Fedora has been build successfully. Woohoo!

http://koji.fedoraproject.org/koji/taskinfo?taskID=48099

Closing review request. Many thanks to everyone who helped. 

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