Full review below. Main points:
- Also call desktop-file-validate on qterminal_drop.desktop
- Download URL does not work? As a possibility, how I handled the github url for python-pillow: [1]
- (Optional) Comments on patch status?

Otherwise ok!

[1] http://pkgs.fedoraproject.org/cgit/python-pillow.git/tree/python-pillow.spec


-------------------------------
Full review:


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

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



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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[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]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[-]: 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 complies to the Packaging Guidelines
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v2 or later)", "Unknown or generated". 8 files have unknown
     license. Detailed output of licensecheck in
     /home/sandro/.Data/Desktop/960194-qterminal/licensecheck.txt
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[-]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[-]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[!]: update-desktop-database is invoked when required
     Note: desktop file(s) in qterminal
[x]: Useful -debuginfo package or justification otherwise.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 30720 bytes in 3 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[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]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

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

Generic:
[!]: Sources can be downloaded from URI in Source: tag
     Note: Could not download Source0
[-]: 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]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: 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.
[!]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[-]: 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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[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)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

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

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[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.


Rpmlint
-------
Checking: qterminal-0.4.0-1.fc20.x86_64.rpm
qterminal.x86_64: W: no-manual-page-for-binary qterminal
1 packages and 0 specfiles checked; 0 errors, 1 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint qterminal
qterminal.x86_64: W: no-manual-page-for-binary qterminal
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'



Requires
--------
qterminal (rpmlib, GLIBC filtered):
    libQtCore.so.4()(64bit)
    libQtGui.so.4()(64bit)
    libQxtCore.so.0()(64bit)
    libQxtGui.so.0()(64bit)
    libX11.so.6()(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libqtermwidget.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libutil.so.1()(64bit)
    rtld(GNU_HASH)



Provides
--------
qterminal:
    qterminal
    qterminal(x86-64)



Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -b 960194

Oh, I forgot: I've never seen %make_install. There is some reference to %makeinstall in the guidelines but it is discouraged. How different is %make_install to make install DESTDIR=%{buildroot} ?

(In reply to comment #1)
> Full review below. Main points:
> - Also call desktop-file-validate on qterminal_drop.desktop
Oops... Will be fixed, Thanks

> - Download URL does not work? As a possibility, how I handled the github url
No :-(
I did _exactly_ according to fedora guides. But developer don't want as push tarball as also make git tags.
I'm sorry :-(

> for python-pillow: [1]
> - (Optional) Comments on patch status?
???
You want python?
This is my 1st language - but I haven't experience to review pyhon packages yet.
If you want python - let's talk: http://download.opensuse.org/repositories/home:/TI_Eugene:/python/

Uhm? I only mentioned python-pillow to show an example of github urls which actually work, i.e. the part
----------
# Refer to the comment for Source0 below on how to obtain the source tarball
# The saved file has format python-imaging-Pillow-$version-$ahead-g$shortcommit.tar.gz
%global commit d1c6db88d4dee462c6bbf4e22555e3ddd410d06a
%global shortcommit %(c=%{commit}; echo ${c:0:7})
%global ahead 105
[...]
# Obtain the tarball for a certain commit via:
#  wget --content-disposition https://github.com/python-imaging/Pillow/tarball/$commit
Source0:        https://github.com/python-imaging/Pillow/tarball/%{commit}/python-imaging-Pillow-%{version}-%{ahead}-g%{shortcommit}.tar.gz
----------

Python does not matter at all here.

And yes, the github guidelines are not up-to-date anymore as far as I can tell.

(In reply to comment #2)
> Oh, I forgot: I've never seen %make_install. There is some reference to
> %makeinstall in the guidelines but it is discouraged. How different is
> %make_install to make install DESTDIR=%{buildroot} ?

%make_install is the sane version that we should be using, %makeinstall is the one that's discouraged. You can always check what a macro does by expanding it on the command line:

rpm -E %make_install
/usr/bin/make install DESTDIR=/home/kalev/rpmbuild/BUILDROOT/%{name}-%{version}-%{release}.x86_64

Thanks kalev.

(For the github guidelines: the URL did indeed change shortly after the guidelines were approved, see [1], so the guideline won't work anymore I'm afraid)

[1] https://bugzilla.redhat.com/show_bug.cgi?id=908114#c13

I've opened a ticket concerning the GitHub guidelines: https://fedorahosted.org/fpc/ticket/284

(In reply to comment #7)
> I've opened a ticket concerning the GitHub guidelines:
> https://fedorahosted.org/fpc/ticket/284

Das ist fantastisch!

Spec URL: http://tieugene.fedorapeople.org/rpms/qterminal/qterminal.spec
SRPM URL: http://tieugene.fedorapeople.org/rpms/qterminal/qterminal-0.4.0-2.fc18.src.rpm

Since the project has no tags, you want:

Source0: https://github.com/qterminal/qterminal/tarball/%{commit}/%{name}-%{name}-%{version}-%{shortcommit}.tar.gz

and you can remove %ahead.

And you forgot the second desktop-file-validate.


(Und ja, mal schaun was mit dem Ticket passiert ;))

Don't the guidelines say you should just spell out:
make install DESTDIR=%{buildroot}
?

(In reply to comment #10)
> Don't the guidelines say you should just spell out:
> make install DESTDIR=%{buildroot}
> ?

This is the same as %make_install

(In reply to comment #9)
> Source0:
> https://github.com/qterminal/qterminal/tarball/%{commit}/%{name}-%{name}-
> %{version}-%{shortcommit}.tar.gz

Yes, works too.

> And you forgot the second desktop-file-validate.

Oops... sorry.

Spec URL: http://tieugene.fedorapeople.org/rpms/qterminal/qterminal.spec
SRPM URL: http://tieugene.fedorapeople.org/rpms/qterminal/qterminal-0.4.0-3.fc18.src.rpm

Ok, got nothing more to say: Approved!

New Package SCM Request
=======================
Package Name: qterminal
Short Description: Qt-based terminal emulator.
Owners: tieugene
Branches: f17 f18 f19 el6
InitialCC:

Git done (by process-git-requests).

qterminal-0.4.0-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/qterminal-0.4.0-3.fc18

qterminal-0.4.0-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/qterminal-0.4.0-3.fc19

qterminal-0.4.0-3.fc19 has been pushed to the Fedora 19 testing repository.

qterminal-0.4.0-3.fc19 has been pushed to the Fedora 19 stable repository.

qterminal-0.4.0-3.fc18 has been pushed to the Fedora 18 stable repository.