Bug 810059 - Review request: opencl-utils - Useful OpenCL tools and utilities
Summary: Review request: opencl-utils - Useful OpenCL tools and utilities
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: 17
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Alec Leamas
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-04-05 02:57 UTC by Jeremy Newton
Modified: 2012-06-28 03:31 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-06-28 03:31:23 UTC
Type: Bug
Embargoed:
leamas.alec: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
ALternative Makefile, clean-up, respects CFLAGS. (304 bytes, text/plain)
2012-04-06 07:56 UTC, Alec Leamas
no flags Details
Patch from release 3 using the new Makefile, fixes most rpmlint warnings (3.18 KB, patch)
2012-04-07 07:41 UTC, Alec Leamas
no flags Details | Diff
My idea how to simplify the package structure. (3.58 KB, patch)
2012-04-07 07:42 UTC, Alec Leamas
no flags Details | Diff
Pkg-config, complete patch (2.35 KB, patch)
2012-04-07 07:45 UTC, Alec Leamas
no flags Details | Diff

Description Jeremy Newton 2012-04-05 02:57:18 UTC
OpenCL Utils is a project that aims to create various tools and utilities to
make the use of OpenCL more useful and efficient, such as: useful functions,
optimization hints and common kernel templates.

The only current tool in the kit is CLRun, which is a application that allows for dynamic loading of OpenCL.

I'm submitting this because another application I wish to submit requires CLrun; I'm sure other projects may benefit.

Here's the SPEC and SRPM:

SPEC:
http://dl.dropbox.com/u/42480493/opencl-utils.spec
SRPM:
http://dl.dropbox.com/u/42480493/opencl-utils-0-1.svn16.fc16.src.rpm

RPMLINT ERRORS:

opencl-utils.src: W: invalid-url Source0: opencl-utils.tar.xz

This is a svn checkout, see the comments above the source line.

CLRun.x86_64: W: no-soname /usr/lib64/libclrun.so

To be honest, I'm not sure if I should patch this or leave it be.

CLRun.x86_64: W: no-documentation
CLRun-devel.x86_64: W: no-documentation
CLRun-examples.x86_64: W: no-documentation

The source does not come with documentation.

CLRun-examples.x86_64: W: devel-file-in-non-devel-package /usr/share/opencl-utils/examples/clrun-example/example1.c
CLRun-examples.x86_64: W: devel-file-in-non-devel-package /usr/share/opencl-utils/examples/clrun-example/example2.cpp

The example package comes with example source code. I'm not sure if it work be better to compile them and include a examples-devel package with the code or leave how it is currently packaged.

Comment 1 Alec Leamas 2012-04-05 08:13:20 UTC
Just some initial remarks, no references (drop a line if need be).
- Package does not build in mock. Patch below fixes.
- As for the missing doc, file a bug upstream to add the license file.
- The examples might just go into -devel IMHO.
- You need to somehow use %{optflags} in the Makefile.
- Makefile contains a 'strip' command which should be removed.
- opencl-utils-debuginfo.i686: E: empty-debuginfo-package related to above.
- Since this a library intended to be used by other programs, the Makefile should be patched to include a soname (a %global macro), and symlinks setup accordingly. That patch should be acceptable upstream.
- The description for -devel says 'source code'; shouldn't that be 'header files'?
- The debug package name opencl-utils-debuginfo is not consistent w the others.


--- *spec.ORG	2012-04-05 09:16:17.211177855 +0200
+++ opencl-utils.spec	2012-04-05 09:17:25.477104941 +0200
@@ -18,6 +18,8 @@
 #http://code.google.com/p/dolphin-emu/source/browse/Externals/CLRun/clrun/generateClRun.pl
 Patch1:         %{name}-trimwhitespace.patch
 
+BuildRequires: mesa-libGL-devel
+
 %package     -n CLRun
 Summary:        Dynamic loader for OpenCL

Comment 2 Alec Leamas 2012-04-05 09:12:43 UTC
A second thought on naming: the %name macro is set to upstream's name opencl-utils. However, you override it everywhere and the package is effectively named CLrun. This does *not* match the upstream name.

I suggest that you name the packages to the default opencl-utils-*. Just live with the fact that only clrun is packaged, making a note in the %descr about it. If/when someone else needs more packaged it's just to fix, but without changing the package name.

Comment 3 Jeremy Newton 2012-04-05 15:36:26 UTC
(In reply to comment #1)
> Just some initial remarks, no references (drop a line if need be).
> - As for the missing doc, file a bug upstream to add the license file.

Yeah I plan to focus on that once I get the rest working

> - The examples might just go into -devel IMHO.

Hmmm, I figure I should look into building them in that case, it maybe more convienient to have them built along with the source; I've seen some packages work like this.

> - You need to somehow use %{optflags} in the Makefile.
> - Makefile contains a 'strip' command which should be removed.
> - opencl-utils-debuginfo.i686: E: empty-debuginfo-package related to above.

Noted, I'll see what I can do.

> - Since this a library intended to be used by other programs, the Makefile
> should be patched to include a soname (a %global macro), and symlinks setup
> accordingly. That patch should be acceptable upstream.

I have no idea how to do the symlink part, the soname part seems easy though. I would think the soname part can be fixed like so:
>sed -i 's/ldl/ldl -Wl,-soname,libclrun.so.0/g' src/clrun/Makefile
Although can you lend me a hand for the symlink? :)

> - The description for -devel says 'source code'; shouldn't that be 'header
> files'?

Not necessarily, it can be the source too, though it maynot be necessary if I can figure out how to patch dolphin-emu to use the *.so instead of rebuilding it.

> - The debug package name opencl-utils-debuginfo is not consistent w the others.

I just realized I need to make a opencl-utils-devel along with a CLRun-devel; I by accident put all the files in CLRun-devel instead.

> 
> --- *spec.ORG 2012-04-05 09:16:17.211177855 +0200
> +++ opencl-utils.spec 2012-04-05 09:17:25.477104941 +0200
> @@ -18,6 +18,8 @@
> 
> #http://code.google.com/p/dolphin-emu/source/browse/Externals/CLRun/clrun/generateClRun.pl
>  Patch1:         %{name}-trimwhitespace.patch
> 
> +BuildRequires: mesa-libGL-devel
> +
>  %package     -n CLRun
>  Summary:        Dynamic loader for OpenCL

Thanks! I knew I forgot something.

(In reply to comment #2)
> A second thought on naming: the %name macro is set to upstream's name
> opencl-utils. However, you override it everywhere and the package is
> effectively named CLrun. This does *not* match the upstream name.
> 
> I suggest that you name the packages to the default opencl-utils-*. Just live
> with the fact that only clrun is packaged, making a note in the %descr about
> it. If/when someone else needs more packaged it's just to fix, but without
> changing the package name.

Good point, that would make more sense, thanks! :)

Comment 4 Jeremy Newton 2012-04-05 16:24:44 UTC
Here's the new files, though it's still a work in progress as I have to fix a no-ldconfig-symlink and a debuginfo-without-sources error.

SPEC:
http://dl.dropbox.com/u/42480493/opencl-utils.spec
SRPM:
http://dl.dropbox.com/u/42480493/opencl-utils-0-2.svn16.fc16.src.rpm

RPMLINT OUTPUT:

opencl-utils-CLRun.x86_64: E: no-ldconfig-symlink /usr/lib64/libclrun.so
opencl-utils-debuginfo.x86_64: E: debuginfo-without-sources

I'm going to need some insight on this on as I'm a little shaky on this, I plan to contact my sponsor if he can help.

opencl-utils-CLRun.x86_64: W: no-documentation
opencl-utils-CLRun-devel.x86_64: W: no-documentation
opencl-utils-devel.x86_64: W: no-documentation

http://code.google.com/p/opencl-utils/issues/detail?id=1

opencl-utils.src: W: invalid-url Source0: opencl-utils.tar.xz

This is a SVN checkout, not an issue

5 packages and 0 specfiles checked; 2 errors, 4 warnings.

Comment 5 Alec Leamas 2012-04-05 18:10:42 UTC
Referring to dolphin-emu, you can add a pkgconfig file without much effort. First, add the opencl-utils.pc file (SourceX):

prefix=@prefix@
includedir=${prefix}/include

Name: opencl-utils
Description:  OpenCL tools and utilities.
Version: @version@
Libs:  -lclrun???
Cflags: -I${includedir}

Then, in %prep:

sed -i -e 's/@version@/%{version}/' -e 's/@prefix@/%{prefix}/' %SOURCEX

Add a Requires: pkgconfig to the devel package, and installthe  file in %_libdir/pkgconfig. List the file in the devel package. Done!

Comment 6 Alec Leamas 2012-04-05 18:31:46 UTC
The soname stuff. First, add a new linker flag
-Wl,-soname=libclrun.so.%{version} to the  gcc link phase, which sets the soname on the library libclrun.

Now, if you define version to 0.0.16 you should rename the resulting library to
libclrun.so.0.0.16 i. e., libclrun.so.%{version}

You will need the links lbclrun.so.0.0, libclrun.so.0 and libclrun.so, all linked to libclrun.so.%{version}. These can be setup using following in %install:

sofile=libclrun.so.%{version}
mkdir -p %{buildroot}%{_libdir}
install -m 755 $sofile %{buildroot}%{_libdir}
cd %{buildroot}%{_libdir}
ln -sf $sofile ${sofile%%.*}
ln -sf $sofile ${sofile%%.*.*}
ln -sf $sofile ${sofile%%.*.*.*}

The libclrun.so (*.so) link belongs to the devel package, the rest (*.so.*)  goes to base package. Standard ldconfig %post and %postun scriptlets.

There are certainly other ways to do this. In particular, there's a debate on how to choose so-name. Since we havn't really any stable API:s here, I think this schema makes most sense, though.

Comment 7 Jeremy Newton 2012-04-06 03:18:25 UTC
Awesome! Thanks for your help :)

New files
SPEC:
http://dl.dropbox.com/u/42480493/opencl-utils.spec
SRPM:
http://dl.dropbox.com/u/42480493/opencl-utils-0-3.svn16.fc16.src.rpm

I just have to work out the debuginfo-without-sources issue

Comment 8 Alec Leamas 2012-04-06 07:54:05 UTC
Basically, its all about the Makefile. Although it's extremely small, it's broken: the dependencies are wrong, make clean is incomplete, it does not respect CFLAGS.

I attach a new Makefile. It's so small that a diff makes no sense. It can be used something like this:

env CFLAGS="%{optflags} -Wl,-soname=libcrun.so.%{version}" make

Doing so, the debuginfo thing should be solver as well as the missing %{optflags}

I'm still worried about the packages. Basically, this is just a library and as such the "standard procedure" would be:

Base package: opencl-utils: *.so.*, %doc e. g. the example files.
Devel package: the header files
debug packagge.

I just don't see why this should be different. Why package the complete sources? Why have a separate package for for clrun? In particular, packaging sources in the include directory is plain wrong IMHO. If they need to be packaged (why) they should live somewhere else e. g. in /usr/share/opencl-utils or just as %doc.

Comment 9 Alec Leamas 2012-04-06 07:56:00 UTC
Created attachment 575645 [details]
ALternative Makefile, clean-up, respects CFLAGS.

Comment 10 Jeremy Newton 2012-04-06 15:34:43 UTC
(In reply to comment #8)
> Basically, its all about the Makefile. Although it's extremely small, it's
> broken: the dependencies are wrong, make clean is incomplete, it does not
> respect CFLAGS.
> 
> I attach a new Makefile. It's so small that a diff makes no sense. It can be
> used something like this:
> 
> env CFLAGS="%{optflags} -Wl,-soname=libcrun.so.%{version}" make
> 
> Doing so, the debuginfo thing should be solver as well as the missing
> %{optflags}
> 
> I'm still worried about the packages. Basically, this is just a library and as
> such the "standard procedure" would be:
> 
> Base package: opencl-utils: *.so.*, %doc e. g. the example files.
> Devel package: the header files
> debug packagge.
> 
> I just don't see why this should be different. Why package the complete
> sources? Why have a separate package for for clrun? In particular, packaging
> sources in the include directory is plain wrong IMHO. If they need to be
> packaged (why) they should live somewhere else e. g. in /usr/share/opencl-utils
> or just as %doc.

I guess the non header files can be removed now that I don't need them anymore, I can agree in some sense to remove them.
As for the separate package for clrun, I'm only matching upstream's naming scheme. Sure it may not make sense but there doesn't seem to be any guideline speaking against it, and if anything the guidelines encourages to be as close to upstream as possible.
The organization seems to make sense:
CLRUN - just the library
CLRUN-devel - the devel files for CLRUN
devel - The files not specific to CLRUN

The main point is that if anyone decides to contribute another library to this package it should be fairly easy without having to move everything around.

(In reply to comment #9)
> Created attachment 575645 [details]
> ALternative Makefile, clean-up, respects CFLAGS.

Thanks a lot! I'll give it a shot :)

Comment 11 Jeremy Newton 2012-04-06 19:05:10 UTC
(In reply to comment #9)
> Created attachment 575645 [details]
> ALternative Makefile, clean-up, respects CFLAGS.

I dropped patch0 because it was giving compiling errors, though I still need to test if this causes adverse effects in functionality. As for the new makefile, though it works fairly well but I had to add -fPIC to the CFLAGS make it compile.

The issue is that I'm now getting the ldconfig error again:
>opencl-utils-CLRun.x86_64: E: no-ldconfig-symlink /usr/lib64/libclrun.so.0.16
my method of symlinking is still the same as in comment#7, which makes me think that the method to fix it originally may not be ideal. Not sure, I may have to do some more research into this.

This is a direct copy of from halfway through the %install section until right before the changelogs:

>#install the library and the links
>install -p -D -m 0755 src/clrun/libclrun.so %{buildroot}%{_libdir}/libclrun.so.%{version}.%{svnversion}
>cd %{buildroot}%{_libdir}
>ln -sf libclrun.so.%{version}.%{svnversion} libclrun.so.%{version}
>ln -sf libclrun.so.%{version}.%{svnversion} libclrun.so
>
>%files CLRun
>%{_libdir}/*.so.*
>
>%files devel
>%{_includedir}/%{name}/include/CL
>
>%files CLRun-devel
>%{_includedir}/%{name}/clrun/
>%{_includedir}/%{name}/include/clrun.h
>%{_datadir}/%{name}/examples
>%{_libdir}/*.so
>
>%post CLRun -p /sbin/ldconfig
>
>%postun CLRun -p /sbin/ldconfig

Did I miss something?

Comment 12 Alec Leamas 2012-04-07 07:41:00 UTC
Created attachment 575878 [details]
Patch from release 3 using the new Makefile, fixes most rpmlint warnings

Comment 13 Alec Leamas 2012-04-07 07:42:16 UTC
Created attachment 575879 [details]
My idea how to simplify the package structure.

Comment 14 Alec Leamas 2012-04-07 07:45:04 UTC
Created attachment 575880 [details]
Pkg-config, complete patch

Examples can now be built using 'cc $(pkg-config --cflags --libs) example1.c' and 'g++ $(pkg-config --cflags --libs) example2.cpp'

Comment 15 Alec Leamas 2012-04-07 08:34:38 UTC
Some notes: 
 - The -devel dependency should be  %{name}%{?_isa} = %{version}-%{release}
 - The %doc clause is probably better placed in the -devel package

rpmlint warnings from all packages (also installed) w all patches applied:
opencl-utils.src: W: invalid-url Source0: opencl-utils.tar.xz
opencl-utils.i686: W: wrong-file-end-of-line-encoding /usr/share/doc/opencl-utils-0/examples/clrun-example/example2.cpp
opencl-utils-devel.i686: W: no-documentation

Comment 16 Alec Leamas 2012-04-07 09:07:02 UTC
(In reply to comment #10)

> 
> The main point is that if anyone decides to contribute another library to this
> package it should be fairly easy without having to move everything around.

I don't really see this as an argument. To have a package with several libraries is not a problem,  so the simple "expand path" is to just add the library and the corresponding headers if need be. Together w the fact that this code seems to be dead since 2009, current package name complexity cannot really be justified IMHO.


But, of course, it's just IMHO.

Comment 17 Jeremy Newton 2012-04-08 15:45:58 UTC
Thanks for your help, your methods seemed to have helped get rid of those errors for whatever reason. No need to question what works. :)

Though I'm not sure how you got it to compile with the opencl-utils-correctOpenCLpointers.patch and that makefile. It still works fine without it so I see little reason for the patch. I believe this is only needed for windows 32bit anyway.

Anyway, I'm happy with my spec file now and it seems to be in working condition. Note that I only get the no-documentation and the invalid-url Source0 rpmlint warnings now.
I've also tested the functionality of the library and it seems fine.

New files
SPEC:
http://dl.dropbox.com/u/42480493/opencl-utils.spec
SRPM:
http://dl.dropbox.com/u/42480493/opencl-utils-0-4.svn16.fc16.src.rpm

Comment 18 Alec Leamas 2012-04-21 09:45:55 UTC
OK, starting review phase. Initial remarks, all based on "common sense" rather than specific rules in the guidelines:

- The package structure is not clear and needlessly complicated. It also doesn't reflect the upstream structure. Please merge this to a simple structure with a opencl-utils base package +  -devel and -debuginfo packages like in comment #13.

- The include files are installed in %{_libdir}/opencl-utils/include. This is not really suitable. Please install the files into %{_libdir}/opencl-utils instead.

- There is no information on how to use the package i. e., compilation and link flags. Please either add a pkg-config file according to comment #14 or a README.fedora which states the flags needed to compile and link using the package.

- The example files does not build, please update to fix. Note that if you go for the pkg-config path, the text in comment #14 is wrong. It should be

'cc $(pkg-config --cflags --libs opencl-utils) example1.c'
and 'g++ $(pkg-config --cflags --libs opencl-utils) example2.cpp'

I understand that from your perspective this is just a dependency which works as-is. However, the whole point with unbundling libraries is that they should be usable also to others. That's why I stress these issues.

Comment 19 Alec Leamas 2012-04-21 09:59:33 UTC
A followup: the build system is based on that the gencl.c and genclgl.c are generated from the system's /usr/include/CL/* files. However, you don't rebuild these and install what's distributed instead. Please add a 'make clean' before 'make' to handle this (like the patch in comment #12).

Comment 20 Alec Leamas 2012-04-22 12:53:15 UTC
A (hopefully last) yet another followup:

- For comment #18, do s/%{_libdir}/%{_includedir}/. How could I write that?
- Example files are documentation. Please install them in %{_docdir} instead of %{_datadir}/%{name}.

Comment 21 Jeremy Newton 2012-06-24 19:43:47 UTC
This should be it, please let me know if I need to fix anything else.

SPEC:
http://dl.dropbox.com/u/42480493/opencl-utils.spec
SRPM:
http://dl.dropbox.com/u/42480493/opencl-utils-0-5.svn16.fc16.src.rpm

It seems to introduce a new rpmlint warning, but it's negligible or a false positive (i.e. can be safely ignored):

opencl-utils.src:47: W: mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 47)
opencl-utils.src: W: invalid-url Source0: opencl-utils.tar.xz
opencl-utils.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 3 warnings.

Comment 22 Alec Leamas 2012-06-24 19:53:59 UTC
The srpm link is 404, the spec is fine. Is it not yet uploaded?

Comment 23 Jeremy Newton 2012-06-24 19:57:35 UTC
sorry, I made a typo: http://dl.dropbox.com/u/42480493/opencl-utils-0-5.svn16.fc17.src.rpm

Comment 24 Alec Leamas 2012-06-24 20:37:46 UTC
Package Review
==============
Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated

==== C/C++ ====
[x]: MUST Header files in -devel subpackage, if present.
[x]: MUST ldconfig called in %post and %postun if required.
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Development (unversioned) .so files in -devel subpackage, if
     present.


==== Generic ====
[x]: EXTRA Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: EXTRA Spec file according to URL is the same as in SRPM.
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[-]: MUST Package contains desktop file if it is a GUI application.
[x]: MUST Development files must be in a -devel package
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[ ]: MUST Package complies to the Packaging Guidelines
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[-]: MUST Large documentation files are in a -doc subpackage, if required.
[-]: 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 is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "*No copyright* UNKNOWN", "*No copyright* GENERATED FILE", "MIT/X11 (BSD
     like)" For detailed output of licensecheck see file:
     /home/leamas/FedoraReview/src/810059-opencl-utils/licensecheck.txt
[x]: MUST License file installed when any subpackage combination is installed.
[x]: MUST Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: MUST Package is named using only allowed ascii characters.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Package is not relocatable.
[x]: MUST Package requires pkgconfig, if .pc files are present. (EPEL5)
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains systemd file(s) if in need.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[!]: 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.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[?]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[!]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[x]: SHOULD The placement of pkgconfig(.pc) files are correct.
[x]: SHOULD Scriptlets must be sane, if used.
[!]: SHOULD SourceX / PatchY prefixed with %{name}.
     Note: Source1: README.fedora (README.fedora)
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[!]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
=======

Basically, this looks fine. Some minor nit-picking:

[!]  Although I'm flattered, the patch needs a better justification
     or upstream link in it's comment :)
[!]  Inform upstream about the missing license file.
[!]  See comment below on rpmlint warning.



Rpmlint
-------
Checking: opencl-utils-0-5.svn16.fc15.x86_64.rpm
          opencl-utils-devel-0-5.svn16.fc15.x86_64.rpm
          opencl-utils-debuginfo-0-5.svn16.fc15.x86_64.rpm
          opencl-utils-0-5.svn16.fc15.src.rpm
opencl-utils.x86_64: W: no-documentation

opencl-utils.src:48: W: mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 48)
--> because sed commands in spec file contains tabs. You would be 
better off using \t IMHO. If you keep the tabs, please add a comment, since
preserving tabs is a problem.

opencl-utils.src: W: invalid-url Source0: opencl-utils.tar.xz
4 packages and 0 specfiles checked; 0 errors, 3 warnings.


Rpmlint (installed packages)
----------------------------
# rpmlint opencl-utils-debuginfo opencl-utils-devel opencl-utils
opencl-utils-debuginfo.x86_64: I: enchant-dictionary-not-found en_US
opencl-utils.x86_64: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'

Requires
--------
opencl-utils-0-5.svn16.fc15.x86_64.rpm (rpmlib, GLIBC filtered):
    
    /sbin/ldconfig  
    libc.so.6()(64bit)  
    libdl.so.2()(64bit)  
    rtld(GNU_HASH)  

opencl-utils-devel-0-5.svn16.fc15.x86_64.rpm (rpmlib, GLIBC filtered):
    
    /usr/bin/pkg-config  
    libclrun.so.0.16()(64bit)  
    opencl-utils(x86-64) = 0-5.svn16.fc15
    pkgconfig  

opencl-utils-debuginfo-0-5.svn16.fc15.x86_64.rpm (rpmlib, GLIBC filtered):
    

Provides
--------
opencl-utils-0-5.svn16.fc15.x86_64.rpm:
    
    libclrun.so.0.16()(64bit)  
    opencl-utils = 0-5.svn16.fc15
    opencl-utils(x86-64) = 0-5.svn16.fc15

opencl-utils-devel-0-5.svn16.fc15.x86_64.rpm:
    
    opencl-utils-devel = 0-5.svn16.fc15
    opencl-utils-devel(x86-64) = 0-5.svn16.fc15
    pkgconfig(opencl-utils) = 0

opencl-utils-debuginfo-0-5.svn16.fc15.x86_64.rpm:
    
    opencl-utils-debuginfo = 0-5.svn16.fc15
    opencl-utils-debuginfo(x86-64) = 0-5.svn16.fc15

MD5-sum check
-------------
Fails, svn standard problem. diff -r is OK.


Generated by fedora-review 0.2.0git 313f811 2012-06-24 20:30:25 +0200 on hemulen.localdomain
External plugins:

Comment 25 Alec Leamas 2012-06-24 21:01:54 UTC
Always something forgotten... please add -a to the cp commands undr %install in order to preserve timestamps.

Comment 26 Jeremy Newton 2012-06-24 23:04:57 UTC
Some comments:

- The license issue has been posted upstream: http://code.google.com/p/opencl-utils/issues/detail?id=1

- I posted the patch upstream as well, I'll add the link to the spec: http://code.google.com/p/opencl-utils/issues/detail?id=2

- I wasn't aware that sed could use \t. After a little bit of research and trial and error, \\t did the trick.

- I repulled/redownloaded the source just to be sure.

Thanks!

SPEC:
http://dl.dropbox.com/u/42480493/opencl-utils.spec
SRPM:
http://dl.dropbox.com/u/42480493/opencl-utils-0-6.svn16.fc16.src.rpm

Comment 27 Jeremy Newton 2012-06-24 23:09:06 UTC
I made the same typo again, my bad :P

http://dl.dropbox.com/u/42480493/opencl-utils-0-6.svn16.fc17.src.rpm

Comment 28 Alec Leamas 2012-06-24 23:11:50 UTC
You're welcome :)

*** Approved

Comment 29 Jeremy Newton 2012-06-24 23:36:03 UTC
New Package SCM Request
=======================
Package Name: opencl-utils
Short Description: Useful OpenCL tools and utilities
Owners: mystro256
Branches: f17 devel
InitialCC:

Comment 30 Gwyn Ciesla 2012-06-26 14:34:05 UTC
Git done (by process-git-requests).

Comment 31 Jeremy Newton 2012-06-26 16:43:52 UTC
Thanks

Comment 32 Fedora Update System 2012-06-26 17:32:06 UTC
opencl-utils-0-7.svn16.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/opencl-utils-0-7.svn16.fc17

Comment 33 Fedora Update System 2012-06-28 03:31:23 UTC
opencl-utils-0-7.svn16.fc17 has been pushed to the Fedora 17 stable repository.


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