Hi Till, I'm looking to get sponsored and your package has raised a few questions for me on my informal review. I'm pretty sure I'm right in saing you don't need prefix={_prefix} in install and configure. However I question if you do need a .desktop file or not considering that the target interface for this is plain WMs not DEs, it is at least something to think about though.

Jussi I hope you can provide some guidance on this and tell me that I am maybe reading too much into the matter of the .desktop file.


* 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....NEEDSWORK
	- Not sure if these apps are considered GUI as they would run in a DE. They may need a .desktop file I'm not too sure on this but its a thought.


* 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....NEEDSWORK
	- Not sure if you need to use prefix in the configure and install sections as the {_prefix} macro is automatically appended to the build, even if source default is /usr/local/ it doesn't end there. At least from the package I built that defaults to /usr/local/ this is true. 

* MUST: rpmlint must be run on every package. The output should be posted in the review....OK
* 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....OK
* MUST: The package must meet the Packaging Guidelines....OK
* MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines....OK
* 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....OK
* MUST: The spec file must be written in American English....OK
* MUST: The spec file for the package MUST be legible....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
* MUST: The package MUST successfully compile and build into binary rpms on at least one primary 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 MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line.....OK
* MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the 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....N/A
* MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun....N/A
* 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....OK
* MUST: A Fedora package must not list a file more than once in the spec file's %files listings....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....OK
* MUST: The package must contain code, or permissable content....OK
* MUST: Large documentation files must 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)....N/A
* 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....N/A
* MUST: Static libraries must be in a -static package....N/A
* MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability)....N/A
* 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....N/A
* MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}....N/A
* MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built....OK
* 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)....OK
* MUST: All filenames in rpm packages must be valid UTF-8....OK

Thx for your preliminary review.

(In reply to comment #1)

> * 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....NEEDSWORK
>  - Not sure if these apps are considered GUI as they would run in a DE. They
> may need a .desktop file I'm not too sure on this but its a thought.
 
I can add a comment, why there is no .desktop file. It does not make sense to have one, because dzen2 needs to get data from stdin, which cannot be passed if it is run from a .desktop file.
 
> * 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....NEEDSWORK

I do not use Prefix: /usr in the spec. This would be a Tag like "Name: dzen2" or "Version 0.8.5" in the beginning of the spec file.

>  - Not sure if you need to use prefix in the configure and install sections as
> the {_prefix} macro is automatically appended to the build, even if source
> default is /usr/local/ it doesn't end there. At least from the package I built
> that defaults to /usr/local/ this is true. 

I am very sure that prefix is needed at least in %install, because it does not use any special build system like autotools oder cmake. There the path is passed to the makefiles using the %configure or %cmake macro. But here only plain make files are used.

(In reply to comment #2)
> >  - Not sure if you need to use prefix in the configure and install sections as
> > the {_prefix} macro is automatically appended to the build, even if source
> > default is /usr/local/ it doesn't end there. At least from the package I built
> > that defaults to /usr/local/ this is true. 
> 
> I am very sure that prefix is needed at least in %install, because it does not
> use any special build system like autotools oder cmake. There the path is
> passed to the makefiles using the %configure or %cmake macro. But here only
> plain make files are used.  

Some programs need to set the prefix also in the build phase, if paths are hard coded with definitions in the source code. This is not the case here. Use just

export CFLAGS="%{optflags}"
export LDFLAGS="%{optflags}"
make %{?_smp_mflags} DZEN_XINERAMA=1 DZEN_XPM=1
make -C gadgets %{?_smp_mflags}

in %build and

rm -rf $RPM_BUILD_ROOT
make install DESTDIR=$RPM_BUILD_ROOT PREFIX=%{_prefix}
make -C gadgets install DESTDIR=$RPM_BUILD_ROOT PREFIX=%{_prefix}

in %install.

rpmlint output is clean.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK

MUST: The spec file for the package is legible and macros are used consistently. NEEDSWORK
- Check comment #3.

MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. OK
- The install target of the makefiles is a bit oddly written (uses 'cp' and 'chmod' instead if 'install').
- All files that are copied by the makefile have been compiled in %build and thus don't have a time stamp worth preserving.

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A

MUST: Desktop files are installed properly. N/A
- Program works with stdin, not applicable.

MUST: No file conflicts with other packages and no general names. NEEDSWORK
- The files are
 %{_bindir}/dzen2
 %{_bindir}/dbar
 %{_bindir}/gcpubar
 %{_bindir}/gdbar
 %{_bindir}/textwidth
and what troubles me most is 'textwidth'. I'm afraid it will have to be changed before this package can be included in Fedora.

MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK

SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. NEEDSWORK
- License text is not included in the tarball, please ask upstream to include it.

SHOULD: The package builds in mock. OK

Spec URL: http://till.fedorapeople.org/review/dzen2.spec
SRPM URL: http://till.fedorapeople.org/review/dzen2-0.8.5-4.fc10.src.rpm

- use make -C instead of pushd/popd
- add manpages from debian
- prefix gadgets with dzen2 like debian does it
- export LDFLAGS
- explain missing .desktop file

(In reply to comment #4)

> SHOULD: If the package does not include license text(s) as separate files from
> upstream, the packager should query upstream to include it. NEEDSWORK
> - License text is not included in the tarball, please ask upstream to include
> it.

I guess you did not see the file LICENSE that is also included in %doc.

> (In reply to comment #4)
> 
> > SHOULD: If the package does not include license text(s) as separate files from
> > upstream, the packager should query upstream to include it. NEEDSWORK
> > - License text is not included in the tarball, please ask upstream to include
> > it.
> 
> I guess you did not see the file LICENSE that is also included in %doc.  

Whoops, sorry.

**

If the binaries work in Debian with just a brute-force rename, then it isn't an issue in Fedora. (No interlinks between them and so on.)

**

You can still drop PREFIX=%{_prefix} from the build phase, it's only used by the install target of the makefile. Do before import to CVS. This package has been

APPROVED

Thank you both for the review. I'll remove the extra PREFIX before import.

New Package CVS Request
=======================
Package Name: dzen2
Short Description: 
Owners: till
Branches: F-10 F-11 EL-5
InitialCC: A general purpose messaging and notification program

It looks like you mixed up "Short Description" and "InitialCC".  I've made the assumption that you wanted InitialCC to be empty.

CVS done.

dzen2-0.8.5-5.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/dzen2-0.8.5-5.el5

dzen2-0.8.5-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/dzen2-0.8.5-5.fc11

dzen2-0.8.5-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/dzen2-0.8.5-5.fc10

(In reply to comment #8)
> It looks like you mixed up "Short Description" and "InitialCC".  I've made the
> assumption that you wanted InitialCC to be empty.

Yes, thank you and everyone else who helped here.

dzen2-0.8.5-5.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update dzen2'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/EL-5/FEDORA-EPEL-2009-0383

dzen2-0.8.5-5.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update dzen2'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-9241

dzen2-0.8.5-5.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update dzen2'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-9252

dzen2-0.8.5-5.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

dzen2-0.8.5-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

dzen2-0.8.5-5.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.