Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 725228 - Review Request: qcodeedit - Qt-Framework for code editing
Summary: Review Request: qcodeedit - Qt-Framework for code editing
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: kde-reviews 724878
TreeView+ depends on / blocked
 
Reported: 2011-07-24 12:06 UTC by hannes
Modified: 2011-07-26 13:39 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-07-26 13:39:52 UTC
Type: ---
Embargoed:
martin.gieseking: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description hannes 2011-07-24 12:06:30 UTC
Spec URL: http://hannes.fedorapeople.org/qcodeedit.spec
SRPM URL: http://hannes.fedorapeople.org/qcodeedit-2.2.3-1.fc15.src.rpm
Description: 
QCodeEdit is a framework designed to make edition of source
code easy for both users and developers.
It is written in C++ using the Qt 4 framework which make it cross-platform.
QCodeEdit has been designed for three main purposes:
- efficiency
- flexibility
- ease of use
QCodeEdit is much more lightweight and significantly faster than 
QTextEdit/QTextDocument. 
However it also has different goals and thus limitations. QCodeEdit is
meant to work with source files and not any kind of rich 
text like QTextEdit handles so well.

Comment 1 hannes 2011-07-24 15:56:58 UTC
I am not 100% sure if the current state with those subpackages is really appropriate especially because all resulting rpms do have a different name starting with lib%{name}-{devel,designer} and the debug package is called qcodeedit-debug. Not really a great solution I think.

Don't know what's more appropriate. Would be glad if someone with more experience could help me on this issue.

Comment 2 Martin Gieseking 2011-07-24 19:09:10 UTC
Just a couple of quick comments:

- adapt Source0 according to 
  http://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net

- Drop BR: gcc-c++. It's present by default
  http://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2

- There's no need to prefix the packages with "lib". Just put the library into
  the base package and the devel files into %{name}-devel

- Is it necessary to call qmake-qt4 %{name}.pro twice in %build?

- Install the shared library files libqcodeedit.so* directly into %{_libdir}

- The designer subpackage should require qt-devel for proper directory
  ownership. Maybe it would also make sense to move the plugin to the devel
  package instead of package it separately.

- The devel package should also require qt-devel which provides directory 
  %{_libdir}/qt4/mkspecs/features/

- The devel package must require the base package this way
  Requires: %{name}%{?_isa} = %{version}-%{release}
  The %{?_isa} part has been added to the guidelines lately:
  http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

- You can drop Requires: pkgconfig. It's added automatically if necessary
  (still required for EPEL < 6, though).

- Add GPL.txt to the base package.

- The tarball contains a Doxyfile. Maybe it's a good idea to build the doxygen
  (html) documentation and add it to the devel package (or a separate doc
  package).

Comment 3 hannes 2011-07-25 06:06:38 UTC
Alright thanks for the suggestions. I fixed all issues you found except I didn't remove the -designer subpackage, since I don't think the -devel package is more appropriate.

SPEC-URL: http://hannes.fedorapeople.org/qcodeedit.spec
SRPM-URL: http://hannes.fedorapeople.org/qcodeedit-2.2.3-2.fc15.src.rpm

rpmlint qcodeedit.spec 
qcodeedit.spec: W: invalid-url Source0: http://downloads.sourceforge.net/qcodeedit/2.2.3/qcodeedit-2.2.3.tar.gz HTTP Error 404: Not Found
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

Comment 4 Martin Gieseking 2011-07-25 07:59:38 UTC
I had a deeper look at the package and it looks almost fine. There are yet a few small things to be fixed:

- The URL given in Source0 is invalid. Change it to
  http://downloads.sourceforge.net/edyuk/%{name}-%{version}.tar.gz

- Fix the file permissions that rpmlint complains about (see below)
  * add chmod 644 README.txt to %prep
  * add chmod 755 %{buildroot}%{_libdir} to %install

- Add qt-devel to the devel package (see comment #2).

- Drop Requires: pkgconfig as there's no .pc file in -devel.

- Replace $RPM_BUILD_ROOT with %{buildroot} to use macros consistently.

- If you don't plan to build the package for EPEL < 6 too, you can drop 
  rm -rf %{buildroot} from install. Otherwise, add a BuildRoot field and a
  %clean section. These are still required for EPEL 4 and 5.
  
- You can drop option -p from "cp" as option -a already includes -p 
  implicitly.

- rpmlint doesn't like non-devel packages requiring a devel package. If the
  designer subpackage is considered a devel package as well, this is probably 
  OK.
  

$ rpmlint *.rpm
qcodeedit.src: W: invalid-url Source0: http://downloads.sourceforge.net/qcodeedit/2.2.3/qcodeedit-2.2.3.tar.gz HTTP Error 404: Not Found
qcodeedit.x86_64: E: non-standard-executable-perm /usr/lib64/libqcodeedit.so.1.0.0 0775L
qcodeedit.x86_64: W: spurious-executable-perm /usr/share/doc/qcodeedit-2.2.3/README.txt
qcodeedit-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/qcodeedit-2.2.3/lib/.build
qcodeedit-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/qcodeedit-2.2.3/lib/.build
qcodeedit-designer.x86_64: E: devel-dependency qt-devel
qcodeedit-designer.x86_64: W: no-documentation
5 packages and 0 specfiles checked; 2 errors, 5 warnings.

The hidden files warnings can be ignored.

---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    - GPLv3 according to source headers

[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum qcodeedit-2.2.3.tar.gz*
    e2453d8e97c2592a870bbddd51876ad0  qcodeedit-2.2.3.tar.gz
    e2453d8e97c2592a870bbddd51876ad0  qcodeedit-2.2.3.tar.gz.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[.] MUST: If the package does not successfully compile, build or work ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[+] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied.
[.] MUST: The spec file MUST handle locales properly.
[+] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[X] MUST: Permissions on files must be set properly.
    - see rpmlint output
 
[X] MUST: Each package must consistently use macros.
    - replace $RPM_BUILD_ROOT with %{buildroot}

[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[+] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[+] MUST: If a package contains .so.* files, then .so files (without suffix) must go in a -devel package.
[+] MUST: devel packages must require the base package using a fully versioned dependency.
[+] MUST: Packages must NOT contain any .la libtool archives.
[.] 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. 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.
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

EPEL <= 5 only:
[X] MUST: The spec file must contain a valid BuildRoot field.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}.
[X] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}.
[X] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
    - no .pc file present => drop Requires: pkgconfig

[.] 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.
[+] SHOULD: Timestamps of files should be preserved.
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[+] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[+] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
[.] SHOULD: Your package should contain man pages for binaries/scripts.

Comment 5 hannes 2011-07-25 10:50:50 UTC
Thanks for your pretty fast and comprehensive review. I think I fixed all issues now and removed the -designer subpackage as well.

SPEC-URL: http://hannes.fedorapeople.org/qcodeedit.spec
SRPM-URL: http://hannes.fedorapeople.org/qcodeedit-2.2.3-3.fc15.src.rpm

Scratch Build in Koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3227198

rpmlint output:
rpmlint qcodeedit.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

rpmlint ../RPMS/x86_64/qcodeedit-2.2.3-3.fc15.x86_64.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

rpmlint ../RPMS/x86_64/qcodeedit-devel-2.2.3-3.fc15.x86_64.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

rpmlint ../RPMS/x86_64/qcodeedit-debuginfo-2.2.3-3.fc15.x86_64.rpm 
qcodeedit-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/qcodeedit-2.2.3/lib/.build
qcodeedit-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/qcodeedit-2.2.3/lib/.build
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

Comment 6 Rex Dieter 2011-07-25 11:55:47 UTC
The designer plugin ought to go in the main pkg, not -devel, as it can be loaded at runtime.

Comment 7 hannes 2011-07-25 12:07:37 UTC
Fixed. Should be fine now, or?

SPEC-URL: http://hannes.fedorapeople.org/qcodeedit.spec
SRPM-URL: http://hannes.fedorapeople.org/qcodeedit-2.2.3-4.fc15.src.rpm

Comment 8 Martin Gieseking 2011-07-25 13:26:12 UTC
I just checked the dependencies again, and noticed that you can drop Require: qt-devel from the base package. Package qt-x11 provides the plugin directory and libQtDesigner.so.4. Since the base package depends on libQtDesigner.so.4, there's no need for qt-devel here.

I also forgot to add the filenames in comment #4. The chmod statement in %install should look like this:
chmod 755 %{buildroot}%{_libdir}/lib%{name}.so.*

Comment 10 Martin Gieseking 2011-07-25 14:31:53 UTC
Not quite, sorry. You should drop Requires: qt-devel from the base package but keep it for -devel.

Comment 11 Rex Dieter 2011-07-25 14:38:07 UTC
And (with my qt maintainer hat on), deps of the form:
Requires: qt4-devel
or
Requires: pkgconfig(QtGui)

are preferable to
Requires: qt-devel

Comment 12 hannes 2011-07-25 18:43:54 UTC
SPEC-URL: http://hannes.fedorapeople.org/qcodeedit.spec
SRPM-URL: http://hannes.fedorapeople.org/qcodeedit-2.2.3-6.fc15.src.rpm

Ok, like this? Quite confusing ;-)

Comment 13 Martin Gieseking 2011-07-25 19:02:56 UTC
Yes. The package looks good now. :)

----------------
Package APPROVED
----------------

Comment 14 Rex Dieter 2011-07-25 19:08:58 UTC
I still see 1 small problem

$RPM_OPT_FLAGS aren't being used when building libqcodeedit due to it building
in debug mode by default.

I'd suggest removing the line
CONFIG += debug
from at least lib/lib.pro and perhaps from qcodeedit.pro too (the latter seems
to set _DEBUG_BUILD_ or _RELEASE_BUILD_ defines, but neither of these seem to
be used in code anywhere that I can tell).

Comment 15 hannes 2011-07-25 19:45:29 UTC
Ok removed these two lines? Is it ok like this?

SPEC-URL: http://hannes.fedorapeople.org/qcodeedit.spec
SRPM-URL: http://hannes.fedorapeople.org/qcodeedit-2.2.3-7.fc15.src.rpm

Comment 16 Martin Gieseking 2011-07-25 20:30:02 UTC
Yes, now the %optflags are present. Sorry for having overlooked this one.

Comment 17 hannes 2011-07-25 20:32:30 UTC
New Package SCM Request
=======================
Package Name: qcodeedit
Short Description: Qt-Framework for code editing
Owners: hannes
Branches: f14 f15
InitialCC:

Comment 18 Gwyn Ciesla 2011-07-26 00:07:59 UTC
Git done (by process-git-requests).

Comment 19 hannes 2011-07-26 13:39:52 UTC
Build in rawhide:
http://koji.fedoraproject.org/koji/buildinfo?buildID=255576


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