I have inherited a python script that is essentially a shell script, it uses repeated calls to os.system() to run a bunch of command line utilities without necessarily checking the result. In order to make the script a little more rugged, I was thinking of adding class methods like below:

    def subprocess(self, cmd):
        proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
        while proc.poll() is None:
            if proc.wait() == 0:
                return True
            else:
                return False

    def check_package_installed(self, pkg):
       cmd = " ".join(["dnf list installed", pkg])
       rv = self.subprocess(cmd)
       if rv != True:
          raise Exception("Package {} not installed.".format(pkg)))

And then, in the main section of the script, something like this:

try:
    checker.check_package_installed("jdkasjdsa")
except Exception as e:
    print("Error: ",e)
    # recovery code here

Is this the kind of idiomatic code another Python programmer would expect for this task?

CodePudding user response：

1. Using of Exception is little bit wide.

   Let's suppose that your pkg will be integer. Then join operation will be failed with TypeError. However TypeError is subclass for Exception and that means that you will try to run recovery code here even if different Exception raised

2. Looks like you creating script for internal usage and that is not a big problem, but generally arguments passed to command should be escaped

3. Your command result check logic is awkward and fragile (most important problem)

You may try this code:

import subprocess
import shlex


# this Exception class allows us to except exact Exceptions only
class PackageNotFoundException(OSError):
  pass


class UnexpectedException(OSError):
  pass


class Checker:

  # using static method we able to run this method without creating instance
  @staticmethod
  def check_package(pkg_name: str):
    # shlex.quote(pkg_name) -> escape command line parameter
    result = subprocess.Popen(
      "/usr/bin/env dnf list installed "   shlex.quote(pkg_name),
      shell=True,
      # hide output
      stdout=subprocess.DEVNULL,
      stderr=subprocess.PIPE
    )

    _, err = result.communicate()
    err = err.decode('utf8').rstrip('\r\n')
    exit_code = result.wait()

    # check exact error (dnf returns 1 if package not found)
    if exit_code == 1 and 'No matching Packages' in err:
      raise PackageNotFoundException(f"Package not found {pkg_name}")
    # if command status code is not 0
    if exit_code != 0:
      raise UnexpectedException(f"Error [{err}] with status code {exit_code} happens when trying to check {pkg_name}")

package = "package name with spaces"

try:
  Checker.check_package(package)
# handle package not found error
except PackageNotFoundException as e:
  print(f"Trying to install package {package}")
  # recovery code here
# re-raise error if unknown (unexpected) error
except Exception as e:
  raise e

CodePudding user response：

My answer was on similar lines as @juanpa.arrivillaga comment. Instead of using Popen you can use subprocess.run command using the below example. The subprocess.run will wait for the command to complete and it will return a CompletedProcess instance. If you dont care output you can use pass check=True and it will raise subprocess.CalledProcessError exception if the command returns error code !=0.

import subprocess
class CheckProcess:

    def execute_cmd(self, cmd):
        result = False
        try:
            output = subprocess.run(cmd, shell=True, check=True)
            result = True
        except subprocess.CalledProcessError as exc:
            print('Exception hit: %s', exc)
        else:
            return result

    def check_package_installed(self, pkg):
       cmd = " ".join(["dnf list installed", pkg])
       result = self.execute_cmd(cmd)
       if result != True:
          raise Exception("Package {} not installed.".format(pkg))

c = CheckProcess()
c.check_package_installed('cchfhfjgj')

Output

Exception hit: %s Command 'dnf list installed cchfhfjgj' returned non-zero exit status 127.

---------------------------------------------------------------------------

Exception                                 Traceback (most recent call last)

<ipython-input-2-029eeeb6eef7> in <module>()
     19 
     20 c = CheckProcess()
---> 21 c.check_package_installed('cchfhfjgj')

<ipython-input-2-029eeeb6eef7> in check_package_installed(self, pkg)
     16        rv = self.subprocess(cmd)
     17        if rv != True:
---> 18           raise Exception("Package {} not installed.".format(pkg))
     19 
     20 c = CheckProcess()

Exception: Package cchfhfjgj not installed.

Please read the subprocess.run documentation specific to Python version that you are using on your system.