Spec Url: http://dmitry.butskoy.name/dvdisaster/dvdisaster.spec SRPM Url: http://dmitry.butskoy.name/dvdisaster/dvdisaster-0.64-1.src.rpm Description: Dvdisaster provides a margin of safety against data loss on CD and DVD media caused by scratches or aging. It creates error correction data, which is used to recover unreadable sectors if the disc becomes damaged at a later time.
Initial comments, not a full review: - Are you sure about the %description i18n syntax? I thought it'd be %description -l $lang, not %description($lang). The Italian one has a typo (two r's in descrription). - Why StartupNotify=false in desktop entry? - "s/Comment= /Comment=/" dvdisaster.desktop - The desktop entry could use an icon, eg. icons/create.png -> /usr/share/icons/hicolor/24x24/apps/dvdisaster.png - "GTK" would sound more appropriate than "GNOME" in desktop entry's categories - "Application" is not a registered desktop entry category - gtk2-devel already pulls in atk-devel and pango-devel (at least in FC4) - Maybe better to specify localedir as %{_datadir}/locale than %{_prefix}/share/locale - LOCALEDIR should be defined in "make" at %build time too as it is compiled in (see dvdisaster.c). Maybe also other dirs, haven't checked. Maybe ./configure is not doing its job properly? From "make show": LOCALEDIR = /usr/locale DOCDIR = /usr/doc DOCSUBDIR = /usr/doc/dvdisaster-0.64 - Missing BuildRequires: gettext - The German and Italian man pages should be marked as %lang(de) and %lang(it). Ditto the other German and Italian documentation. - Buildroot remains in installed files, only man pages though (install fedora-rpmdevtools and add this to your ~/.rpmmacros to reproduce): %__arch_install_post /usr/lib/rpm/check-rpaths /usr/lib/rpm/check-buildroot + /usr/lib/rpm/check-buildroot /var/tmp/dvdisaster-0.64-1-buildroot-scop/usr/share/man/it/man1/dvdisaster.1:.IR /var/tmp/dvdisaster-0.64-1-buildroot-scop/usr/share/doc/dvdisaster-0.64/en /var/tmp/dvdisaster-0.64-1-buildroot-scop/usr/share/man/man1/dvdisaster.1:.IR /var/tmp/dvdisaster-0.64-1-buildroot-scop/usr/share/doc/dvdisaster-0.64/en /var/tmp/dvdisaster-0.64-1-buildroot-scop/usr/share/man/de/man1/dvdisaster.1:.IR /var/tmp/dvdisaster-0.64-1-buildroot-scop/usr/share/doc/dvdisaster-0.64/de Found '/var/tmp/dvdisaster-0.64-1-buildroot-scop' in installed files; aborting error: Bad exit status from /var/tmp/rpm-tmp.95477 (%install) - I wonder why the icons are installed as %doc in /usr/share/doc/dvdisaster-0.64 ?
(In reply to comment #1) > - I wonder why the icons are installed as %doc > in /usr/share/doc/dvdisaster-0.64 ? Oh, never mind this.
> Are you sure about the %description i18n syntax? Not sure. :) I've tried to guess correct syntax, believing that either rpmbuild or rpmlint will tell me if i've not guessed... Both say nothing about it. :( > Why StartupNotify=false in desktop entry? Just take it from the wiki/PackagingGuidelines example. AFAIK dvdisaster is not so much closely integrated with Gnome or KDE to support startup-notification features. > The desktop entry could use an icon, eg. icons/create.png Add this one, actually placed as /usr/share/pixmaps/dvdisaster.png > "Application" is not a registered desktop entry category cd /usr/share/applications grep 'Categories.*Application' * it gives 93 matches on my FC3 box... As I make this package for Fedora (not for general use), if Fedora for some reason does not follow standards, then I should believe Fedora. > Maybe better to specify localedir as %{_datadir}/locale than > %{_prefix}/share/locale "share/locale" is hardcoded in /usr/lib/rpm/find-debuginfo.sh, therefore I've chosen such variant. Other things are done. New SPEC: http://dmitry.butskoy.name/dvdisaster/dvdisaster.spec New SRPM: http://dmitry.butskoy.name/dvdisaster/dvdisaster-0.64-2.src.rpm
Created attachment 120282 [details] gdb backtrace Well, StartupNotify=false disables at least KDE's generic startup notification, which is inconsistent with practically all other apps. I don't see a reason why it should be so. The app segfaults at startup here on FC4, on a laptop that doesn't have a CD or DVD writer but just a DVD/CD-ROM drive. gdb backtrace attached.
OK, change to "StartupNotify=true" > The app segfaults at startup here on FC4, on a laptop that doesn't have a CD or > DVD writer but just a DVD/CD-ROM drive. gdb backtrace attached. Hmmm... Cannot reproduce. FC3 + CD-ROM-only works fine. Current version is not "0.1"-like (0.64), therefore I believe that the upstream tested CD/DVD read-only drive case. Can you check this segfault at least on one other FC4 box?
In the future, please bump the release and add a changelog entry when you make changes to the package, also during review. Before setting StartupNotify=true, be sure that it's indeed what you want: http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#recognized-keys I guess it would be correct to omit StartupNotify altogether here (unchecked). Tested the app on two additional FC4 boxes (one i386 with a DVD/CD writer, and one ppc with just a regular DVD/CD reader), both crash at startup the same way as the laptop mentioned earlier. However, I noticed this: $ ls -la /dev/dvd* /dev/cd* /dev/hda lrwxrwxrwx 1 root root 3 Oct 21 22:25 /dev/cdrom -> hda lrwxrwxrwx 1 root root 3 Oct 21 22:25 /dev/cdwriter -> hda lrwxrwxrwx 1 root root 3 Oct 21 22:25 /dev/dvd -> hda lrwxrwxrwx 1 root root 3 Oct 21 22:25 /dev/dvdwriter -> hda brw-rw---- 1 root disk 3, 0 Oct 21 22:25 /dev/hda $ dvdisaster dvdisaster-0.64 Copyright 2004,2005 Carsten Gnoerlich. This software comes with ABSOLUTELY NO WARRANTY. This is free software and you are welcome to redistribute it under the conditions of the GNU GENERAL PUBLIC LICENSE. See the file "COPYING" for further information. No CD/DVD drives found in /dev. No drives will be pre-selected. ...so, no crash when the DVD/CD writer device is owned by root and running as a regular user. dvdisaster says "no drives found" though. But after changing the device ownership to the normal user and trying again, no other changes: $ ls -la /dev/hda brw-rw---- 1 scop disk 3, 0 Oct 21 22:25 /dev/hda $ dvdisaster Segmentation fault (core dumped)
> Before setting StartupNotify=true Reverted back. Thanks for the doc link... Coredumps: When device is owned by root -- the same report as you wrote above, when owned by the user -- all works fine on any CD/DVD type (no coredumps). ...and no any gcc warnings while compile. FC3 is OK, FC4 is not... My desktop is GNOME, what is your one?
Also one question: On the system where dvdisaster crashes, how it works in command line mode?
Created attachment 120312 [details] gcc4 compilation warnings On the laptop, desktop is KDE. On the other i386 box with the DVD/CD writer, no desktop, but a very minimal installation, basically just kdelibs and gtk2 (no full desktop of either). On the ppc, no idea. The latter two are accessed remotely from the KDE-running laptop. gcc4 compilation warnings attached. Command line mode appears to work. FYI, I noticed that the written medium.img images have 0700 permissions (unnecessarily executable).
Argh, please read comment 6 again. Your SRPM has changed yet again without not bumping the release tag and without any change log entry. This makes reviewing a PITA. StartupNotify=false will still disable KDE's built-in generic startup notification. Removing StartupNotify altogether from the .desktop file would look like the correct solution.
s/without not/without/
> Your SRPM has changed yet again Sorry, I just hoped that you will work with already downloaded srpm, as I did not point a new srpm here. > StartupNotify=false OK, will be deleted. > medium.img images have 0700 permissions (unnecessarily executable). IMHO this is a heritage of the fact that this application is multiplatform (win32/UNIX), probably developers still incorrectly understand UNIX permissions. I'll think about a patch. > gcc4 compilation warnings attached. Thanks, but nothing interesting here yet. I also have checked under clean KDE and when compiled by gcc4 -- all works fine on FC3. It seems to be some specific FC4-related issue. Ville, Can you try my FC3 working binary on your host? http://dmitry.butskoy.name/dvdisaster/dvdisaster-fc3.bin (I hope it will be compatible with your shared libraries etc...)
(In reply to comment #12) > http://dmitry.butskoy.name/dvdisaster/dvdisaster-fc3.bin > (I hope it will be compatible with your shared libraries etc...) Seems to be, but crashes the same way as my local builds.
Sadly. It means that something has changed in FC4 environment/libraries, that forces the program to crash in GUI mode. May be it is better to delay this review for a while, and send all the issues upstream?
Sure, no problem, just post updates here when available.
And yep, the crash is in the GUI code, in a callback function that receives the drive selected in a combobox, see comment 4, attachment 120282 [details].
(In reply to comment #16) > And yep, the crash is in the GUI code, in a callback function that receives > the drive selected in a combobox, see comment 4, attachment 120282 [details] [edit]. This was a bug in dvdisaster-0.64 in conjunction with gtk+-2.6. Please use the corrected sources for version 0.64-1 from http://www.dvdisaster.com/download.html And of course feel free to bug me with further questions if something is still wrong ;-)
The new upstream version is eaten. New SPEC: http://dmitry.butskoy.name/dvdisaster/dvdisaster.spec New SRPM: http://dmitry.butskoy.name/dvdisaster/dvdisaster-0.64-3.src.rpm ping Ville :) Carsten, repeat here too: - don't use versioning like "0.64-1", use "0.64.1" or "0.64pl1" instead. - rename downloadable tarball to be suffixed as ".tar.bz2" (not ".tar.bz") - see my Patch1 in the rpm package -- IMHO you should not create files with executable bit set...
Dmitry, > - don't use versioning like "0.64-1", use "0.64.1" or "0.64pl1" instead. > - rename downloadable tarball to be suffixed as ".tar.bz2" (not ".tar.bz") > - see my Patch1 in the rpm package -- IMHO you should not create files with > executable bit set... yup, all three points make sense :-) I'll wrap them up with some other minor fixes in 0.64pl2 over the weekend.
(In reply to comment #3) just some other info... > [..] AFAIK dvdisaster is not > so much closely integrated with Gnome or KDE to support startup-notification > features. Yes, dvdisaster is rather desktop agnostic. The general policy is trying to play well on any sane desktop environment, but not to depend on any specific desktop features and/or libraries (sometimes quite contradictory, I know ;-) > > The desktop entry could use an icon, eg. icons/create.png Let me know if you need this rendered in different resolutions. Carsten
I suggest we wait for that "pl2" release before proceeding with this, then. Carsten, may I suggest sticking with regular numbers and dots in the release number? Non-numeric versions tend to often interact badly with version ordering algorithms, such as rpm's or deb's, forcing packagers to apply weird versioning tricks so that upgrades work. I'd suggest 0.64.2 instead of 0.64pl2.
(In reply to comment #21) > Non-numeric versions tend to often interact badly with version > ordering algorithms, such as rpm's or deb's, forcing packagers to apply weird > versioning tricks so that upgrades work. I'd suggest 0.64.2 instead of > 0.64pl2. I changed the package handling to allow separate package and "GUI" version names in the just released update. Therefore we can now use standard package names like dvdisaster-0.64.2.tar.bz2, but the program can advertise itself in the GUI in a more convenient manner [I prefer 0.64 (pl2) for some reason ;-)] Carsten
Update to 0.64.2 New SPEC: http://dmitry.butskoy.name/dvdisaster/dvdisaster.spec New SRPM: http://dmitry.butskoy.name/dvdisaster/dvdisaster-0.64.2-1.src.rpm
Created attachment 120660 [details] Use user's preferred browser The attached patch would make it more likely that the manual is opened with the user's preferred browser. With it or another sane derivative applied, approved.
>Use user's preferred browser Add a patch. Just use: Closure->browser = g_strdup("htmlview"); it is enough to do changes here only. Notwithstanding htmlview is considered itself as deprecated, it is still used in FC3/FC4/devel. htmlview is a desktop-independed way, therefore I'll use it for this. Carsten, IMHO this patch is Fedora-specific (other distros may no have htmlview at all), therefore don't apply it upstream... :) Also I've added you to CC list of this package (you will receive all bugzilla reports too). If you don't want it, tell me :)
As long as htmlview is installed by default and it's hard enough to get rid of it due to dependencies, your patch should be ok'ish. But note that htmlview is not really that desktop independent. On FC4 it uses the browser configured in GNOME if gconftool-2 and gnome-open are installed even if running under KDE. Also, in that case, /etc/htmlview.conf or ~/.htmlviewrc are not even read, so there doesn't seem to be a way around it.
(In reply to comment #24) > [..] > The attached patch would make it more likely that the manual is opened with the > user's preferred browser. Using gnome-open et al looks like a good idea. However the patch is not yet water-proof for the case that gnome-open can be executed, but eventually fails to open the document (may not happen on Fedora, but I could name at least one distro where this happens ;-) This needs some rework of the code which wait()s for the forked browser process; I'll look into this tomorrow. (Switching to Dmitry's reply) > IMHO this patch is Fedora-specific (other distros may no have htmlview at all), > therefore don't apply it upstream... :) That won't hurt. If calling a specific "browser" fails, dvdisaster will try the next alternative from the list in show-manual.c (if the above problem is solved, that is). > Also I've added you to CC list of this package (you will receive all bugzilla > reports too). Fine :-) Carsten
Created attachment 120765 [details] More robust browser selection
Oops, sorry. Forgot to paste in the comments. I have merged Ville's patch and some additional logic against misconfigured gnome-open/html-views into dvdisaster-browsers2.patch. Please note that the order of the browser list is not that important since the last working choice is remembered in the .dvdisaster file and always tried first. Carsten
(for comment #29) I've already released dvdisaster before this comment, you can find it at http://download.fedora.redhat.com/pub/fedora/linux/extras/ Is there a chance this change will be appeared soon in the next upstream version? If so, I'll just wait for it, else I'll apply this patch immediately.
FWIW, to me it looks like the version in FE is fine for now, no need to roll a new one just because of this patch.
Hi Dmitry and Ville, I agree that the 0.64.2* versions look stable now, so I'll save the second browser patch for 0.65 (which does not yet have a release date). Carsten
Package Change Request ====================== Package Name: dvdisaster New Branches: EL-4 EL-5
cvs done.