Bugfixes for aria2 and convert_magnet#1608
Conversation
Prevent crash in case both download and aria2 output plugins are enabled at the same time. Download plugin may move the temp .torrent file created by convert_magnet away, causing aria2 plugin to crash. Fixes Flexget#1604
| # for some reason the info_hash needs to be bytes but it's a struct called sha1_hash | ||
| params['info_hash'] = bytes(params['info_hash']) | ||
| lt_version = [int(v) for v in libtorrent.version.split('.')] | ||
| if lt_version[0] > 0 or (lt_version[0] == 0 and lt_version[1] == 16 and lt_version[2] > 13): |
There was a problem hiding this comment.
Could this be more clearly expressed as if lt_version > [0, 16, 13]:? Also, are you sure this is to do with a change since libtorrent 0.16.14, and not something to do with python 2 vs 3? Have, (or can you,) test with both python 2 and 3?
There was a problem hiding this comment.
$ python2.7
Python 2.7.12 (default, Nov 19 2016, 06:48:10)
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> magnet_uri='magnet:?xt=urn:btih:2fd26e37489b543179faefee0febf199b8f35698&dn=Indiana+Jones+and+the+Raiders+of+the+Lost+Ark+%281981%29+720p+BrRip+&tr=udp%3A%2F%2Ftracker.leechers-paradise.org%3A6969&tr=udp%3A%2F%2Fzer0day.ch%3A1337&tr=udp%3A%2F%2Ftracker.coppersurfer.tk%3A6969&tr=udp%3A%2F%2Fpublic.popcorn-tracker.org%3A6969'
>>> import libtorrent
>>> libtorrent.version
'1.0.7.0'
>>> params = libtorrent.parse_magnet_uri(magnet_uri)
>>> session = libtorrent.session()
>>> params['info_hash'] = bytes(params['info_hash'])
>>> handle = libtorrent.add_magnet_uri(session, magnet_uri, params)
>>>
$ python3.5
Python 3.5.2 (default, Nov 17 2016, 17:05:23)
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> magnet_uri='magnet:?xt=urn:btih:2fd26e37489b543179faefee0febf199b8f35698&dn=Indiana+Jones+and+the+Raiders+of+the+Lost+Ark+%281981%29+720p+BrRip+&tr=udp%3A%2F%2Ftracker.leechers-paradise.org%3A6969&tr=udp%3A%2F%2Fzer0day.ch%3A1337&tr=udp%3A%2F%2Ftracker.coppersurfer.tk%3A6969&tr=udp%3A%2F%2Fpublic.popcorn-tracker.org%3A6969'
>>> import libtorrent
>>> libtorrent.version
'1.0.7.0'
>>> params = libtorrent.parse_magnet_uri(magnet_uri)
>>> session = libtorrent.session()
>>> params['info_hash'] = bytes(params['info_hash'])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: 'sha1_hash' object is not iterable
$ python2.7
Python 2.7.6 (default, Oct 26 2016, 20:46:32)
[GCC 4.8.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> magnet_uri='magnet:?xt=urn:btih:2fd26e37489b543179faefee0febf199b8f35698&dn=Indiana+Jones+and+the+Raiders+of+the+Lost+Ark+%281981%29+720p+BrRip+&tr=udp%3A%2F%2Ftracker.leechers-paradise.org%3A6969&tr=udp%3A%2F%2Fzer0day.ch%3A1337&tr=udp%3A%2F%2Ftracker.coppersurfer.tk%3A6969&tr=udp%3A%2F%2Fpublic.popcorn-tracker.org%3A6969'
>>> import libtorrent
>>> libtorrent.version
'0.16.13.0'
>>> params = libtorrent.parse_magnet_uri(magnet_uri)
>>> session = libtorrent.session()
>>> params['info_hash'] = bytes(params['info_hash'])
>>> handle = libtorrent.add_magnet_uri(session, magnet_uri, params)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: No registered converter was able to produce a C++ rvalue of type libtorrent::big_number from this Python object of type str
I don't have python3 on the machine that has libtorrent 0.16.13, so, I can't test it, sorry.
Seems that it is still broken (both before and after this patch) for python 3, for a different reason though.
There was a problem hiding this comment.
Here is the test on python3.5 without the bytes cast:
$ python3.5
Python 3.5.2 (default, Nov 17 2016, 17:05:23)
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> magnet_uri='magnet:?xt=urn:btih:2fd26e37489b543179faefee0febf199b8f35698&dn=Indiana+Jones+and+the+Raiders+of+the+Lost+Ark+%281981%29+720p+BrRip+&tr=udp%3A%2F%2Ftracker.leechers-paradise.org%3A6969&tr=udp%3A%2F%2Fzer0day.ch%3A1337&tr=udp%3A%2F%2Ftracker.coppersurfer.tk%3A6969&tr=udp%3A%2F%2Fpublic.popcorn-tracker.org%3A6969'
>>> import libtorrent
>>> libtorrent.version
'1.0.7.0'
>>> params = libtorrent.parse_magnet_uri(magnet_uri)
>>> session = libtorrent.session()
>>> handle = libtorrent.add_magnet_uri(session, magnet_uri, params)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: No registered converter was able to produce a C++ rvalue of type bytes from this Python object of type sha1_hash
There was a problem hiding this comment.
Oh, how bout we switch it like in the other issue you mentioned lt.big_number(params['info_hash']) instead of bytes(), does that work?
There was a problem hiding this comment.
Like this?
$ python3.5
Python 3.5.2 (default, Nov 17 2016, 17:05:23)
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> magnet_uri='magnet:?xt=urn:btih:2fd26e37489b543179faefee0febf199b8f35698&dn=Indiana+Jones+and+the+Raiders+of+the+Lost+Ark+%281981%29+720p+BrRip+&tr=udp%3A%2F%2Ftracker.leechers-paradise.org%3A6969&tr=udp%3A%2F%2Fzer0day.ch%3A1337&tr=udp%3A%2F%2Ftracker.coppersurfer.tk%3A6969&tr=udp%3A%2F%2Fpublic.popcorn-tracker.org%3A6969'
>>> import libtorrent
>>> libtorrent.version
'1.0.7.0'
>>> params = libtorrent.parse_magnet_uri(magnet_uri)
>>> session = libtorrent.session()
>>> params['info_hash'] = libtorrent.big_number(params['info_hash'])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
Boost.Python.ArgumentError: Python argument types in
sha1_hash.__init__(sha1_hash, sha1_hash)
did not match C++ signature:
__init__(_object*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)
__init__(_object*)
|
Shoot. Maybe |
|
Yes, it is working properly for both 0.16.13 and 1.0.7 when using python2.7 after the patch. Can't cast params['info_hash'] to bytes(): But I think I found the fix for python3: |
|
There is still one detail that needs fixing, unrelated to the previous fixes... Options from the config file aren't being passed to aria2 when using this code path... Fix in a minute.. |
|
I'm not too happy about having multiple unrelated plugin changes in the same PR. |
| lt_version = [int(v) for v in libtorrent.version.split('.')] | ||
| if lt_version > [0,16,13,0]: | ||
| # for some reason the info_hash needs to be bytes but it's a struct called sha1_hash | ||
| params['info_hash'] = params['info_hash'].to_bytes() |
There was a problem hiding this comment.
From what I understand, the info_hash value still has to be set for older versions of libtorrent. It just expects a different type.
There was a problem hiding this comment.
@cvium It's already in the return value of params, it's just that certain versions of libtorrent can't handle feeding its own data structure back in as 'info_hash', so this just does the cast. At least that's my understanding.
There was a problem hiding this comment.
Oh, you're probably right. Hadn't read it properly it seems.
There was a problem hiding this comment.
This is correct. Problem is that the C++ bindings don't support custom data structures, so, it has to be converted to some standard. LT <= 0.16.13 simply used a string to represent the info_hash, and later versions use a custom struct.
Why libtorrent doesn't handle it by itself in the python-libtorrent glue? I have no idea, they should, IMO. Code above is ugly, and this caused breakage on pretty much every project that uses libtorrent.
Yeah, separate PRs for separate problems would have been nice. Especially since we can easily squash PRs at the end. Don't want to squash these 2 fixes into one commit though. |
|
It's actually 4 bugfixes, for things I only found after starting the first 2...
My original intention was to have a separate/single commit/pr for 1 and 2, in a way that it could be easily cherry-picked individually, but I only found 3 and 4 later, after sending out the PR and after gazpachoking's review. Unless there is a reason to not accept one of the fixes and accept the others, I don't see a point in having these as separate PRs, IMO, it's better to not squash anything and keep the full commit history. And even in this case, git allows you to cherrypick any of the individual commits if you don't want all 4 fixes. I can still reshuffle things here and send another N PRs if you guys really want it, but there is no benefit from doing it at the end, bissection would be just the same if you just do a normal merge and keep commit history. |
|
Having one commit for logical things, (whether that be from squashing separate prs, or just having clean commits,) is nice though when looking through the history, (or blame for a file,) rather than a bunch of jumbled stuff that normally ends up in there like 'typo', 'clean up docs' 'tweak whatever', 'oops', which are out of context and don't provide meaningful info. When just a small amount of code is getting touched I don't think the separation of commits adds quite as much value. For more complicated changes squashing doesn't seem as appropriate. That said, I'm not super fussed either way.
Doesn't matter here I don't think. Just meant as a guideline in the future. Makes it easier to get stuff in right away instead of tying it up with other changes. I can assure you, having to manually cherry pick a commit rather than push the merge button on github could substantially delay a PR (in more places than just here.) I'm not trying to lecture or scold btw, sorry if I'm coming across that way. Thanks for the contribution, and all your debugging work to get things working properly! 🍰 |
|
I understand. I will keep that in mind for my next contributions. Thanks
for merging. :)
2017-01-05 19:36 GMT-02:00 Chase Sterling <notifications@github.com>:
… Having one commit for logical things, (whether that be from squashing
separate prs, or just having clean commits,) is nice though when looking
through the history, (or blame for a file,) rather than a bunch of jumbled
stuff that normally ends up in there like 'typo', 'clean up docs' 'tweak
whatever', 'oops', which are out of context and don't provide meaningful
info. When just a small amount of code is getting touched I don't think the
separation of commits adds quite as much value. For more complicated
changes squashing doesn't seem as appropriate. That said, I'm not super
fussed either way.
Unless there is a reason to not accept one of the fixes and accept the
others, I don't see a point in having these as separate PRs, IMO, it's
better to not squash anything and keep the full commit history. And even in
this case, git allows you to cherrypick any of the individual commits if
you don't want all 4 fixes.
Doesn't matter here I don't think. Just meant as a guideline in the
future. Makes it easier to get stuff in right away instead of tying it up
with other changes. I can assure you, having to manually cherry pick a
commit rather than push the merge button on github could substantially
delay a PR (in more places than just here.)
I'm not trying to lecture or scold btw, sorry if I'm coming across that
way. Thanks for the contribution, and all your debugging work to get things
working properly! 🍰
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1608 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAhRBkfzafCOoitdxjOzvFIQD9lupO63ks5rPWJXgaJpZM4LbVpp>
.
--
Daniel Ribeiro
|
Fixes #1514 and #1604