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

Skip win_unicode_console import for linux #9

Closed
christian-intra2net opened this issue Jul 8, 2019 · 13 comments
Closed

Skip win_unicode_console import for linux #9

christian-intra2net opened this issue Jul 8, 2019 · 13 comments
Assignees

Comments

@christian-intra2net
Copy link

christian-intra2net commented Jul 8, 2019

I am a heavy user of oletools which now requires this module. I was wondering if you would object to wrapping all the win_unicode_console imports and requirements into a test whether the current platform actually is windows. I can create a pull request for this if you approve.

Specifically, in setup.py I would write

INSTALL_REQUIRES = ['oletools>=0.54']
if platform.system().lower() == "windows":
    INSTALL_REQUIRES.append('win_unicode_console'

and similarly in pcodedmp.py I would write:

if platform.system().lower() == "windows":
    import win_unicode_console

This would make the life of many oletools-linux-users easier

@christian-intra2net
Copy link
Author

I just checked, there is a better way to edit the requirements in setup.py using "Environment Markers" as specified in PEP-508: https://stackoverflow.com/a/54281345/4405656

@bontchev
Copy link
Owner

bontchev commented Jul 8, 2019

I'd do this in the develop branch, if necessary (I don't think that just this change requires a new version release), but why is it necessary? I've installed both the tool and the win_unicode_console module on Linux and they work just fine there. I assume the module just does nothing on Linux.

@christian-intra2net
Copy link
Author

christian-intra2net commented Jul 8, 2019

I've checked the code of win_unicode_console, it has the same if platform.system().lower() == "windows": checks. So you are right, the module does nothing.
However, not everybody has the luxury of having pip or other tools install all required modules for them. In my setup, I have to download, package and install all required modules myself. That is a bit of an effort and I would like to avoid it for something that I know I will not need anyway.
So, maybe my remark about "many oletools linux users" may have been too strong, most people will never notice this since pip just does the work for them. However, I am sure I am not the only one not using pip or finding it strange to install a windows-specific module on their linux machine.
And after all it is just a small change...

@decalage2
Copy link
Collaborator

Actually there is another issue related to win_unicode_console: it is not supported on Pypy (on Windows), and triggers an error. I will propose a change so that pcodedmp simply ignores it when win_unicode_console cannot be imported.

@bontchev
Copy link
Owner

bontchev commented Jul 8, 2019

I'll do it, at least based on the principle that I'd rather not install a module somewhere if I can avoid it, but even if you aren't using pip, can't you just do python setup.py install after downloading the project?

It's not just a configuration change, BTW. I'd have to modify the source of the main script too and not try to import or use the win_unicode_console module when not running on Windows.

OK, I think I've made the necessary changes. Please try the develop branch and tell me if it now works as you wanted.

@christian-intra2net
Copy link
Author

Perfect, thanks very much!

Keeping dependencies to a minimum in general is also a very good motive.

And to anwer your question: for my own machine I could just do setup.py install. But I have to distribute the package to several machines and we are using rpm for that, so I have to create an rpm with a few modifications, which is quite a bit more effort.

Issue solved for my part

@christian-intra2net
Copy link
Author

BTW: I've read your homepage and searched for other mentions of your name. I have learned a lot. Thanks for all your impressive work

@bontchev
Copy link
Owner

bontchev commented Jul 9, 2019

:) Yeah, I used to be a big name in the anti-virus field when I was young. Still got a few moves, though. :)

OK, I won't be closing the issue just yet, because @decalage2 has some additional modifications in mind, in order to make the project compatible with PyPy.

@bontchev bontchev self-assigned this Jul 9, 2019
@decalage2
Copy link
Collaborator

I propose to close this one and I'll open another one for the Pypy issue. It's related, but not the same.

@bontchev
Copy link
Owner

bontchev commented Jul 9, 2019

OK, closed.

@bontchev bontchev closed this as completed Jul 9, 2019
@christian-intra2net
Copy link
Author

christian-intra2net commented Jul 25, 2019

May I ask when you plan on merging this into master?

@bontchev
Copy link
Owner

I was waiting for @decalage2's modifications for PyPy compatibility. Merged now, version 1.2.6 released.

@christian-intra2net
Copy link
Author

I see. Thank you!

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

No branches or pull requests

3 participants