9
\$\begingroup\$

I'm trying to write a platform independent Python (>=3.6.5) package that has a handful of java .class files that need to compiled using javac and run using java. This is simple enough task but I need to know if user has these installed already and the location of the executable assuming that neither of them is on the system path. That way I can just call the executable by it's path using subprocess.run. Which bring me to the subject of this review a platform independent whereis command.

import os, subprocess
from subprocess import CalledProcessError


def whereis(app):
    """Return a list of locations a given application.

    Relies on the `where` system command in Windows and the `whereis` command in Unix.

    Parameters
    ----------
    app : str
        Can be any application on the system path e.g. java.

    Returns
    -------
    result : list
        A list of locations where applications is installed.

    Useage
    ------
    >>>whereis('javac')
    'C:\\Program Files\\Java\\jdk1.8.0_162\\bin\\javac.exe'
    """

    result = None

    if os.name == "nt":# Windows
        try:
            result = subprocess.check_output("where {}".format(app))

        except CalledProcessError as err:
            print("Application ,",app,", not forund.",err)

    else:# Unix
        try:
            result = subprocess.check_output("whereis {}".format(app))

        except CalledProcessError as err:
            print("Application ,",app,", not found.",err)

    if result is None:
        print("")
        result = []
        return result

    else:
        result = result.decode().replace("\r", "").split("\n")
        result = list(filter(lambda x: len(x)>0, result))
        return result

Questions

  1. Is there a standard library function that already covers this? I couldn't find one but that doesn't mean anything.
  2. Are there any caveats or edge cases that I'm missing?
  3. Can any general improvements be made to the code or docstring?
\$\endgroup\$
7
\$\begingroup\$

A couple of small things:

  1. If you're specifically looking for executable files, you might use which instead of whereis command. From this answer, you can see the difference between the two using whatis:

    $  whatis which
    which                (1)  - shows the full path of (shell) commands
    
    $  whatis whereis
    whereis              (1)  - locate the binary, source, and manual page files for a command
    
  2. Be consistent. You've used two different formatting styles:

    "where {}".format(app)
    

    and

    "Application ,",app,", not forund.",err  # you also have a spelling mistake here (forund -> found) 
    

    I'd recommend you use the former one

  3. You should have at least two spaces before an inline comment

  4. You should put a space after the comma in almost every situation. (e.g: print("Application,",app,", not found.",err) -> print("Application,", app, ", not found.", err))
\$\endgroup\$
4
\$\begingroup\$

Small nit:

# Split this into individual lines - much easier to read
import os, subprocess

Reduce duplicate code and nesting (and also use which) - this is way too complicated for what it needs to be (I didn't test it though):

def whereis(app):
    result = None

    command = 'where'
    if os.name != "nt":# Windows
        command = 'which'

    try:
        result = subprocess.check_output("{} {}".format(command, app))
    except CalledProcessError as err:
        print("Application ,",app,", not found.",err)

    if result is None:
        return []

    result = result.decode().splitlines()
    return [line for line in result if len(line)]
\$\endgroup\$
3
\$\begingroup\$

You can use a ternary conditional operator for determining the command per operating system, making that bit of logic a one liner. Unless necessary for your intended use, I don't see the point in returning an empty list, just return None if your script finds nothing. If you have a bit of code elsewhere that looks roughly like:

if not paths:
    # do something

If paths is an empty list, it'll work exactly the same if we change it to None.

import os
import subprocess

def whereis(app):
    command = 'which' if os.name != 'nt' else 'where'
    try:
        result = subprocess.check_output('{} {}'.format(command, app), stderr=subprocess.STDOUT)
        return result.decode().split()
    except subprocess.CalledProcessError:
        return


if __name__ == '__main__':
    paths = whereis('notepad')

Output:

['C:\\Windows\\System32\\notepad.exe', 'C:\\Windows\\notepad.exe']
\$\endgroup\$
2
\$\begingroup\$

As far as I can see your code has zero escaping for spaces or shell characters, therefore I would assume it is highly insecure and could be easily used to take over / crash / ... the entire application when being passed malicious input.

You really need to be using shlex.quote or something similar, or use which as a non-bash built-in: subprocess.check_output(["which", your_argument])

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.