Bug 498413 - (vfrnav) Review Request: vfrnav - Flight Planning software for flights under Visual Flight Rules (VFR)
Review Request: vfrnav - Flight Planning software for flights under Visual Fl...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mattias Ellert
Fedora Extras Quality Assurance
:
Depends On: zfstream
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-30 08:01 EDT by Thomas Sailer
Modified: 2013-01-13 07:32 EST (History)
5 users (show)

See Also:
Fixed In Version: 0.3-8.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-06-19 09:34:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mattias.ellert: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Thomas Sailer 2009-04-30 08:01:58 EDT
Spec URL: http://sailer.fedorapeople.org/vfrnav.spec
SRPM URL: http://sailer.fedorapeople.org/vfrnav-0.3-2.fc11.src.rpm
Description: vfrnav is a flight planning and navigation application for flights under visual flight rules.
Comment 1 Igor Jurišković 2009-04-30 08:34:09 EDT
Hello Thomas,

this is not official review. I'm in need of sponsor but I can tell you what can be better or fixed.

- your Release: doesn't match changelog version.

- Group doesn't exist. You should use most appropriate Group available.
less /usr/share/doc/rpm-*/GROUPS* - will give you all possible groups.

- you dont need Require: gypsy. It will get pulled by gypsy-devel

- gcc-c++ is not needed. Read here why: http://fedoraproject.org/wiki/Packaging/Guidelines

- in %files section you can omit INSTALL. You are making installation so other users have no need to look into that file.

- NEWS is empty. You can remove it. Same goes for README 

- package doesn't own %{_datadir}/%{name} dir. Include it to %files section with %dir.

- You can speed up packaging by using asterix(*) to own all files in some directory. You can use this on
%{_datadir}/%{name}/* to own all files inside.


That's what I can see in fast - 10 minute - review.
Comment 2 Igor Jurišković 2009-04-30 08:37:00 EDT
Oops, forgot to tell you about licence. Its wrong. src file says GPLv2 or any later version. So you should change it from GPLv2 to GPLv2+
Comment 3 Thomas Sailer 2009-04-30 11:00:20 EDT
Thanks, Igor, for your comments!

I've updated the package:
Spec URL: http://sailer.fedorapeople.org/vfrnav.spec
SRPM URL: http://sailer.fedorapeople.org/vfrnav-0.3-3.fc11.src.rpm
Comment 4 Igor Jurišković 2009-04-30 15:46:02 EDT
No problem.

rpmlint says:
vfrnav.spec:32: W: non-standard-group Applications/Navigation

You should change that group too to something appropriate.

You can simplify this part:
%dir %{_datadir}/%{name}
%{_datadir}/%{name}/vfrnav.png
%{_datadir}/%{name}/bluetooth.png
%{_datadir}/%{name}/BlankMap-World_gray.svg
%{_datadir}/%{name}/dbeditor.glade
%{_datadir}/%{name}/navigate.glade
%{_datadir}/%{name}/routeedit.glade
%{_datadir}/%{name}/prefs.glade
%{_datadir}/%{name}/acftperformance.glade

Like this:
%dir %{_datadir}/%{name}
%{_datadir}/%{name}/*

- you are supplying desktop file but you are not installing it. http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

Desktop file supplied with vfrnav is weird - I never seen that kind of file. I would recommend you to make your own desktop file. All of this is explained at previous link.

- You are installing icons for your desktop file but you never update icon cache. https://fedoraproject.org/wiki/PackagingDrafts/Icon_Cache
Comment 5 Thomas Sailer 2009-04-30 16:32:19 EDT
I've updated the package:
Spec URL: http://sailer.fedorapeople.org/vfrnav.spec
SRPM URL: http://sailer.fedorapeople.org/vfrnav-0.3-4.fc11.src.rpm

(In reply to comment #4)

> You should change that group too to something appropriate.
Ugh. Oversaw that one. Fixed now.

> Like this:
> %dir %{_datadir}/%{name}
> %{_datadir}/%{name}/*

Yes I know I can use wildcards, however I'd like to get notified (by the unpackaged files check) if something changes there, so I would like to keep tne enumeration of all files in that directory.

> - you are supplying desktop file but you are not installing it.
> http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

Fixed (now validating them)

> Desktop file supplied with vfrnav is weird - I never seen that kind of file. I
> would recommend you to make your own desktop file. All of this is explained at
> previous link.

What is actually wierd about them? They conform to the spec as I read it...

> - You are installing icons for your desktop file but you never update icon
> cache. https://fedoraproject.org/wiki/PackagingDrafts/Icon_Cache  

Fixed, thanks!
Comment 6 Igor Jurišković 2009-04-30 16:46:45 EDT
(In reply to comment #5)
> > Desktop file supplied with vfrnav is weird - I never seen that kind of file. I
> > would recommend you to make your own desktop file. All of this is explained at
> > previous link.
> 
> What is actually wierd about them? They conform to the spec as I read it...

Take a look at other desktop files. None of them has Encoding, Version, X-WIndow-Icon, X-Osso-Type. I never seen such desktop file. Best thing would be to wait for advanced packager to tell you is it ok or not.
Comment 7 Christoph Wickert 2009-05-08 15:17:53 EDT
(In reply to comment #4)
> Like this:
> %dir %{_datadir}/%{name}
> %{_datadir}/%{name}/*

And even shorter to 
%{_datadir}/%{name}/


(In reply to comment #5)

> Yes I know I can use wildcards, however I'd like to get notified (by the
> unpackaged files check) if something changes there, so I would like to keep tne
> enumeration of all files in that directory.

Well, IMO you just need to make sure you don't accidentally package unwanted files like leftovers from svn or cvs. I would use

%dir %{_datadir}/%{name}
%{_datadir}/%{name}/*.png
%{_datadir}/%{name}/*.svg
%{_datadir}/%{name}/*.glade

But this is up to you. At least for desktop-file-validate I'd use wildcards because you might forget to validate a file otherwise.

(In reply to comment #6)
> Take a look at other desktop files. None of them has Encoding, Version,
> X-WIndow-Icon, X-Osso-Type.

Encoding should not be in the file, this is correct, but the rest of the file is acceptable. The version key is usually added during install by desktop-file-install and the other keys starting with X- are ok. Whenever you want to extend the freedesktop spec with custom keys you need to prefix them with X-, see http://standards.freedesktop.org/menu-spec/latest/ and especially 
http://standards.freedesktop.org/menu-spec/latest/ar01s03.html

But there are other problems with the desktop files:
- /usr/bin is hardcoded in the Exec tag. Ether remove it or walk over it with sed during %prep to make sure the path always works:
sed -i 's!/usr/bin!%{_bindir}!' data/*.desktop
- What is "X-Osso-Type=application/x-executable" meant for? Are these programs supposed to open a mimetype? I guess not, at least not x-executable, but if so, you need to follow https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database
Comment 8 Thomas Sailer 2009-05-10 13:08:33 EDT
(In reply to comment #7)

> But this is up to you. At least for desktop-file-validate I'd use wildcards
> because you might forget to validate a file otherwise.

desktop-file-validate does not support multiple desktop files on its command line, so it needs to be enclosed in a for loop.

> Encoding should not be in the file, this is correct, but the rest of the file

Why should it not be? We seem to have lots of desktop files having an Encoding entry (lots of gnome-*, eg.).

> But there are other problems with the desktop files:
> - /usr/bin is hardcoded in the Exec tag. Ether remove it or walk over it with
Removed the hardcoded path.

> - What is "X-Osso-Type=application/x-executable" meant for? Are these programs
> supposed to open a mimetype? I guess not, at least not x-executable, but if so,
> you need to follow
This is needed for maemo. On Fedora, it can go, so removed the X- tags.

SRPM: http://sailer.fedorapeople.org/vfrnav-0.3-5.fc11.src.rpm
SPEC: http://sailer.fedorapeople.org/vfrnav.spec
Comment 9 Christoph Wickert 2009-05-10 13:59:20 EDT
(In reply to comment #8)
> Why should it not be? We seem to have lots of desktop files having an Encoding
> entry (lots of gnome-*, eg.).

Because it got removed from freedesktop specs a while ago. Now it is assumed now that files are UTF-8 by default. Yes, there are lots of old files left in Fedora and we should clean them up in the future, but we should not let anything enter Fedora when it's not following the specs.
Comment 10 Thomas Sailer 2009-05-10 14:46:22 EDT
Ok, so another update:
SRPM: http://sailer.fedorapeople.org/vfrnav-0.3-6.fc11.src.rpm
SPEC: http://sailer.fedorapeople.org/vfrnav.spec
Comment 11 Mattias Ellert 2009-05-28 05:58:52 EDT
Package fails to build due to wrong/missing BuildRequires:

1) BuildRequires wrong: libxml++ should be libxml++-devel
2) BuildRequires missing: zfstream-devel
Comment 13 Mattias Ellert 2009-05-29 02:01:05 EDT
Fedora review vfrnav-0.3-7.fc11.src.rpm 2009-05-29

$ rpmlint *.rpm vfrnav.spec 
vfrnav-utils.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 1 warnings.

* OK
? needs attention

* rpmlint is OK

* package is named according to the guidelines

* specfile is named after the package

* package is licensed under a Fedora approved licence (GPLv2+)

* package license matches the license of the sources

* license file (COPYING) is included as %doc

* specfile is written in legible English

* sources matches upstream

$ md5sum vfrnav-0.3.tar.gz SRPM/vfrnav-0.3.tar.gz 
674af747da8ea31642df815e909b2342  vfrnav-0.3.tar.gz
674af747da8ea31642df815e909b2342  SRPM/vfrnav-0.3.tar.gz

* package builds in mock (Fedora 10)

? is the boost-devel BuildRequires needed?
  there are no include statements in the sources including any boost headers
  and there are no boost library dependencies in the built rpms
  configure does check for it though...

? directories not owned by package or its dependencies:

  26x26 and 40x40 are very odd sized icons, neither
  /usr/share/icons/hicolor/26x26 nor /usr/share/icons/hicolor/40x40
  are available in the hicolor-icon-theme package

  the subdirectories /usr/share/icons/hicolor/26x26/hildon and
  /usr/share/icons/hicolor/40x40/hildon are not owned by any package
  either - and a subdirectory called hildon is not standard either
  (not present in any of the directories for normal sized icons), is
  there a reason for not using something standard like apps instead?

* no duplicate files

* permissions are sane and %files has %defattr

* %clean clears buildroot

* spec file consistently uses macros

* package contains code

* %doc is not runtime essential

? is the utils subpackage useful/usable without the main package?
  it currently has no Requires on the main package

* desktop files are processed with desktop-file-validate

* package does not own other's directories

* %install clears buildroot

* installed filenames are UTF-8
Comment 14 Thomas Sailer 2009-05-29 10:29:29 EDT
(In reply to comment #13)

Thanks for starting the review!

> ? is the boost-devel BuildRequires needed?
Indeed. It's not needed.

> ? directories not owned by package or its dependencies:
Convert the icons to more standard sizes (using ImageMagick), and then store them in the correct locations. Now at least plasma finds them.

> ? is the utils subpackage useful/usable without the main package?
>   it currently has no Requires on the main package

You could use the database conversion utilities in vfrnav-utils without vfrnav. But I added the Requires now, it doesn't hurt either.

SRPM: http://sailer.fedorapeople.org/vfrnav-0.3-8.fc11.src.rpm
SPEC: http://sailer.fedorapeople.org/vfrnav.spec

Scratch Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1383258
Comment 15 Mattias Ellert 2009-05-29 14:43:33 EDT
Package approved.
Comment 16 Thomas Sailer 2009-06-16 08:43:11 EDT
Thanks for the review!

New Package CVS Request
=======================
Package Name: vfrnav
Short Description: Flight Planning software for flights under Visual Flight Rules (VFR)
Owners: sailer
Branches: F-11
InitialCC:
Comment 17 Jason Tibbitts 2009-06-17 12:36:54 EDT
CVS done.
Comment 18 Fedora Update System 2009-06-17 14:37:32 EDT
vfrnav-0.3-8.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/vfrnav-0.3-8.fc11
Comment 19 Fedora Update System 2009-06-19 09:34:44 EDT
vfrnav-0.3-8.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

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