Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: kradview - An image viewer oriented to images obtained by X-Ray machines
https://bugzilla.redhat.com/show_bug.cgi?id=710995
Summary: Review Request: kradview - An image viewer oriented to images obtained by X-Ray machines Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: sanjay.ankur@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora Story Points: ---
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
Ankur Sinha sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |673841(fedora-medical)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
Ankur Sinha sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Alias| |kradview
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
Volker Fröhlich volker27@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |volker27@gmx.at
--- Comment #1 from Volker Fröhlich volker27@gmx.at 2011-06-07 16:45:00 EDT --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #2 from Ankur Sinha sanjay.ankur@gmail.com 2011-06-30 14:47:39 EDT --- (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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #3 from Volker Fröhlich volker27@gmx.at 2011-07-10 17:07:23 EDT --- 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-lates...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #4 from Ankur Sinha sanjay.ankur@gmail.com 2011-07-11 11:57:42 EDT --- Hello,
Updated:
http://ankursinha.fedorapeople.org/kradview/kradview-1.1.0-3.fc15.src.rpm
http://ankursinha.fedorapeople.org/kradview/kradview.spec
Thanks, Ankur
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #5 from Volker Fröhlich volker27@gmx.at 2011-07-11 16:55:15 EDT --- 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.)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #6 from Ankur Sinha sanjay.ankur@gmail.com 2011-07-12 12:03:35 EDT --- (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!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #7 from Volker Fröhlich volker27@gmx.at 2011-07-16 16:22:28 EDT --- 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."
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #8 from Volker Fröhlich volker27@gmx.at 2011-07-16 17:35:37 EDT --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #9 from Ankur Sinha sanjay.ankur@gmail.com 2011-07-17 02:04:59 EDT --- * 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
Veeti Paananen veeti.paananen@rojekti.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |veeti.paananen@rojekti.fi
--- Comment #10 from Veeti Paananen veeti.paananen@rojekti.fi 2011-07-26 12:22:50 EDT --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
Neal Becker ndbecker2@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ndbecker2@gmail.com
--- Comment #11 from Neal Becker ndbecker2@gmail.com 2011-08-13 14:46:35 EDT --- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #12 from Ankur Sinha sanjay.ankur@gmail.com 2011-08-14 00:13:11 EDT --- Hi Neal,
I just built it on koji successfully:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3271510
Thanks, Ankur
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #13 from Ankur Sinha sanjay.ankur@gmail.com 2011-08-14 00:13:41 EDT --- Rawhide:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3271507
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
Ankur Sinha sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution| |WONTFIX Last Closed| |2011-10-15 00:59:45
--- Comment #14 from Ankur Sinha sanjay.ankur@gmail.com 2011-10-15 00:59:45 EDT --- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
Christoph Wickert cwickert@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |cwickert@fedoraproject.org
--- Comment #15 from Christoph Wickert cwickert@fedoraproject.org 2011-10-15 03:48:42 EDT --- What about the medical packages you already own? I tried to contact you in bug 666726 but got no response.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=710995
--- Comment #16 from Ankur Sinha sanjay.ankur@gmail.com 2011-10-15 04:23:13 EDT --- 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
package-review@lists.fedoraproject.org