Bug 710995 (kradview) - Review Request: kradview - An image viewer oriented to images obtained by X-Ray machines
Summary: Review Request: kradview - An image viewer oriented to images obtained by X-R...
Keywords:
Status: CLOSED WONTFIX
Alias: kradview
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: fedora-medical
TreeView+ depends on / blocked
 
Reported: 2011-06-06 06:49 UTC by Ankur Sinha (FranciscoD)
Modified: 2011-10-15 08:23 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-10-15 04:59:45 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Ankur Sinha (FranciscoD) 2011-06-06 06:49:47 UTC
Spec URL: http://ankursinha.fedorapeople.org/kradview/kradview.spec
SRPM URL: http://ankursinha.fedorapeople.org/kradview/kradview-1.1.0-1.fc15.src.rpm

Description: 
An image viewer oriented to images obtained by X-Ray machines.

It was developed by David Santo Orcero during his work on his Ph.D.

============================================================================

[ankur@ankur SRPMS]$ rpmlint ../SPECS/kradview.spec kradview-1.1.0-1.fc15.src.rpm /var/lib/mock/fedora-rawhide-i386/result/*.rpm
kradview.src: W: invalid-url URL www.orcero.org/irbis/kradview/
kradview.i686: W: invalid-url URL www.orcero.org/irbis/kradview/
kradview.i686: W: no-manual-page-for-binary kradview_client
kradview.i686: W: no-manual-page-for-binary kradview
kradview.src: W: invalid-url URL www.orcero.org/irbis/kradview/
kradview-debuginfo.i686: W: invalid-url URL www.orcero.org/irbis/kradview/
4 packages and 1 specfiles checked; 0 errors, 6 warnings.

Comment 1 Volker Fröhlich 2011-06-07 20:45:00 UTC
The invalid URL is easy to sort out: http://www.orcero.org/irbis/kradview

I suggest to run rpmlint on ALL your RPM files or run it after having installed the packages: rpmlint my_package

No buildroot definition, no rm -rf $RPM_BUILD_ROOT necessary.

$ desktop-file-validate ./src/kradview.desktop
./src/kradview.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated
./src/kradview.desktop: warning: value "kradview %i %m -caption "%c"" for key "Exec" in group "Desktop Entry" contains a deprecated field code "%m"
./src/kradview.desktop: warning: boolean key "Terminal" in group "Desktop Entry" has value "0", which is deprecated: boolean values should be "false" or "true"

I'm not an expert in Docbook, but it seems to me, the documentation is still in Docbook format -- not HTML.

It is common to put %doc as the first element of the files list, but don't know whether it's a rule.

Comment 2 Ankur Sinha (FranciscoD) 2011-06-30 18:47:39 UTC
(In reply to comment #1)
> The invalid URL is easy to sort out: http://www.orcero.org/irbis/kradview

Corrected.

> 
> I suggest to run rpmlint on ALL your RPM files or run it after having installed
> the packages: rpmlint my_package
> 
> No buildroot definition, no rm -rf $RPM_BUILD_ROOT necessary.
> 
> $ desktop-file-validate ./src/kradview.desktop
> ./src/kradview.desktop: warning: key "Encoding" in group "Desktop Entry" is
> deprecated

Corrected. 

> ./src/kradview.desktop: warning: value "kradview %i %m -caption "%c"" for key
> "Exec" in group "Desktop Entry" contains a deprecated field code "%m"

Don't know how to correct this. Letting this warning be.

> ./src/kradview.desktop: warning: boolean key "Terminal" in group "Desktop
> Entry" has value "0", which is deprecated: boolean values should be "false" or
> "true"
> 

Corrected. 

> I'm not an expert in Docbook, but it seems to me, the documentation is still in
> Docbook format -- not HTML.

The build is supposed to generate the documentation. It requires doxygen. I'll look into it. 

> 
> It is common to put %doc as the first element of the files list, but don't know
> whether it's a rule.


[ankur@ankur SRPMS]$ rpmlint ../SPECS/kradview.spec kradview-1.1.0-2.fc15.src.rpm /var/lib/mock/fedora-rawhide-i386/result/*.rpm
kradview.i686: W: no-manual-page-for-binary kradview_client
kradview.i686: W: no-manual-page-for-binary kradview
4 packages and 1 specfiles checked; 0 errors, 2 warnings.


Updated spec/srpm:

http://ankursinha.fedorapeople.org/kradview/kradview.spec

http://ankursinha.fedorapeople.org/kradview/kradview-1.1.0-2.fc15.src.rpm

Thanks!
Ankur

Comment 3 Volker Fröhlich 2011-07-10 21:07:23 UTC
You are right in your comment: Don't install a desktop file to the applnk directory. That is outdated.

You can drop the defattr line.

"An image viewer oriented to images obtained by X-Ray machines." is not a sentence, thus has no full-stop.

Ad desktop file: The %m is the culprit. Please see http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html

Comment 5 Volker Fröhlich 2011-07-11 20:55:15 UTC
Don't parse "ls". Using the find command is a lot cleaner. Something like:

find . -type f  \( -name "*.cpp" -o -name "*.h" -o -name "*.c" -o -name "*.xpm" \) -exec chmod a-x {} \;

You're missing the .desktop file and icons though. I'd just chmod the whole src directory.

Don't install the documentation manually. Use the %doc macro instead (and put it first under %files -- that is common). Please visit the RPM documentation on how to use it.

Ad iconv: Try iconv -f iso-8859-1 -t utf-8 dicomfformat_8c.xml

(Probably put that desktop file comment above the three seds. You might want to start your comments with either a capital letter or not, but not mix it, but that is just a question of style and not necessary for the review.)

Comment 6 Ankur Sinha (FranciscoD) 2011-07-12 16:03:35 UTC
(In reply to comment #5)
> Don't parse "ls". Using the find command is a lot cleaner. Something like:
> 
> find . -type f  \( -name "*.cpp" -o -name "*.h" -o -name "*.c" -o -name "*.xpm"
> \) -exec chmod a-x {} \;
> 

find gives me a "too many open files error" at times. 

> You're missing the .desktop file and icons though. I'd just chmod the whole src
> directory.

Done

> 
> Don't install the documentation manually. Use the %doc macro instead (and put
> it first under %files -- that is common). Please visit the RPM documentation on
> how to use it.

Done

> 
> Ad iconv: Try iconv -f iso-8859-1 -t utf-8 dicomfformat_8c.xml

Done 

> 
> (Probably put that desktop file comment above the three seds. You might want to
> start your comments with either a capital letter or not, but not mix it, but
> that is just a question of style and not necessary for the review.)

Corrected.

http://ankursinha.fedorapeople.org/kradview/kradview-1.1.0-4.fc15.src.rpm

http://ankursinha.fedorapeople.org/kradview/kradview.spec

Thanks!

Comment 7 Volker Fröhlich 2011-07-16 20:22:28 UTC
I took a closer look and it is basically only class documentation. I think you should drop the HTML, xml, html and latex directories (or not even produce them).

Please delete the lone "%doc" at the bottom.

I'd suggest starting the description like "Kradview is an image viewer, oriented to images obtained by X-Ray machines."

Comment 8 Volker Fröhlich 2011-07-16 21:35:37 UTC
By the way, the desktop file could use a "Categories" entry. Please notice, that categories always terminate with a semi-colon. The "Comment"s are also pretty useless. Maybe you can come up with a good comment.

See http://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files for further information.

Comment 9 Ankur Sinha (FranciscoD) 2011-07-17 06:04:59 UTC
* Sun Jul 17 2011 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 1.1.0-5
- Improve desktop file
- Removed extra documentation


http://ankursinha.fedorapeople.org/kradview/kradview.spec

http://ankursinha.fedorapeople.org/kradview/kradview-1.1.0-5.fc15.src.rpm

Comment 10 Veeti Paananen 2011-07-26 16:22:50 UTC
Some comments:

- I think that the "get rid of build error" sed command should go to a patch file. The patch should be sent to upstream.

- At least some of the desktop file modifications (if not all) can be done with desktop-file-install. A patch would probably work too.

- Join the two "correct permissions" sections for clarity, instead of having them split by the UTF-8 conversion.

- You could do something like "%{_datadir}/icons/hicolor/*x*/apps/%{name}.png" in the files section to avoid the two lines.

Comment 11 Neal Becker 2011-08-13 18:46:35 UTC
Tried to build on f15 x86_64.  Got:

checking for Qt... configure: error: Qt (>= Qt 3.0) (headers and libraries) not found. Please check your installation!

Don't know what it could be missing:
Package qt3-devel-3.3.8b-35.fc15.x86_64 already installed and latest version

Comment 12 Ankur Sinha (FranciscoD) 2011-08-14 04:13:11 UTC
Hi Neal,

I just built it on koji successfully:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3271510

Thanks,
Ankur

Comment 13 Ankur Sinha (FranciscoD) 2011-08-14 04:13:41 UTC
Rawhide:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3271507

Comment 14 Ankur Sinha (FranciscoD) 2011-10-15 04:59:45 UTC
Hi,

I'm not working on -medical related packages anymore and will not be pursuing this review ticket. If someone else wishes to take up the review, please open a new ticket. 

Thanks,
Ankur

Comment 15 Christoph Wickert 2011-10-15 07:48:42 UTC
What about the medical packages you already own? I tried to contact you in bug 666726 but got no response.

Comment 16 Ankur Sinha (FranciscoD) 2011-10-15 08:23:13 UTC
Hi Christoph,

I'm sorry, I must've missed out on the email. xmedcon has been pushed to updates a while back. I'm holding on to the other -medical packages I own for the time being. I had sent out emails to the devel and medical lists requesting people who actually use these packages to take them over but I haven't received any replies there. I haven't heard from Susmit in a while too, either on the reviews or the mailing lists. 

Thanks,
Ankur


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