From 0a9eb12aeea95a7f8b4ed8bf54c32871619d8fc5 Mon Sep 17 00:00:00 2001 From: Sean Brogan Date: Thu, 20 May 2021 10:36:35 -0700 Subject: [PATCH] Resolve LGTM reported errors and warnings (#248) * BaseException issue reported by LGTM. Don't use empty except as this catches KeyboardInterrupt and SystemExits as well. To keep same behavior use Exception as this will catch errors as expected. * Fix staticmethods in WebDependency set two functions to static to match usage. Remove function get_internal_path_root as it is never used. Remove unit tests for function. * LGTM remove unreachable code and unused variables Co-authored-by: Bret Barkelew --- edk2toolext/base_abstract_invocable.py | 6 +++--- edk2toolext/edk2_git.py | 3 +-- edk2toolext/edk2_invocable.py | 5 ++--- edk2toolext/environment/conf_mgmt.py | 4 ++-- .../environment_descriptor_files.py | 2 +- .../environment/extdeptypes/web_dependency.py | 7 ++----- edk2toolext/environment/external_dependency.py | 1 - edk2toolext/environment/repo_resolver.py | 2 +- edk2toolext/environment/uefi_build.py | 12 +++++++++++- edk2toolext/environment/version_aggregator.py | 1 - edk2toolext/omnicache.py | 1 - edk2toolext/tests/test_web_dependency.py | 18 ------------------ setup.py | 4 ++-- 13 files changed, 25 insertions(+), 41 deletions(-) diff --git a/edk2toolext/base_abstract_invocable.py b/edk2toolext/base_abstract_invocable.py index 0cfe921c..955e129b 100644 --- a/edk2toolext/base_abstract_invocable.py +++ b/edk2toolext/base_abstract_invocable.py @@ -93,9 +93,9 @@ def ConfigureLogging(self): md_log_file = self.GetLoggingLevel("md") if(md_log_file is not None): - md_file, md_logger = edk2_logging.setup_markdown_logger(log_directory, - self.GetLoggingFileName("md"), - md_log_file) + edk2_logging.setup_markdown_logger(log_directory, + self.GetLoggingFileName("md"), + md_log_file) logging.info("Log Started: " + datetime.strftime(datetime.now(), "%A, %B %d, %Y %I:%M%p")) diff --git a/edk2toolext/edk2_git.py b/edk2toolext/edk2_git.py index 8bb2360e..ee921a4c 100644 --- a/edk2toolext/edk2_git.py +++ b/edk2toolext/edk2_git.py @@ -68,7 +68,6 @@ def _update_from_git(self): self._logger.error("GIT ERROR for {0}".format(self._path)) self._logger.error(e) raise e - return False def _get_submodule_list(self): submodule_list = [] @@ -273,6 +272,6 @@ def clone_from(self, url, to_path, branch=None, shallow=False, reference=None, * params = ["submodule", "update", "--init", "--recursive"] params.append("--reference %s" % reference) param_string = " ".join(params) - ret = RunCmd(cmd, param_string) + RunCmd(cmd, param_string) return Repo(to_path) diff --git a/edk2toolext/edk2_invocable.py b/edk2toolext/edk2_invocable.py index e1eb6305..c3f863f3 100644 --- a/edk2toolext/edk2_invocable.py +++ b/edk2toolext/edk2_invocable.py @@ -127,7 +127,7 @@ def GetLoggingLevel(self, loggerType): level = self.PlatformSettings.GetLoggingLevel(loggerType) if level is not None: return level - except: + except Exception: pass if(loggerType == "con") and not self.Verbose: @@ -199,9 +199,8 @@ def ParseCommandLineOptions(self): imported_classes = ", ".join(classNameList) # Join the classes together print(f"The module you imported contains {imported_classes}") except Exception: - # Otherwise, oh well we'll just ignore this. raise - pass + settingsParserObj.print_help() sys.exit(1) diff --git a/edk2toolext/environment/conf_mgmt.py b/edk2toolext/environment/conf_mgmt.py index 54981987..decb4833 100644 --- a/edk2toolext/environment/conf_mgmt.py +++ b/edk2toolext/environment/conf_mgmt.py @@ -87,7 +87,7 @@ def _get_version(self, conf_file: str) -> str: try: version = str(float(line.split("=")[1].split()[0].strip())) break - except: + except Exception: pass return version @@ -108,7 +108,7 @@ def _is_older_version(self, conf_file: str, template_file: str) -> bool: logging.debug("Conf version: %s", str(conf)) template = float(self._get_version(template_file)) logging.debug("Template Version: %s", str(template)) - except: + except Exception: logging.error("Failed to get version from file") finally: return (conf < template) diff --git a/edk2toolext/environment/environment_descriptor_files.py b/edk2toolext/environment/environment_descriptor_files.py index 7aeffaac..07c45948 100644 --- a/edk2toolext/environment/environment_descriptor_files.py +++ b/edk2toolext/environment/environment_descriptor_files.py @@ -38,7 +38,7 @@ def __init__(self, file_path): with open(file_path, 'r') as file: try: self.descriptor_contents = yaml.safe_load(file) - except: + except Exception: pass # We'll pick up this error when looking at the data. # diff --git a/edk2toolext/environment/extdeptypes/web_dependency.py b/edk2toolext/environment/extdeptypes/web_dependency.py index c694c0f4..fe9f2f9e 100644 --- a/edk2toolext/environment/extdeptypes/web_dependency.py +++ b/edk2toolext/environment/extdeptypes/web_dependency.py @@ -47,12 +47,14 @@ def __str__(self): """ return a string representation of this """ return f"WebDependecy: {self.source}@{self.version}" + @staticmethod def linuxize_path(path): ''' path: path that uses os.sep, to be replaced with / for compatibility with zipfile ''' return "/".join(path.split("\\")) + @staticmethod def unpack(compressed_file_path, destination, internal_path, compression_type): ''' compressed_file_path: name of compressed file to unpack. @@ -87,11 +89,6 @@ def unpack(compressed_file_path, destination, internal_path, compression_type): _ref.extract(member=file, path=destination) _ref.close() - def get_internal_path_root(outer_dir, internal_path): - temp_path_root = internal_path.split(os.sep)[0] if os.sep in internal_path else internal_path - unzip_root = os.path.join(outer_dir, temp_path_root) - return unzip_root - def fetch(self): url = self.source temp_folder = tempfile.mkdtemp() diff --git a/edk2toolext/environment/external_dependency.py b/edk2toolext/environment/external_dependency.py index dc299a2d..8b627e0f 100644 --- a/edk2toolext/environment/external_dependency.py +++ b/edk2toolext/environment/external_dependency.py @@ -117,7 +117,6 @@ def clean(self): def fetch(self): # The base class does not implement a fetch. logging.critical("Fetch() CALLED ON BASE EXTDEP CLASS!") - pass def verify(self): result = True diff --git a/edk2toolext/environment/repo_resolver.py b/edk2toolext/environment/repo_resolver.py index a03f5095..6616b96b 100644 --- a/edk2toolext/environment/repo_resolver.py +++ b/edk2toolext/environment/repo_resolver.py @@ -235,7 +235,7 @@ def checkout(abs_file_system_path, dep, repo, update_ok=False, ignore_dep_state_ # try to fetch it and try to checkout again logger.info("We failed to checkout this branch, we'll try to fetch") repo.fetch(branch=branch) - result = repo.checkout(branch=branch) + repo.checkout(branch=branch) repo.submodule("update", "--init", "--recursive") else: if repo.active_branch == dep["Branch"]: diff --git a/edk2toolext/environment/uefi_build.py b/edk2toolext/environment/uefi_build.py index 39af531e..9f6b0a31 100644 --- a/edk2toolext/environment/uefi_build.py +++ b/edk2toolext/environment/uefi_build.py @@ -172,10 +172,20 @@ def Go(self, WorkSpace, PackagesPath, PInHelper, PInManager): logging.critical("Flash Image failed") return ret - except: + except Exception: logging.critical("Build Process Exception") logging.error(traceback.format_exc()) return -1 + + except SystemExit: + logging.critical("Build Process Exit") + logging.error(traceback.format_exc()) + return -1 + + except KeyboardInterrupt: + logging.critical("Build Cancelled by user") + return -2 + finally: end_time = time.perf_counter() elapsed_time_s = int((end_time - start_time)) diff --git a/edk2toolext/environment/version_aggregator.py b/edk2toolext/environment/version_aggregator.py index ccc90116..a1ddbb1a 100644 --- a/edk2toolext/environment/version_aggregator.py +++ b/edk2toolext/environment/version_aggregator.py @@ -32,7 +32,6 @@ def ReportVersion(self, key, value, versionType, path=None): old_version = self._Versions[key] if old_version["version"] == value and old_version["path"] == path: self._logger.info(f"version_aggregator: {key} re-registered at {path}") - pass else: error = "version_aggregator: {0} key registered with a different value\n\t" \ "Old:{1}@{3}\n\tNew:{2}@{4}\n".format( diff --git a/edk2toolext/omnicache.py b/edk2toolext/omnicache.py index c456e158..a059cef5 100644 --- a/edk2toolext/omnicache.py +++ b/edk2toolext/omnicache.py @@ -160,7 +160,6 @@ def InitOmnicache(path): def AddEntry(config, name, url, tags=False): logging.info("Adding remote ({0} : {1}) to Omnicache".format(name, url)) - param = "remote add {0} {1}".format(name, url) if config.Contains(name): logging.info("Updating remote ({0} : {1}) in Omnicache".format(name, url)) diff --git a/edk2toolext/tests/test_web_dependency.py b/edk2toolext/tests/test_web_dependency.py index 1148f500..7f8e55bf 100644 --- a/edk2toolext/tests/test_web_dependency.py +++ b/edk2toolext/tests/test_web_dependency.py @@ -211,24 +211,6 @@ def test_sha256_lowercase_single_file(self): if not os.path.isfile(file_path): self.fail("The downloaded file isn't there") - # Test that get_internal_path_root works the way we expect with a flat directory structure. - # test_dir\inner_dir - test_dir\inner_dir should be the root. - def test_get_internal_path_root_flat(self): - outer_dir = test_dir - inner_dir_name = "inner_dir" - inner_dir_path = os.path.join(outer_dir, inner_dir_name) - self.assertEqual(WebDependency.get_internal_path_root(outer_dir, inner_dir_name), inner_dir_path) - - # Test that get_internal_path_root works the way we expect with a flat directory structure - # test_dir\first_dir\second_dir - test_dir\first_dir should be the root - def test_get_internal_path_root_with_subfolders(self): - outer_dir = test_dir - first_level_dir_name = "first_dir" - second_level_dir_name = "second_dir" - inner_dir_path = os.path.join(outer_dir, first_level_dir_name) - inner_second_dir_path = os.path.join(first_level_dir_name, second_level_dir_name) - self.assertEqual(WebDependency.get_internal_path_root(outer_dir, inner_second_dir_path), inner_dir_path) - # Test that a single file zipped is able to be processed by unpack. def test_unpack_zip_file(self): compressed_file_path = os.path.join(test_dir, "bad_ext_dep_zip.zip") diff --git a/setup.py b/setup.py index 82bb7d31..47bace3e 100644 --- a/setup.py +++ b/setup.py @@ -29,7 +29,7 @@ class PostInstallCommand(install): def run(self): try: DownloadNuget() - except: + except Exception: pass install.run(self) @@ -39,7 +39,7 @@ class PostDevCommand(develop): def run(self): try: DownloadNuget() - except: + except Exception: pass develop.run(self)