Bug 372094 - Please restore wodim support
Summary: Please restore wodim support
Status: RESOLVED FIXED
Alias: None
Product: k3b
Classification: Applications
Component: Burning/Hardware (show other bugs)
Version: unspecified
Platform: Fedora RPMs Linux
: NOR major
Target Milestone: ---
Assignee: k3b developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-05 01:40 UTC by Kevin Kofler
Modified: 2016-11-09 07:58 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Kofler 2016-11-05 01:40:55 UTC
In: https://quickgit.kde.org/?p=k3b.git&a=commitdiff&h=fcb0ff1f36c319fd1e2b4bd92c2c08fdc690212c you removed support for wodim. Please revert this commit. Fedora ships only cdrkit, not cdrtools, due to licensing issues: https://fedoraproject.org/wiki/Forbidden_items#cdrtools . If you need a distribution shipping wodim for testing, try Fedora.

DVD support in wodim is crap and BluRay support is nonexistent, but that's why the code you removed, which prefers growisofs, is there!

What will happen now that you removed the detection for wodim is that K3b will think that wodim is cdrecord (because it symlinks itself to cdrecord) and no longer disable use of wodim for things it does not support. Please do not remove code you don't understand.
Comment 1 Kevin Kofler 2016-11-05 01:44:35 UTC
I will also note that your changes have several logic errors:

-    : K3b::AbstractCdrtoolsProgram( QLatin1String( "cdrecord" ), QLatin1String( "wodim" ) )
+    : K3b::AbstractCdrtoolsProgram( QLatin1String( "cdrecord" ), QLatin1String("cdrecord") )

(2 times the same command, probably not what you want)

-        // but prefer growisofs to wodim for DVDs
+        // but prefer growisofs for DVDs
         if ( d->usedWritingApp == K3b::WritingAppAuto ) {
-            if (k3bcore->externalBinManager()->binObject("cdrecord")->hasFeature( "wodim" ))
-                d->usedWritingApp = K3b::WritingAppGrowisofs;
-            else
-                d->usedWritingApp = K3b::WritingAppCdrecord;
+            d->usedWritingApp = K3b::WritingAppCdrecord;

(The comment says "prefer growisofs", the code says "prefer cdrecord".)

but instead of fixing those errors, please revert this commit wholesale, or K3b will have to be removed from Fedora.
Comment 2 Kevin Kofler 2016-11-05 01:55:08 UTC
And in the long run, to work with something maintained, see wishlist bug #137436.
Comment 3 Kevin Kofler 2016-11-05 02:09:35 UTC
Also, your change is labeled "Use cdrecord for burning blueray instead of wodim.", but K3b did not attempt to use wodim to burn BluRay even before your change. If you look more closely at the code you removed, you will notice that it took care to prefer growisofs over wodim for all DVD and BluRay burning tasks. (The only reason we need wodim at all is that growisofs does not support CDs.) In fact, ironically, your change actually made K3b attempt to use wodim for BluRay! (Then you added a followup to just unconditionally prefer growisofs: http://quickgit.kde.org/?p=k3b.git&a=commitdiff&h=f3f4602e30c67bf622b5a52ea39c78817b1dd020 , which I'm not sure is a good idea either. It is much better than wodim, but is it better than current versions of cdrecord? Probably not. You will need to revert that first so that you can revert fcb0ff1f.)
Comment 4 Rex Dieter 2016-11-05 12:31:26 UTC
marking confirmed, fedora will likely need this fixed before we can consider shipping this new version
Comment 5 Leslie Zhai 2016-11-09 03:05:07 UTC
Hi Kevin,

I am a newbie of K3b maintainer http://www.leetcode.cn/2016/08/k3b.html so welcome to report bug to me ;-)

But Thomas Schmitt , the author of libburnia, have already reviewed my patch https://bugs.kde.org/show_bug.cgi?id=367639#c14 *including*:

> Isn't your patch
>  "Use cdrecord for burning blueray instead of wodim."
>  http://commits.kde.org/k3b/fcb0ff1f36c319fd1e2b4bd92c2c08fdc690212c
> mislabelled ?
> Shouldn't it rather be
> "Use cdrecord for burning blueray and DVD instead of growisofs"
>?

And I am double sure I fixed it!

> DVD support in wodim is crap and BluRay support is nonexistent, but that's why the code you removed, which prefers growisofs, is there!
> The only reason we need wodim at all is that growisofs does not support CDs

NOPE!
[R.E.Q]I will add libburnia backup backend W.I.P. *BUT* I will never ever use wodim! I *hate* meaningless forked then unmaintained!

Regards,
Leslie Zhai - a KDE developer https://git.reviewboard.kde.org/users/lesliezhai/
Comment 6 Kevin Kofler 2016-11-09 03:43:53 UTC
Removing working detection code just because you did not understand what it really does is not helpful. Especially when your "fixes" actually make the code misbehave on our distribution. And answering a request to restore the code with a "NOPE!" in ALL CAPS is also not helpful.
Comment 7 Kevin Kofler 2016-11-09 03:44:51 UTC
And we will default to libburnia if you add tested support for it. But I object to removing wodim support before a replacement is ready. We cannot use cdrecord in Fedora.
Comment 8 Leslie Zhai 2016-11-09 03:52:31 UTC
(In reply to Kevin Kofler from comment #7)
> And we will default to libburnia if you add tested support for it. But I
> object to removing wodim support before a replacement is ready. We cannot
> use cdrecord in Fedora.

Yes, I am editing libk3b/core/k3bdefaultexternalprograms.cpp for adding cdrskin to K3b::AbstractCdrtoolsProgram( QLatin1String( "cdrskin" ), QLatin1String("cdrecord") )

I might be too anxious ;-) sorry for my expression! but I want to ship still-maintaining && BUGFREE product to end-users.
for example, transcode was unmaintained too, please see KDEBUG-360170 http://www.leetcode.cn/2016/08/k3b.html#bug I migrated it, *NOT* be merged again, so deprecated transcode resolutely, end-users do *NOT* need unuseful software even it still be packaged again and again...
Comment 9 Kevin Kofler 2016-11-09 04:35:51 UTC
> but I want to ship still-maintaining && BUGFREE product to end-users.

I actually agree with you there. I really think we need to switch to libburnia.

You should just clearly communicate this to packagers when announcing the new release, so we do not end up with things working poorly because wodim is getting silently picked up due to outdated dependencies in the package.

You should also make sure that cdrskin is preferred over wodim even if wodim is installed/symlinked as "cdrecord" (because updating K3b, even if we add a Requires: cdrskin, is not magically going to make installed wodim packages go away, we cannot really use Obsoletes or Conflicts to force it removed).

> Yes, I am editing libk3b/core/k3bdefaultexternalprograms.cpp for adding
> cdrskin to K3b::AbstractCdrtoolsProgram( QLatin1String( "cdrskin" ),
> QLatin1String("cdrecord") )

But that is only the easy part. You need to look at all the places where a decision what burning program to prefer is made (or checks for "if burning program == cdrecord, apply workaround foo"), make it prefer cdrskin for things it is actually better at (remember that growisofs is unmaintained too, though not as long as wodim, so cdrskin is the better choice if it works), disable cdrecord-specific workarounds (start by looking at those workarounds already disabled for wodim in the code you removed, but keep in mind that cdrskin is not a fork of cdrecord as wodim is, so probably does not need ANY of the cdrecord workarounds), etc.

I have seen a bunch of cdrecord workarounds while browsing K3b code in the past, but unfortunately I do not remember the exact locations in the code. For example, there is something that says that cdrecord fails at doing some things for audio CDs in DAO mode, so cdrdao (another unmaintained tool (no news since 2009), that in addition also uses libraries from cdrtools/cdrkit, so also uses the unmaintained wodim code) is used for those instead. (Searching for "cdrdao" will probably find that particular one.) You really want to go through all the burning code and check whether what it is doing still makes sense if cdrecord is actually cdrskin.
Comment 10 Kevin Kofler 2016-11-09 06:04:35 UTC
Also, please keep in mind that cdrtools/cdrkit is not only cdrecord/wodim, but also mkisofs/genisoimage. You will also want to detect and use xorrisofs from libburnia, which is likely a better mkisofs replacement than genisoimage.
Comment 11 Leslie Zhai 2016-11-09 06:34:22 UTC
Git commit abe673e2f1b9b016d3a9c3aaad42d808a80a0b4a by Leslie Zhai.
Committed on 09/11/2016 at 06:30.
Pushed by lesliezhai into branch 'cdrskin'.

Restore wodim support

Although wodim from cdrkit (CD only, DVD deprecated, unmaintained) was
unmaintained, Kevin Kofler suggested restoring it. Please review it, if
it looks good to you, I will merge to master.

CCMAIL: kevin.kofler@chello.at

M  +4    -1    libk3b/core/k3bdefaultexternalprograms.cpp
M  +1    -1    libk3b/jobs/k3bdvdcopyjob.cpp
M  +1    -0    libk3b/jobs/k3bmetawriter.cpp
M  +8    -2    libk3b/projects/datacd/k3bdatajob.cpp
M  +1    -1    libk3b/projects/k3bcdrecordwriter.cpp
M  +1    -1    src/k3bsystemproblemdialog.cpp
M  +2    -1    src/option/k3bexternalbinpermissionmodel.cpp

http://commits.kde.org/k3b/abe673e2f1b9b016d3a9c3aaad42d808a80a0b4a
Comment 12 Kevin Kofler 2016-11-09 06:39:20 UTC
Looks OK to me.