Bug#947393: calibre: Unneeded patches deviate from upstream for no gain.
On 12/26/19 1:05 AM, Norbert Preining wrote: >> Fedora has a patch which completely prevents the application from >> checking for application updates, but leaves the plugin updater check >> running. Perhaps you would find this interesting? > > Really? The patch seems to remove the plugins update check code, too, at > least to my eyes? But yeah, this is where I would patch code if > necessary. > >> https://src.fedoraproject.org/rpms/calibre/blob/master/f/calibre-no-update.patch > ... > -try: > -update_plugins = > get_plugin_updates_available(raise_error=True) > -if update_plugins is not None: > -plugins_update_found = len(update_plugins) > It's too late at night for me to analyze this, but I have vague memories of looking at the patch and figuring that was what it did or was intended to do. Could be I misremembered. Maybe I'll take another look. Either way, that's indeed the logical place to patch it, so yeah. -- Eli Schwartz Arch Linux Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Bug#947393: calibre: Unneeded patches deviate from upstream for no gain.
Hi Eli, On Wed, 25 Dec 2019, Eli Schwartz wrote: > Could be. The patch doesn't clearly explain the rationale, so without I send a message to debian-devel to reconfirm. Thanks for the details. > Fedora has a patch which completely prevents the application from > checking for application updates, but leaves the plugin updater check > running. Perhaps you would find this interesting? Really? The patch seems to remove the plugins update check code, too, at least to my eyes? But yeah, this is where I would patch code if necessary. > https://src.fedoraproject.org/rpms/calibre/blob/master/f/calibre-no-update.patch ... -try: -update_plugins = get_plugin_updates_available(raise_error=True) -if update_plugins is not None: -plugins_update_found = len(update_plugins) > Well, it cannot because it doesn't run as root... Ok, maybe I need to check how Debian builds packages on the build servers, I guess it is sbuild (while I use cowbuilder at home). Still, failing there doesn't sound right, but ... > I used to use XDG_DATA_DIRS in my recipe, until I eventually submitted > https://github.com/kovidgoyal/calibre/commit/2a63948440fe2d60a5573b829f27000d5c0389e2 > upstream to do this automatically when staging an install into a > packaging root. THanks, will look into my own patch again and see if it is still necessary, maybe it can still be dropped, but .. > xdg-desktop-menu does have some strange interactions with > /usr/share/gnome/apps, but AFAICT it manages to disable those bits if > the directory is not writable by the invoking user. Yes, that rings a bell with me, too, ... I will check whether the changes done by you above in upstream code make the changes of mine irrelevant. Thanks for the pointer. > Way back in the day, [...] let us forget these time. I wasn't working on Calibre, and I also don't feel responsible for what was done back then, but I will try to fix as much as possible, time, energy, and information allowing! > Taking a look at the current packaging you have, I'm happy to say that > nothing jumps out at me as problematic anymore. Thanks for your work > improving calibre on Debian. :) Thanks, hearing this from you (you have been a very consistent critique of the Debian packages on mobileread etc), I feel a bit relieved. And big thanks for all you valuable suggestions. Have some relaxed holidays (if you have), a good start into the new year, and I am looking forward to good collaboration in the next year! Best Norbert -- PREINING Norbert http://www.preining.info Accelia Inc. + IFMGA ProGuide + TU Wien + JAIST + TeX Live + Debian Dev GPG: 0x860CDC13 fp: F7D8 A928 26E3 16A1 9FA0 ACF0 6CAC A448 860C DC13
Bug#947393: calibre: Unneeded patches deviate from upstream for no gain.
On 12/25/19 9:48 PM, Norbert Preining wrote: > Hi Eli, > > thanks for the report. [...] >> - Disable-update-check-by-default.patch >> >> While I'm on the topic of Debian-specific patches... this patch seems to >> cause calibre to start up with the update checker disabled, but that >> will disable both the application version check and the plugins version >> check, and I don't believe the latter is appropriate to disable. Please >> check if this is overreaching, and find a more targeted patch if so. > > Indeed, but I think this was the original idea. Programs should not > start up and contact outside entities by default - at least this is what > I learned back then, maybe this is not a requirement anymore. Could be. The patch doesn't clearly explain the rationale, so without insider information I cannot be sure whether the intention was to prevent contacting a server, or just to prevent confusing messages about an update that isn't applicable to a stable release. For what it's worth, this is linked to: https://calibre-ebook.com/dynamic/calibre-usage "Usage statistics are collected whenever a user starts calibre. Every calibre installation has a unique ID, this ID remains unchanged by upgrades and even an uninstall/re-install. This ID is used to collect usage statistics. Only this ID is stored, no other identifying information is collected." So per the developer's promise, nothing is logged other than a UUID, operating system, version, and icon theme (if any). That last bit of information will be used in Preferences -> Look & Feel -> Change icon theme, in order to sort by popularity. I don't know if that commitment is sufficient for Debian. > I will check back with debian-devel. I would change the patch to make > it only check for updated plugins. We don't want users to try to mess up > the dpkg version with some hand-updates as root, that will not work out. That's a reasonable concern. My distro provides updates usually within a day (well, we are rolling), so I don't bother to disable this as generally no one is going to sudo install anything. Fedora has a patch which completely prevents the application from checking for application updates, but leaves the plugin updater check running. Perhaps you would find this interesting? https://src.fedoraproject.org/rpms/calibre/blob/master/f/calibre-no-update.patch > The patches that will be used (atm) for the package I am preparing are > - Disable-update-check-by-default.patch > see above > - Fix-desktop-integration-installation.patch > I am surprised that on Arch nothing like this is necessary ... > When I do this on Debian, the calibre installer tries to > actually install into system directories Well, it cannot because it doesn't run as root... I used to use XDG_DATA_DIRS in my recipe, until I eventually submitted https://github.com/kovidgoyal/calibre/commit/2a63948440fe2d60a5573b829f27000d5c0389e2 upstream to do this automatically when staging an install into a packaging root. I haven't run across any cases of it attempting to install to system directories when using this. xdg-desktop-menu does have some strange interactions with /usr/share/gnome/apps, but AFAICT it manages to disable those bits if the directory is not writable by the invoking user. > - no-detach-in-desktop-files > also something reasonable I think > - Hardening-Qt-code.patch > same with that > So I really think that Debian packages do **not** degress from upstream > as far as it is always assumed. The only *functional* change is the > update check, which, as I wrote, is a requirement AFAIR of Debian. To be fair, the package is a lot better now that it's using the upstream-approved mathjax bundle, and now that both Debian and calibre agree on whether markdown is a system module. Also several fixes that resulted from the forum thread at https://www.mobileread.com/forums/showthread.php?t=288093 Way back in the day, when cherrypy was still in use by the calibre codebase, that was broken on Debian too... there were a whole bunch of local modifications in the calibre fork. I actually submitted a rather snarky bug report to Arch Linux about that, since it was broken there too, although that was before I joined the packaging team. Taking a look at the current packaging you have, I'm happy to say that nothing jumps out at me as problematic anymore. Thanks for your work improving calibre on Debian. :) -- Eli Schwartz Arch Linux Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Bug#947393: calibre: Unneeded patches deviate from upstream for no gain.
Hi Eli, thanks for the report. > - Fix-Markdown-module-loader.patch > - Remove-Qt4-hack.patch/ Disabled for the next package version based on 4.99.2 > - Use-packaged-instead-of-bundled-feedparser-Python-module.patch > - Don-t-load-external-URLs-for-privacy.patch Not actually used, please look into debian/patches/series for actually used patches (ok, I should clean up!) > - Disable-update-check-by-default.patch > > While I'm on the topic of Debian-specific patches... this patch seems to > cause calibre to start up with the update checker disabled, but that > will disable both the application version check and the plugins version > check, and I don't believe the latter is appropriate to disable. Please > check if this is overreaching, and find a more targeted patch if so. Indeed, but I think this was the original idea. Programs should not start up and contact outside entities by default - at least this is what I learned back then, maybe this is not a requirement anymore. I will check back with debian-devel. I would change the patch to make it only check for updated plugins. We don't want users to try to mess up the dpkg version with some hand-updates as root, that will not work out. The patches that will be used (atm) for the package I am preparing are - Disable-update-check-by-default.patch see above - Fix-desktop-integration-installation.patch I am surprised that on Arch nothing like this is necessary ... When I do this on Debian, the calibre installer tries to actually install into system directories - no-detach-in-desktop-files also something reasonable I think - Hardening-Qt-code.patch same with that So I really think that Debian packages do **not** degress from upstream as far as it is always assumed. The only *functional* change is the update check, which, as I wrote, is a requirement AFAIR of Debian. Thanks Norbert -- PREINING Norbert http://www.preining.info Accelia Inc. + IFMGA ProGuide + TU Wien + JAIST + TeX Live + Debian Dev GPG: 0x860CDC13 fp: F7D8 A928 26E3 16A1 9FA0 ACF0 6CAC A448 860C DC13
Bug#947393: calibre: Unneeded patches deviate from upstream for no gain.
Package: calibre Version: 4.6.0+dfsg-1 The current version of calibre has some patches which should probably be removed. - Fix-Markdown-module-loader.patch This is simply incorrect. The upstream code this is patching out, works with the system markdown but ensures that existing code which tries to import the old "calibre.ebooks.markdown" vendored version, continues to work by correctly proxying to the system version. This is implemented via a meta_path hook which intercepts "import " statements. Previous versions of calibre in debian incorrectly did a tree-wide sed to "fix" the vendored version of markdown, but the correct solution is to simply drop all custom modifications and trust upstream to get it right -- not add different patches. (The debian patch will break thirdparty plugins that rely on the stable calibre.ebooks API having this historic module. And it's harmless to leave it in.) - Use-packaged-instead-of-bundled-feedparser-Python-module.patch Same here, the meta_path hook ensures "from calibre.web.feeds.feedparser import parse" sees the system version, and I have no idea what the current version of the patch does at all, anymore... calibre already randomizes user agents, what is Debian trying to solve here? - Remove-Qt4-hack.patch/ Why are you doing this? It won't do anything or harm anything, unless someone has a pre-2.0 plugin installed, and this code is meant to prevent such plugins from breaking anything. Sure, it's unlikely such plugins still exist, but... it's not needed in order to package calibre on Debian, so why patch the code? In order to make it prettier? Submit upstream patches, in that case, but this is not Debian's job. - Don-t-load-external-URLs-for-privacy.patch The code you are patching is the code used to generate an HTML file which is uploaded to the upstream author's website, at https://plugins.calibre-ebook.com/ It's entirely reasonable for it to therefore use urls pointing at said website, but, it's not used in the calibre package at all and should therefore be ignored, rather than distracting efforts by making sure such a patch still applies. - Disable-update-check-by-default.patch While I'm on the topic of Debian-specific patches... this patch seems to cause calibre to start up with the update checker disabled, but that will disable both the application version check and the plugins version check, and I don't believe the latter is appropriate to disable. Please check if this is overreaching, and find a more targeted patch if so. -- Eli Schwartz Arch Linux Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
