Taking the review.

* One of the files is distributed under the MIT/X license. Please, correct the License tag to "BSD and MIT".
* Respect Fedora %optflags/$RPM_OPT_FLAGS.
* Use %global instead of %define.

The rest of the package looks good to me.

All three remarks fixed plus after discussion with upstream I slightly changed description texts:

https://github.com/lzap/spec_reviews/commit/79d5d2862ebcd13a1c38cf199bf049c31d9730fe

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

Thanks Petr.

What does the STATIC variable do?
From a brief look, it doesn't seem to affect CFLAGS at all.

It's unused by default, I have decided to leverage it until there is a new version with dedicated variables for packagers.

config.mk:

CFLAGS   += -g --std=c99 -pedantic -Wall -Wno-overlength-strings -Os ${INCS} ${STATIC} ${CPPFLAGS}

I see it's not used: http://kojipkgs.fedoraproject.org//work/tasks/3526/4433526/build.log Investigating.

I usually just patch config.mk to suit Fedora needs.

I really do not understand why rpmbuild is quoting the parameter. Tried several scenarios, without any luck. I will need to patch it.

Giving up the review since I won't be available for some time starting now.

From my point of view, the package is okay except for the optflags issue.  Once it gets fixed, the package can be approved.

Ok I created a patch that respects fedora CFLAGS:

https://github.com/lzap/spec_reviews/commit/5772d10508c16094089a1047ade42dde8e067055

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

ANYONE PLEASE FINISH THE REVIEW ^^^ Thanks.

Ok, I can take it.

Can you post newer srpm and spec with the various changes ( ease my work with fedora review )?

So far, i found there is a unowned directory :
%{_datadir}/%{name}

And shouldn't it be started when X start, or this is done "on demand" by dbus, with the proper X cookie being set ?


Also, could the description be improved, since I have no idea of what it does exactly :/ ( I see it display message, but do not know what is dmenu )

Changes made:

https://github.com/lzap/spec_reviews/commit/663c81793a2c8dde06e577044c83ca742fa0a4d1

Links:
https://github.com/lzap/spec_reviews/raw/master/dunst.spec
http://kojipkgs.fedoraproject.org//work/tasks/7917/4447917/dunst-0.3.1-3.fc17.src.rpm

I don't thing we want to start it by default, but what could we do is to add it in the afterstart gnome menu (disabled by default).

The srpm si no longer here, can you host it somewhere else ?

Sure, sorry about that:

http://lzap.fedorapeople.org/fedora-packaging/dunst/0.3.1-3/dunst-0.3.1-3.fc17.src.rpm

http://lzap.fedorapeople.org/fedora-packaging/dunst/0.3.1-3/dunst-0.3.1-3.fc17.x86_64.rpm

http://github.com/lzap/spec_reviews/raw/master/dunst.spec

New release 0.4.0 is available. I will bump the package version once it is approved.

Message from the upstream author:

Since dunst has lost its ability to show notifications independendly of
dbus/libnotify a long time ago I think it is time that the describtion reflects
that. Even though this breaks the acronym. So if you're a packager please update
the package description to read something like:

short:
"Dunst - a dmenu-ish notification-daemon"

long:
"Dunst is a highly configurable and lightweight notification-daemon"

Release Tarballs are now available at:
http://www.knopwob.org/public/dunst-release/

For more information have a look at the CHANGELOG and the new options
in dunstrc.

There is bundled library :
http://code.google.com/p/inih/

( and I think draw.h come from dmenu as well ). 

Could you please open a ticket with fesco to have a execption for this ? ( as I cannot approve it without this, I guess given the size of the library, the expection should be granted )

Package Review
==============

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed

Package issues:

[!]: Package contains no bundled libraries.
There is a bundled copy of inih library 



===== MUST items =====

C/C++:
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[!]: Package contains no bundled libraries.
[x]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package complies to the Packaging Guidelines
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: 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]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "BSD (3 clause)", "MIT/X11 (BSD like)", "Unknown or generated". 3 files
     have unknown license. Detailed output of licensecheck in
     /home/misc/checkout/git/FedoraReview/852211-dunst/licensecheck.txt
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[-]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
[x]: Package is named using only allowed ASCII characters.
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: Package do not use a name that already exist
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package installs properly.
[x]: Package is not relocatable.
[x]: Requires correct, justified where necessary.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file is legible and written in American English.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: Package contains systemd file(s) if in need.
[x]: File names are valid UTF-8.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 20480 bytes in 4 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[-]: 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]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[-]: Package functions as described.
[-]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX tarball generation or download is documented.
[!]: SourceX / PatchY prefixed with %{name}.
     Note: Source0 (knopwob-dunst-v0.3.1-0-g63ceed3.tar.gz)
[x]: SourceX is a working URL.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.


Rpmlint
-------
Checking: dunst-debuginfo-0.3.1-3.fc17.x86_64.rpm
          dunst-0.3.1-3.fc17.x86_64.rpm
          dunst-0.3.1-3.fc17.src.rpm
dunst.x86_64: W: spelling-error %description -l en_US dmenu -> menu, d menu, madmen
dunst.src: W: spelling-error %description -l en_US dmenu -> menu, d menu, madmen
3 packages and 0 specfiles checked; 0 errors, 2 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint dunst-debuginfo dunst
dunst.x86_64: W: spelling-error %description -l en_US dmenu -> menu, d menu, madmen
2 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'



Requires
--------
dunst-debuginfo-0.3.1-3.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    

dunst-0.3.1-3.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    
    dbus  
    libX11.so.6()(64bit)  
    libXft.so.2()(64bit)  
    libXinerama.so.1()(64bit)  
    libXss.so.1()(64bit)  
    libc.so.6()(64bit)  
    libdbus-1.so.3()(64bit)  
    libpthread.so.0()(64bit)  
    librt.so.1()(64bit)  
    libxdg-basedir.so.1()(64bit)  
    rtld(GNU_HASH)  



Provides
--------
dunst-debuginfo-0.3.1-3.fc17.x86_64.rpm:
    
    dunst-debuginfo = 0.3.1-3.fc17
    dunst-debuginfo(x86-64) = 0.3.1-3.fc17

dunst-0.3.1-3.fc17.x86_64.rpm:
    
    dunst = 0.3.1-3.fc17
    dunst(x86-64) = 0.3.1-3.fc17



MD5-sum check
-------------
http://github.com/knopwob/dunst/tarball/v0.3.1/knopwob-dunst-v0.3.1-0-g63ceed3.tar.gz :
  CHECKSUM(SHA256) this package     : 1e3acc788b47b1abd7f67938d45469ac62c2a415a0d653c47a52131b1b2a95ad
  CHECKSUM(SHA256) upstream package : 1e3acc788b47b1abd7f67938d45469ac62c2a415a0d653c47a52131b1b2a95ad


Generated by fedora-review 0.2.0 (Unknown) last change: Unknown
Buildroot used: fedora-17-x86_64
Command line :./try-fedora-review -b 852211

Thank you, submitting https://fedorahosted.org/fesco/ticket/955

What is your recommendation for the package name? Github is a big pain for packagers, it is formatting tarballs as username-project-hash.tar.gz.

Should I create a script for getting the tarball and repack it? I asked about standard way on the channel and every packager has a different approach or recommendation.

Oh I filed into the wrong trac. Filing as https://fedorahosted.org/fpc/ticket/216 now.

the guideline for tarball are here :
http://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control

as long as you document it, this should be ok.

Created subpackage - small static library for INI parsing. Going to incorporate the changes with upstream:

https://bugzilla.redhat.com/show_bug.cgi?id=865116

BTW, in the meantime we've got a new version 0.4.0:
http://www.knopwob.org/public/dunst-release/dunst-0.4.0.tar.bz2

Will do bump, but I need the review of the INIH library first ;-) ^^^

Ok I am bumping version to 0.5.0. Changes:

- no inih static library dependency anymore
- now proper release (no more github snapshot)
- new features :-)

Please re-review. Thank you!

Is anyone working on this review?

I do not have time to re-review it for now, so you can take it. I basically already checked most issues in comment #17, so that should be fast.

OK I'm taking this. Lukas, can you post a link to your latest SRPM? I don't see one for 0.5.0.

Andrew: Sure, I apologize for the delay:

http://lzap.fedorapeople.org/fedora-packaging/dunst/0.5.0-1/

http://lzap.fedorapeople.org/fedora-packaging/dunst/0.5.0-1/dunst-0.5.0-1.f15.src.rpm

(Pls ignore the disttag it just wrong)

Test build in koji failed with:

+ make -j5 VERSION=0.5.0 PREFIX=/usr 'EXTRACFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic'
pod2man --name=dunst -c "Dunst Reference" --section=1 --release=0.5.0 README.pod > dunst.1
dunst build options:
creating config.h from config.def.h
/bin/sh: pod2man: command not found
make: *** [dunst.1] Error 127

(Full log: http://kojipkgs.fedoraproject.org//work/tasks/7017/5057017/build.log )

So I've gone through all the review points and the only issue remaining is the build failure in comment #29. Once that's fixed I think this review will likely be over and done with.

Right, I was missing a build require:

https://koji.fedoraproject.org/koji/taskinfo?taskID=5084022

https://github.com/lzap/spec_reviews/commit/3b05d0ac42433e0c7a04443d24ddd4b30e58c299

===== MUST items =====

C/C++:
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package contains no bundled libraries.
[x]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
[-]: Macros in Summary, %description expandable at SRPM build time.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package complies to the Packaging Guidelines
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: 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]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "MIT/X11 (BSD like)", "Unknown or generated". 2 files have unknown
     license. Detailed output of licensecheck in
     /home/andy/review/852211-dunst/licensecheck.txt
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[-]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
[x]: Package is named using only allowed ASCII characters.
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: Package do not use a name that already exist
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package installs properly.
[x]: Package is not relocatable.
[x]: Requires correct, justified where necessary.
[x]: CheckResultdir
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file is legible and written in American English.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: Package contains systemd file(s) if in need.
[x]: File names are valid UTF-8.
[x]: Useful -debuginfo package or justification otherwise.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 20480 bytes in 4 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[-]: 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]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[ ]: Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[-]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX / PatchY prefixed with %{name}.
[x]: SourceX is a working URL.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.


Rpmlint
-------
Checking: dunst-0.5.0-1.fc18.x86_64.rpm
          dunst-debuginfo-0.5.0-1.fc18.x86_64.rpm
          dunst-0.5.0-1.fc18.src.rpm
dunst.x86_64: W: spelling-error %description -l en_US dmenu -> menu, d menu, madmen
dunst.src: W: spelling-error %description -l en_US dmenu -> menu, d menu, madmen
3 packages and 0 specfiles checked; 0 errors, 2 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint dunst dunst-debuginfo
dunst.x86_64: W: spelling-error %description -l en_US dmenu -> menu, d menu, madmen
2 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'



Requires
--------
dunst-0.5.0-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered):
    
    dbus
    libX11.so.6()(64bit)
    libXext.so.6()(64bit)
    libXft.so.2()(64bit)
    libXinerama.so.1()(64bit)
    libXss.so.1()(64bit)
    libc.so.6()(64bit)
    libdbus-1.so.3()(64bit)
    libfreetype.so.6()(64bit)
    libm.so.6()(64bit)
    libxdg-basedir.so.1()(64bit)
    rtld(GNU_HASH)

dunst-debuginfo-0.5.0-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered):
    



Provides
--------
dunst-0.5.0-1.fc18.x86_64.rpm:
    
    dunst = 0.5.0-1.fc18
    dunst(x86-64) = 0.5.0-1.fc18

dunst-debuginfo-0.5.0-1.fc18.x86_64.rpm:
    
    dunst-debuginfo = 0.5.0-1.fc18
    dunst-debuginfo(x86-64) = 0.5.0-1.fc18



MD5-sum check
-------------
http://www.knopwob.org/public/dunst-release/dunst-0.5.0.tar.bz2 :
  CHECKSUM(SHA256) this package     : 4be8cf366930b297f03078f233f2fec479638d0c69b0e5e75bb49da487684b05
  CHECKSUM(SHA256) upstream package : 4be8cf366930b297f03078f233f2fec479638d0c69b0e5e75bb49da487684b05

Congratulations on review+, finally ;)

Thank you Andrew!

Thanks, we are not there yet :-)

New Package SCM Request
=======================
Package Name: dunst
Short Description: Lightweight d-bus notification daemon
Owners: lzap
Branches: f18 f19
InitialCC:

Git done (by process-git-requests).

rawhide and f19

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