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.
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.
(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
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
Hello, Updated: http://ankursinha.fedorapeople.org/kradview/kradview-1.1.0-3.fc15.src.rpm http://ankursinha.fedorapeople.org/kradview/kradview.spec Thanks, Ankur
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.)
(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!
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."
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.
* 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
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.
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
Hi Neal, I just built it on koji successfully: http://koji.fedoraproject.org/koji/taskinfo?taskID=3271510 Thanks, Ankur
Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=3271507
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
What about the medical packages you already own? I tried to contact you in bug 666726 but got no response.
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