Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ion failing to raise an exception when decrypt fails #662

Merged
merged 2 commits into from
Nov 10, 2024

Conversation

C0rn3j
Copy link

@C0rn3j C0rn3j commented Oct 27, 2024

ex is never set to anything but None, so it just tries to raise None and fails.

There are also small whitespace cleanups and moving commented docs to a docstring.

Future import added (stabilized in 3.14) as the exception variable now has an annotated type.

Review with hidden whitespace changes for saner view - https://github.com/noDRM/DeDRM_tools/pull/662/files?diff=split&w=1

Fixes:

Traceback (most recent call last):
  File "/home/user/.config/calibre/plugins/DeDRM.zip/kfxdedrm.py", line 105, in decrypt_voucher
    voucher.decryptvoucher()
  File "/home/user/.config/calibre/plugins/DeDRM.zip/ion.py", line 1368, in decryptvoucher
    raise ex
          ^^
UnboundLocalError: cannot access local variable 'ex' where it is not associated with a value

Issues that ran into it - #434 #472 #561

Built release with this change (in case anyone cares, mostly left it here for myself)

@noDRM
Copy link
Owner

noDRM commented Nov 10, 2024

Thanks, that does seem to be a sensible fix.

@noDRM noDRM merged commit 34c4c06 into noDRM:master Nov 10, 2024
@C0rn3j C0rn3j deleted the ex branch November 10, 2024 13:32
@C0rn3j
Copy link
Author

C0rn3j commented Nov 10, 2024

I see you have added back the Python 2 UTF header in 815d86e, is there a reason why?

It looks like this project does the usual thing of telling people with dead Python(and Calibre) versions to use old versions of the plugin: "Users with calibe 4.x or earlier should use release 6.8.x of the tools."

Afaik this PR itself won't work on any unsupported Python version (sans the freshly-EOL 3.8), as they don't support the required typing features.

EDIT: Oh, there's a copied readme from two projects.

I suggest dropping the old versions, not having typing available is going to lead to bugs like these, the main issue this PR fixes is highlighted even with a basic linter/LSP, which is Ruff/Jedi in my case.

EDIT2: Fixed the readme - #671

@noDRM
Copy link
Owner

noDRM commented Nov 10, 2024

Yeah, the readme is kind of a mess ...

The release 7.2.0 from Apprentice Harper years ago did drop support for Python 2 (Calibre < 5) and this recommendation was added, but in my fork I later decided to add back Python 2 support so people that were stuck on an old Calibre installation for whatever reason could still enjoy all the new features, so that's why I added back the UTF8 header (because IIRC it's needed for Python 2).

As for the type hint - I simply missed that that's not yet supported in Python 2. For the time being, I think i'm going to replace that with a # type: Exception | None comment that should work on both Python 2 and Python 3. Maybe it is time to reconsider dropping Python2 support eventually, but for now I haven't really ran into any situations where I absolutely needed to have a Python3+ feature.

A few years ago quite a few people were still stuck on Python2, but maybe the number has decreased to a point where it might be worth it to drop Python2 support. But if i decide to do that I'll definitely need to make a proper last release with Python2 support instead of the current, rolling-release 10.0.9 builds.

I'll take a look at your readme MR later.

@C0rn3j
Copy link
Author

C0rn3j commented Nov 10, 2024

You should probably also comment out the future import since you commented out the annotation, unsure how well dead Python versions handle it.

so people that were stuck on an old Calibre installation for whatever reason could still enjoy all the new features

I think that today, the use case is gone.

From the eBooks ecosystem, even Kindle mods use Python 3 nowadays, and those took even longer to switch than Calibre itself.

I haven't really ran into any situations where I absolutely needed to have a Python3+ feature.

This bug fix itself should highlight the usefulness of the typing system, the issue is marked as an error in an LSP.

You'll likely find many more.

People have had some 17 years to migrate, including 5 years since Python 2 was marked EOL.

Making a final P2 release, tearing out support and switching to modern, secure and support Python versions will be beneficial.

@noDRM
Copy link
Owner

noDRM commented Nov 10, 2024

Thanks for the note regarding the import, I'll remove that as well.
The typing system also works while keeping it compatible with Python 2, by using the syntax I now used for this statement (the type comments). Also, yes, it's been 17 years since Python 3 was released and 5 years since Python 2 was marked EOL, but that doesn't mean people had 17 years to migrate.

The first Python 3 versions of Calibre were released at the end of 2020, so, like 4 years ago. If I remember, the very first Calibre versions with Python 3 also had a bunch of teething problems, and they also dropped support for Windows 7. Yes, Windows 7 is also already EOL, but only since 2023. So it wasn't too surprising that people were still on older Calibre builds up until then. Linux distributions like Ubuntu 20.04 (which is still actively being supported for another year or so) are also shipping with Calibre < 5 (Python2).

As for the many more errors stricter typing would catch - i definitely agree, it would. And for new code it's probably best to write it with the correct typing indicators, whether that would use the Python3-only syntax or the comment syntax also compatible with Python2. So it's not like A) Python-2-support currently prevents me from using proper typing, nor would B) dropping Pytho 2 immediately result in better code quality, it would still require me or someone else to update all the code to actually make use of newer Python3 features. All while making sure not to break existing code because I don't even have books with all the different DRM types to test this with.

When I started working on the plugins back in mid-2021 (half a year after the first Python3 release), almost a third of all users were still on a Python 2 build of Calibre, that's why I added back Python2 support.

That said, looking at Calibre's usage statistics now, the amount of people still on Calibre 4 or older seems to be drastically dropping (thankfully). November 21 it was ~30%, November 22 it was 19%, November 23 it was 13% and now it's "only" 10%. So maybe it's time to think about dropping support for Python2 eventually.

@C0rn3j
Copy link
Author

C0rn3j commented Nov 10, 2024

Calibre's usage statistics
now it's "only" 10%.

That exists, huh, neat!
Looks like 8.7% unless I am mathing wrong... Of installations using Python 2-based Calibre over the internet... that is scary.

syntax also compatible with Python2

It's unfortunately however incompatible with the tooling and presumably, Python itself won't toss an error if you add a nonsensical type:
image

image

Windows 7 is also already EOL, but only since 2023

Windows 7 itself is EOL since 2020, supporting software on insecure operating systems is in my opinion a disservice to the user, rather than giving them a positive reason to migrate to a secured version and informing them, they will keep using the insecure software, possibly while being ignorant about it.
Which is not good for either them, or the rest of the internet, which is going to get a new noisy neighbour when they inevitably get the system compromised.

Linux distributions like Ubuntu 20.04 (which is still actively being supported for another year or so) are also shipping with Calibre < 5 (Python2).

It will in fact be supported even way beyond that with a subscription, which is necessary in the first place, as Calibre is in Canonical's Universe repository, which does not get guaranteed security updates outside of a Ubuntu Pro subscription.

The software ecosystem getting the burden of maintaining legacy versions simply because someone slaps a supported sticker on a software package abandoned by upstream years ago is not necessarily a good thing.

People can always install a newer version than the provided out of date one.

it would still require me or someone else to update all the code to actually make use of newer Python3 features

That someone else first needs to see the project is actually open to being maintained :)
For me personally it would be very unfun to maintain this in the current state:

  • The tooling won't work in some cases, unfixable in the current legacy state
  • On my setup, half the codebase gets marked red, due to old-style hardcoded indentation-width, which is not even consistent. That's over using tabs and allowing people to use whatever indent-width preference they have.
    • Hardcoding indent width is advised in PEP-8, which is far from the only questionable bit of advice in there
  • Support legacy versions with their own quirks is a major bother and I don't even know which there are

I would suggest:

  • Adding some basic linter rules (Ruff is great)
  • Adopting a LSP for yourself and suggesting it to others interested in contributing(Jedi/Pyright)
  • Dropping everything but the supported Python versions
  • Adopting the typing system
  • Tabs over spaces - this tends to be needlessly controversial, it boils down to enabling indent-width preference rather than forcing whatever the project uses onto everyone - I enjoy 2, htop uses 3, this project uses 2-4 at random, Linux uses 8... everyone has an idea - let everyone have an idea, but in their own editor.
  • Having some smaller file converted to the above, to point others interested in the maintenance to as an example of how it should look like

Dropping old versions will make it easier to do CI too.
And hey, if breakage happens, old code and releases are still available to test if they'll decrypt DRM files that are reported to not work.
There could also be breakage already that you don't know about.

On the end note, massive thanks for keeping this project alive!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants