bontchev / pcodedmp

A VBA p-code disassembler
GNU General Public License v3.0
456 stars 85 forks source link

Skip win_unicode_console import for linux #9

Closed christian-intra2net closed 5 years ago

christian-intra2net commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

:) 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.

decalage2 commented 5 years ago

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

bontchev commented 5 years ago

OK, closed.

christian-intra2net commented 5 years ago

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

bontchev commented 5 years ago

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

christian-intra2net commented 5 years ago

I see. Thank you!