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.
I don't think that dbench should be packaged in that package, but in a separate package.
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.
dbench package review has been submitted at #244936 FYI.
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
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}
Updated spec file and SRPMS http://people.redhat.com/sundaram/olpc-utils.spec http://people.redhat.com/sundaram/olpc-utils-0.11-1.src.rpm
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.
Going through checklist
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
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.
turns out he is already sponsored.
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
Ah, both Parag and I missed the %{_datadir}/olpc ownership issue. I can fix the upstream tarball.
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
Fixed spec file and SRPM http://people.redhat.com/sundaram/olpc-utils.spec http://people.redhat.com/sundaram/olpc-utils-0.14-1.fc7.src.rpm
still needs to own %dir %{_datadir}/olpc
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
Fixed http://people.redhat.com/sundaram/olpc-utils.spec http://people.redhat.com/sundaram/olpc-utils-0.14-1.fc7.src.rpm
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
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
(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.
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.
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
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?
(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.
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.
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.
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
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.
New spec and SRPMS http://people.redhat.com/sundaram/olpc-utils.spec http://people.redhat.com/sundaram/olpc-utils-0.15-1.fc7.src.rpm
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.
(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.
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,johnp Branches: olpc-2 InitialCC:
cvs done.
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.