Bug 171418 - Review Request: dvdisaster - CD/DVD media data loss/scratch/aging protection
Review Request: dvdisaster - CD/DVD media data loss/scratch/aging protection
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
David Lawrence
http://www.dvdisaster.com
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-10-21 12:18 EDT by Dmitry Butskoy
Modified: 2007-12-10 11:40 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-11-03 07:46:16 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)
gdb backtrace (5.14 KB, text/plain)
2005-10-22 15:55 EDT, Ville Skyttä
no flags Details
gcc4 compilation warnings (2.65 KB, text/plain)
2005-10-24 13:31 EDT, Ville Skyttä
no flags Details
Use user's preferred browser (974 bytes, patch)
2005-11-02 15:58 EST, Ville Skyttä
no flags Details | Diff
More robust browser selection (5.97 KB, patch)
2005-11-05 15:07 EST, Carsten Gnoerlich
no flags Details | Diff

  None (edit)
Description Dmitry Butskoy 2005-10-21 12:18:59 EDT
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.
Comment 1 Ville Skyttä 2005-10-22 11:47:01 EDT
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 ? 
Comment 2 Ville Skyttä 2005-10-22 11:50:41 EDT
(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. 
Comment 3 Dmitry Butskoy 2005-10-22 15:20:05 EDT
> 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
Comment 4 Ville Skyttä 2005-10-22 15:55:48 EDT
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.
Comment 5 Dmitry Butskoy 2005-10-24 09:15:06 EDT
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?
Comment 6 Ville Skyttä 2005-10-24 11:13:05 EDT
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) 
Comment 7 Dmitry Butskoy 2005-10-24 11:59:09 EDT
> 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?


Comment 8 Dmitry Butskoy 2005-10-24 12:04:03 EDT
Also one question:

On the system where dvdisaster crashes, how it works in command line mode?
Comment 9 Ville Skyttä 2005-10-24 13:31:46 EDT
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).
Comment 10 Ville Skyttä 2005-10-24 13:43:15 EDT
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. 
Comment 11 Ville Skyttä 2005-10-24 13:44:06 EDT
s/without not/without/ 
Comment 12 Dmitry Butskoy 2005-10-25 07:25:50 EDT
> 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...)
Comment 13 Ville Skyttä 2005-10-26 11:16:07 EDT
(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. 
Comment 14 Dmitry Butskoy 2005-10-26 11:34:58 EDT
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?
Comment 15 Ville Skyttä 2005-10-26 13:20:03 EDT
Sure, no problem, just post updates here when available. 
Comment 16 Ville Skyttä 2005-10-26 13:22:51 EDT
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]. 
Comment 17 Carsten Gnoerlich 2005-10-27 10:15:14 EDT
(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 ;-)
Comment 18 Dmitry Butskoy 2005-10-27 11:22:46 EDT
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...


Comment 19 Carsten Gnoerlich 2005-10-27 14:23:40 EDT
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.
Comment 20 Carsten Gnoerlich 2005-10-27 14:34:58 EDT
(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
Comment 21 Ville Skyttä 2005-10-27 14:41:21 EDT
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. 
Comment 22 Carsten Gnoerlich 2005-11-01 16:05:35 EST
(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
Comment 24 Ville Skyttä 2005-11-02 15:59:00 EST
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.
Comment 25 Dmitry Butskoy 2005-11-03 07:46:16 EST
>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 :)
Comment 26 Ville Skyttä 2005-11-03 09:41:27 EST
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. 
Comment 27 Carsten Gnoerlich 2005-11-03 15:09:11 EST
(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
Comment 28 Carsten Gnoerlich 2005-11-05 15:07:50 EST
Created attachment 120765 [details]
More robust browser selection
Comment 29 Carsten Gnoerlich 2005-11-05 15:10:18 EST
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
Comment 30 Dmitry Butskoy 2005-11-07 06:52:29 EST
(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.

Comment 31 Ville Skyttä 2005-11-07 11:41:09 EST
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. 
Comment 32 Carsten Gnoerlich 2005-11-09 14:16:32 EST
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
Comment 33 Dmitry Butskoy 2007-12-10 07:43:59 EST
Package Change Request
======================
Package Name: dvdisaster
New Branches: EL-4 EL-5
Comment 34 Kevin Fenzi 2007-12-10 11:40:15 EST
cvs done.

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