A quick look at the spec file I noticed that you are not using a vendor for the
desktop-file-install command.  According to the Packaging guidelines
(http://fedoraproject.org/wiki/Packaging/Guidelines#desktop) you should use
"fedora" if upstream does not provide a vedor_id

Thank you for your look, I have made the changes needed, and I have also add the
strip -s command as I obtained a unstripped-binary-or-object error message with
rpmlint

The new src.rpm
http://pingoured.dyndns.org/public/RPM/SteGUI/SteGUI-0.0.1-6.fc6.src.rpm
The new spec file
http://pingoured.dyndns.org/public/RPM/SteGUI/SteGUI.spec

Thanks

Just a brief look at the spec, not at the src.rpm:

> Name:           SteGUI
> Summary:        SteGUI is a graphical front-end to Steghide
> Summary(fr):    SteGUI est un interface graphique pour Steghide

Repeating the package/software name in the summary is considered
bad taste often, since in many package database interfaces it
usually results in displaying the package %{name} multiple times, e.g.:
SteGUI - SteGUI is a graphical front-end to Steghide

> Requires:	fltk

A dependency on the fltk shared libs is automatically added by
rpmbuild. If not, then your package doesn't link against fltk
actually, and in that case you should add a comment in the spec file
as why it requires fltk.

> %install

Below this you're missing:  rm -rf $RPM_BUILD_ROOT

> strip -s $RPM_BUILD_ROOT%{_bindir}/%{name}

Don't strip binaries, since that makes the -debuginfo packages useless.

Based on your comment I changed the spec :-)

There are the new spec and src.rpm files:
SPEC: http://pingoured.dyndns.org/public/RPM/SteGUI/SteGUI.spec
SRPM: http://pingoured.dyndns.org/public/RPM/SteGUI/SteGUI-0.0.1-7.fc6.src.rpm

Thanks for your comment :-)

So, let's try building:

$ rpmlint SteGUI-0.0.1-7.fc6.src.rpm 
W: SteGUI strange-permission SteGUI.spec 0755
W: SteGUI mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 14)


* Package doesn't adhere to $RPM_OPT_FLAGS. The spec tries to modify
CXXFLAGS and CFLAGS, but the code compiles with different flags, e.g:

g++ -Wall -pedantic -O2 `fltk-config --cxxflags`   -c -o src/MsgWindow.o
src/MsgWindow.cc


$ desktop-file-validate /usr/share/applications/fedora-SteGUI.desktop
/usr/share/applications/fedora-SteGUI.desktop: warning: The 'Application'
category is not defined by the desktop entry specification.  Please use one of
"AudioVideo", "Audio", "Video", "Development", "Education", "Game", "Graphics",
"Network", "Office", "Settings", "System", "Utility" instead


Further: SteGUI in the "Office" category? Isn't "Utility"
more appropriate?

And in the menu entry "steGUI" is not spelled with upper-case "S".

Ok I think I corrected every things that you indicated to me :)

I removed the CFLAGS and CXXFLAGS to let the code compile with it owns

Here is the new spec:
http://pingoured.dyndns.org/public/RPM/SteGUI/SteGUI.spec

Here is the new sprm:
http://pingoured.dyndns.org/public/RPM/SteGUI/SteGUI-0.0.1-8.fc6.src.rpm

Thanks for your help :-)

This now fails to build for me:

+ desktop-file-install --vendor=fedora --dir
/var/tmp/SteGUI-0.0.1-8.fc8-root-mockbuild/usr/share/applications/ --mode 0644
/builddir/build/SOURCES/SteGUI.desktop
/var/tmp/SteGUI-0.0.1-8.fc8-root-mockbuild/usr/share/applications/fedora-SteGUI.desktop:
error: value "0.0.1" for key "Version" in group "Desktop Entry" is not a known
version
desktop-file-install created an invalid desktop file!
error: Bad exit status from /var/tmp/rpm-tmp.61612 (%install)

The Version key contains the version of the desktop entry specification that the
particular desktop file is compliant with.  It has nothing to do with the
version of your program.  If you're going to use Version at all, you probably
want to set it to "1.0" unless you're specifically following a different version
of the specification.

> I removed the CFLAGS and CXXFLAGS to let the code compile with it owns

Instead, you need to patch it to build with $RPM_BUILD_FLAGS or
find other ways to make it accept the custom CXXFLAGS passed in from
the outside. For example:

  make CXXFLAGS="$RPM_OPT_FLAGS" CFLAGS="$RPM_OPT_FLAGS" all

would work.

See comment 5, which shows an excerpt from the build log
where the code did not accept the flags yet. Here for clarity:

CFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protec
tor --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unw
ind-tables'
CXXFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-prot
ector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-u
nwind-tables'
[...]
make all
g++ -Wall -pedantic -O2 `fltk-config --cxxflags`   -c -o src/Callback.o src/Call
back.cc
[...]
gcc -O2   -c -o src/Player.o src/Player.c

OK
I changed the desktop file to a version 1.0
I change the make all to make CXXFLAGS="$RPM_OPT_FLAGS" CFLAGS="$RPM_OPT_FLAGS" all

rpmlint is clean here, and mock works (but I am not very good to find the error,
all that I saw is that the line before and after are different)

SPEC : http://pingoured.dyndns.org/public/RPM/SteGUI/SteGUI.spec
SPRM : http://pingoured.dyndns.org/public/RPM/SteGUI/SteGUI-0.0.1-9.fc6.src.rpm

Thank you for your help :-)

- License guideline has been updated -- license field now needs to be GPLv2+
- Please separate changelog entries with blank lines
- Would be nice to have an icon..

I added a blocker on FE-NEEDSPONSOR, hopefully a qualified developer will see
this request

Thanks for updating this review.

I have update the license tag (sorry for forgotten to do it before)
I have change the changelog
For the icon I use one present I believe by default on the system
Icon=/usr/share/icons/hicolor/48x48/apps/esc.png

Should I had it as source, and include it in the rpm ??

Regards

Here are the new spec and srpm

SPEC
http://www.pingoured.fr/public/RPM/SteGUI/SteGUI.spec
SRPM
http://www.pingoured.fr/public/RPM/SteGUI/SteGUI-0.0.1-10.fc6.src.rpm

>I added a blocker on FE-NEEDSPONSOR, hopefully a qualified developer will see
>this request
I have already been sponsored... I believe I do not need this qualification any
more...

Thanks for helping any way

Regards

That icon is actually from the 'esc' package -- so if you want to use it, you
should probably package it yourself. Probably need permission from the author
though. How about this image, from Wikipedia's Steganography article?

http://en.wikipedia.org/wiki/Image:Inkblot.svg

The advantage is that Wiki pictures are free of copyright restrictions so we're
safe using that.

You don't need to hardcode the path in Exec= and Icon= , so just

Exec=SteGUI
Icon=foo

is enough. Note that the extension for the icon should be left out as well, that
way if an icon theme comes along with, say, SVG icons, your desktop file does
not need updating (see http://fedoraproject.org/wiki/Packaging/Guidelines)

One last thing -- for Comment you probably want to say "Hides and extracts
information within files" (describes what the program does), and for
GenericName, "Steganography tool"? Not that GenericName is really used right now
(it might be used by the KDE panel, not sure).

Perhaps you could include French translations in the desktop file as well, to
match the French translations in the spec file?

Apart from that, ready to go. Pity that steghide itself is currently broken :(

I have made the changes that you advised me, the only one that I did not do is
the translation of the desktop file in french, I never did it.
How should I do it ?

Comment(fr)= ??
GenericName(fr)= ??

You can find the last Desktop file here :
http://www.pingoured.fr/public/RPM/SteGUI/SteGUI.desktop

Thanks

The GenericName and Comment fields are swapped. Concrete example:
Name=Firefox Web Browser
GenericName=Web Browser <= the product name has been removed
Comment=Browse the Web

So in case of SteGUI, the generic name can be just "Steganography tool" and the
comment explains what it does.

For desktop file translations, use:

Name[fr]=
GenericName[fr]=
Comment[fr]=

Also, you can leave out the extension when specifying the icon file in
SteGUI.desktop -- don't hardcode it.

Looks good -- upload the final RPM and I'll approve it.

Ok I have done the changes on the desktop file.

you can find the last version there :
SRPM
http://www.pingoured.fr/public/RPM/SteGUI/SteGUI-0.0.1-11.fc6.src.rpm
SPEC
http://www.pingoured.fr/public/RPM/SteGUI/SteGUI.spec
Desktop file
http://www.pingoured.fr/public/RPM/SteGUI/SteGUI.desktop

Thanks for your help :)

All good! The Name[fr] in desktop is not required if the name is identical, by
the way, so you might want to chop it off when committing the spec to CVS.

APPROVED

MUST

• rpmlint: OK
• package name: OK
• spec file name: OK
• package guideline-compliant: OK
• license complies with guidelines: OK
• license field accurate: OK
• license file not deleted: OK
• spec in US English: OK
• spec legible: OK
• source matches upstream: OK
• builds under >= 1 archs, others excluded: OK
• build dependencies complete: OK
• own all directories: OK
• no dupes in %files: OK
• permission: OK
• %clean RPM_BUILD_ROOT: OK
• desktop file uses desktop-file-install: OK
• clean buildroot before install: OK
• filenames UTF-8: OK

SHOULD
• desc and summary contain translations if available: OK
• package build in mock on all architectures: OK
• package functioned as described: OK
• require package not files: OK

New Package CVS Request
=======================
Package Name: SteGUI
Short Description: SteGUI is a graphical front-end to Steghide
Owners: pingoufc4
Branches: FC-6 F-7
InitialCC: 
Cvsextras Commits: yes

CVS done.

Note: Owners: should be your FAS ID, not your email address.  Fortunately I
remember what yours is.

I saw cvs commits for this package in fedora-extras-commits mailing list.
This mean package got built successfully for all requested branches.
Therefore,
closing this review ticket.

SteGUI-0.0.1-12.fc7 has been pushed to the Fedora 7 testing repository.  If problems still persist, please make note of it in this bug report.

SteGUI-0.0.1-12.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.