-
Notifications
You must be signed in to change notification settings - Fork 583
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
Python3 Support for Loki #123
Conversation
Code Refactoring to support Loki usage in Python3.7
Add Python 3 Support
Minor Change to use the private rules in Python3
Python3 Support Added
To compile it correctly from scratch you need to fix the codecs library in particular the write function |
def calculate_doublepulsar_xor_key(s): | ||
x = (2 * s ^ (((s & 0xff00 | (s << 16)) << 8) | (((s >> 16) | s & 0xff0000) >> 8))) | ||
x = x & 0xffffffff # this line was added just to truncate to 32 bits | ||
return x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No newline: PEP8 violation
import re | ||
import psutil | ||
try: | ||
from io import StringIO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it python2 incompatible, I believe. It was compatible with both before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it there but StringIO is not used in the code, because in py3 you should use BytesIO, can be an option using both to assure py2 compatability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point here is that StringIO is located in different modules between the two python versions. Your change makes it so it tries io, and if that fails it just tries io again, which we can expect to fail again. By trying to import it from two different modules, as it was before, it should work for both py2 and py3.
The StringIO name isn't used in helpers.py, but is used within loki.py itself (without begin imported there, I assume because it's imported here). That's pretty hackish itself, but that's apparently how it was hacked together...
@@ -1,266 +1,265 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No Shebang
date_obj = datetime.datetime.utcnow() | ||
date_str = date_obj.strftime("%Y%m%dT%H:%M:%SZ") | ||
return date_str | ||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No SheBang
lib/pesieve.py
Outdated
except Exception as e: | ||
traceback.print_exc() | ||
self.logger.log("ERROR", "PESieve", "Something went wrong during PE-Sieve scan.") | ||
return(results) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No parens required here: return is not a function, it's a keyword.
Why are all diffs showing up as full-file replacements? Did you change line ends? |
It's working!
…--
----------------------------------------
An outsider, the BaRRaKudaRain
-----Original Message-----
From: "van1sh" <notifications@github.com>
Sent: 5/17/2019 3:33 PM
To: "Neo23x0/Loki" <Loki@noreply.github.com>
Cc: "Subscribed" <subscribed@noreply.github.com>
Subject: [Neo23x0/Loki] Python3 Support for Loki (#123)
I have done the first refactoring to make Loki compatible with Python3, happy to share. I added also the 2 executable compiled and everything seems to work
You can view, comment on, or merge this pull request online at:
#123
Commit Summary
Python3 Support Added
Python3 Support
Python 3 Build File
Python 3 Support to Run PE-Sieve
Python3 Support
Merge pull request #1 from poppopjmp/poppopjmp-patch-1
File Changes
M build.bat (4)
M lib/doublepulsar.py (438)
M lib/helpers.py (645)
M lib/levenshtein.py (86)
M lib/lokilogger.py (531)
M lib/pesieve.py (143)
M lib/pluginframework.py (146)
M lib/privrules.py (229)
M loki-package-builder.py (92)
M loki-upgrader.py (507)
M loki.py (8)
Patch Links:
https://github.com/Neo23x0/Loki/pull/123.patch
https://github.com/Neo23x0/Loki/pull/123.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I'd recommend rejecting this PR in its entirety. Commit descriptions should describe the change, not the file that's changed; that's easy to see elsewhere. White-space only commits (eg LF->CRLF) should be clearly labeled as such. When replacing line ends, diff shows a complete file replacement, making it difficult to locate any code changes. Python should be line-end agnostic, so switching everything to DOS EOLs (CRLF) should not be needed. I know that loki.py used CRLF before the changes, but that appeared to be the only one. I'm all for consistency, but following the principle of least change means loki.py should have been switched to LF, not all others to CRLF. Is that really needed for python3 compatibility? |
Please resync to resolve conflicts. |
I have done the first refactoring to make Loki compatible with Python3, happy to share. I added also the 2 executable compiled and everything seems to work