boramalper / himawaripy

Set near-realtime picture of Earth as your desktop background
http://labs.boramalper.org/himawaripy
MIT License
1.63k stars 239 forks source link

is_running has locale issues, returns true on partial matches #11

Closed rbong closed 8 years ago

rbong commented 8 years ago

When I run the script, I get an error and the following traceback:

Traceback (most recent call last):
  File "/usr/lib/python3.5/pdb.py", line 1661, in main
    pdb._runscript(mainpyfile)
  File "/usr/lib/python3.5/pdb.py", line 1542, in _runscript
    self.run(statement)
  File "/usr/lib/python3.5/bdb.py", line 431, in run
    exec(cmd, globals, locals)
  File "<string>", line 1, in <module>
  File "/home/rbong/programs/himawaripy/himawaripy.py", line 3, in <module>
    from io import BytesIO
  File "/home/rbong/programs/himawaripy/himawaripy.py", line 55, in main
    de = get_desktop_environment()
  File "/home/rbong/programs/himawaripy/utils.py", line 45, in get_desktop_environment
    elif is_running("xfce-mcs-manage"):
  File "/home/rbong/programs/himawaripy/utils.py", line 73, in is_running
    if re.search(process, x):
  File "/usr/lib/python3.5/re.py", line 173, in search
    return _compile(pattern, flags).search(string)
TypeError: cannot use a string pattern on a bytes-like object

The offending function is this:

def is_running(process):
    ...
    try: # Linux/Unix
        s = subprocess.Popen(["ps", "axw"],stdout=subprocess.PIPE)
    except: #Windows
        s = subprocess.Popen(["tasklist", "/v"],stdout=subprocess.PIPE)
    for x in s.stdout:
        if re.search(process, x):
            return True
    return False

The issue for me is that when it uses re.search(process, x), x is a bytearray. subprocess gets its locale from locale.getlocale. This solution would work with both strings and any encoding.

...
    for x in s.stdout:
        if re.search(process, str (x)):
            return True

However, this function has another problem.

>>> is_running('firefox')
True
>>> is_running('firefo')
True

As you might've guessed, I don't have a process called "firefo" running on my system. We could tweak the search function a bit with regex, but it's not worth it. Here's a better solution for the whole function.

def is_running(process):
    try:
        subprocess.check_output (["pidof", "--", process])
        return True
    except subprocess.CalledProcessError:
        return False

Since "--" blocks further arguments (for my implementation of pidof, at least) the function should only throw subprocess.CalledProcessError if the function is not running according to my pidof man page. Here are the results of a test:

>>> is_running('firefox')                                      
True
>>> is_running('firefo')
False

There are two downsides to this method. The first is that it is not portable on Windows (however you do not support Windows in this fork). The second is that you cannot search for processes by regular expression as re's search function supports (however you do not use regex in this fork). It is for these reasons I'm going to hold off on a pull request, as I'm sure you have your own ideas about the best way to fix this function that preserves the potential for extension you appear to have planned in the original function.

predator5047 commented 8 years ago

Didn't this commit 6b822b53928e452786df9ee36628889f7bec4b8a fixed the issue? For me it now works.

rbong commented 8 years ago

I somehow got an old commit.

Edit: Began putting together this issue before the commit.

boramalper commented 8 years ago

Many thanks! I completely agree with you, modifying now. :)