Summary: | Please restore wodim support | ||
---|---|---|---|
Product: | [Applications] k3b | Reporter: | Kevin Kofler <kevin.kofler> |
Component: | Burning/Hardware | Assignee: | k3b developers <k3b> |
Status: | RESOLVED FIXED | ||
Severity: | major | CC: | michalm, rdieter, trueg, zhaixiang |
Priority: | NOR | ||
Version: | unspecified | ||
Target Milestone: | --- | ||
Platform: | Fedora RPMs | ||
OS: | Linux | ||
Latest Commit: | Version Fixed In: | ||
Sentry Crash Report: |
Description
Kevin Kofler
2016-11-05 01:40:55 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. And in the long run, to work with something maintained, see wishlist bug #137436. 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.) marking confirmed, fedora will likely need this fixed before we can consider shipping this new version 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/ 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. 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. (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... > 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. 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. 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 Looks OK to me. |