path_utilities: report invalid paths correctly (#364)

on Edk2Path instantiation (and error_on_invalid_pp=True), an error would
be logged for each invalid path, but the actual exception raised would
report the last path checked, which could be a valid path.

This commit updates the exception to report all invalid pps.

edk2toollib/uefi/edk2/path_utilities.py

@@ -85,20 +85,19 @@ def __init__(self, ws: os.PathLike, package_path_list: Iterable[os.PathLike],
                     # assume current working dir relative.  Will catch invalid dir when checking whole list
                     candidate_package_path_list.append(Path(os.getcwd(), a))
 
-        error = False
+        invalid_pp = []
         for a in candidate_package_path_list[:]:
             if not a.is_dir():
                 self.logger.log(logging.ERROR if error_on_invalid_pp else
                                 logging.WARNING,
                                 f"Invalid package path entry {a.resolve()}")
                 candidate_package_path_list.remove(a)
-                error = True
+                invalid_pp.append(str(a.resolve()))
 
         self.PackagePathList = [str(p) for p in candidate_package_path_list]
 
-        if error and error_on_invalid_pp:
-            raise NotADirectoryError(errno.ENOENT, os.strerror(errno.ENOENT),
-                                     a.resolve())
+        if invalid_pp and error_on_invalid_pp:
+            raise NotADirectoryError(errno.ENOENT, os.strerror(errno.ENOENT), invalid_pp)
 
         #
         # Nested package check - ensure packages do not exist in a linear

tests.unit/test_path_utilities.py

@@ -7,25 +7,26 @@
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 ##
 
-import unittest
 import os
+import shutil
 import sys
 import tempfile
-import shutil
-from edk2toollib.uefi.edk2.path_utilities import Edk2Path
+import unittest
 from pathlib import Path, PurePath
 
+from edk2toollib.uefi.edk2.path_utilities import Edk2Path
+
 
 class PathUtilitiesTest(unittest.TestCase):
 
     def _make_file_helper(self, parentpath: str, filename: str) -> None:
-        ''' simple internal helper to make a file and write basic content'''
+        '''Simple internal helper to make a file and write basic content.'''
         with open(os.path.join(parentpath, filename), "w") as f:
             f.write("Unit test content")
 
     def _make_edk2_module_helper(self, parentpath: str, modname: str, extension_case_lower: bool = True) -> str:
-        ''' simple internal helper to make a folder and inf like edk2
-        return the path to the module folder
+        '''Simple internal helper to make a folder and inf like edk2
+        return the path to the module folder.
         '''
         modfolder = os.path.join(parentpath, modname)
         os.makedirs(modfolder, exist_ok=True)
@@ -34,7 +35,7 @@ def _make_edk2_module_helper(self, parentpath: str, modname: str, extension_case
         return modfolder
 
     def _make_edk2_package_helper(self, path: str, packagename: str, extension_case_lower: bool = True) -> str:
-        ''' simple internal helper to make an ed2 package as follows:
+        '''Simple internal helper to make an ed2 package as follows:
 
         path/
           packagename/
@@ -60,43 +61,44 @@ def _make_edk2_package_helper(self, path: str, packagename: str, extension_case_
         return pkgfolder
 
     def setUp(self):
-        ''' unittest fixture run before each test.
-        Create a tmpdir and set the current working dir there'''
+        '''Unittest fixture run before each test.
+        Create a tmpdir and set the current working dir there.
+        '''
         self.tmp = tempfile.mkdtemp()
         self.precwd = os.getcwd()
         os.chdir(self.tmp)  # move to tempfile root
 
     def tearDown(self):
-        ''' unittest fixture run after each test.
-        Delete the tempdir and set the current working dir back'''
+        '''Unittest fixture run after each test.
+        Delete the tempdir and set the current working dir back.
+        '''
         os.chdir(self.precwd)
         if os.path.isdir(self.tmp):
             shutil.rmtree(self.tmp)
 
     # TESTS
 
     def test_basic_init_ws_abs(self):
-        ''' test edk2path with valid absolute path to workspace'''
+        '''Test edk2path with valid absolute path to workspace.'''
         pathobj = Edk2Path(self.tmp, [])
         self.assertEqual(pathobj.WorkspacePath, self.tmp)
 
     def test_basic_init_ws_cwd(self):
-        ''' test edk2path with a relative path to workspace'''
+        '''Test edk2path with a relative path to workspace.'''
         relpath = "testrootfolder"
         fullpath = os.path.join(self.tmp, relpath)
         os.mkdir(fullpath)
         pathobj = Edk2Path(relpath, [])
         self.assertEqual(pathobj.WorkspacePath, fullpath)
 
     def test_nonexistant_ws(self):
-        ''' test edk2path with invalid workspace'''
-
+        '''Test edk2path with invalid workspace.'''
         invalid_ws = os.path.join(self.tmp, "invalidpath")
         with self.assertRaises(Exception):
             Edk2Path(invalid_ws, [])
 
     def test_nonexistant_abs(self):
-        ''' test edk2path with valid ws but invalid pp'''
+        '''Test edk2path with valid ws but invalid pp.'''
         pp = "doesnot_exist"
         pp_full_1 = os.path.join(self.tmp, pp)
         # absolute path
@@ -110,12 +112,12 @@ def test_nonexistant_abs(self):
         self.assertEqual(len(pathobj.PackagePathList), 0)
 
     def test_pp_inside_workspace(self):
-        ''' test with packagespath pointing to folder nested inside workspace
+        '''Test with packagespath pointing to folder nested inside workspace
         root/                   <-- current working directory
             folder_ws/           <-- workspace root
                 folder_pp/       <-- packages path
                     pp packages here
-                ws packages here
+                ws packages here.
         '''
         ws_rel = "folder_ws"
         ws_abs = os.path.join(self.tmp, ws_rel)
@@ -140,12 +142,12 @@ def test_pp_inside_workspace(self):
         self.assertEqual(pathobj.PackagePathList[0], folder_pp1_abs)
 
     def test_pp_outside_workspace(self):
-        ''' test with packagespath pointing to folder outside of workspace
+        '''Test with packagespath pointing to folder outside of workspace
         root/                   <-- current working directory
             folder_ws/           <-- workspace root
                 ws packages here
             folder_pp/       <-- packages path
-                pp packages here
+                pp packages here.
 
         '''
         ws_rel = "folder_ws"
@@ -165,6 +167,21 @@ def test_pp_outside_workspace(self):
         self.assertEqual(pathobj.WorkspacePath, ws_abs)
         self.assertEqual(pathobj.PackagePathList[0], folder_pp1_abs)
 
+    def test_invalid_pp(self):
+        """Tests that the exception message contains all invalid paths and no valid paths."""
+        ws = Path(self.tmp, "folder_ws")
+        ws.mkdir()
+        (ws / "good_path").mkdir()
+
+        with self.assertRaises(NotADirectoryError) as context:
+            Edk2Path(ws, ["bad_pp_path", "bad_pp_path2", "good_path"], error_on_invalid_pp=True)
+        self.assertTrue('bad_pp_path' in str(context.exception))
+        self.assertTrue('bad_pp_path2' in str(context.exception))
+        self.assertTrue('good_path' not in str(context.exception))
+
+        # Make sure we don't throw an exception unless we mean to
+        Edk2Path(ws, ["bad_pp_path", "bad_pp_path2", "good_path"], error_on_invalid_pp=False)
+
     @unittest.skipUnless(sys.platform.startswith("win"), "requires Windows")
     def test_basic_init_ws_abs_different_case(self):
         inputPath = self.tmp.capitalize()
@@ -175,8 +192,8 @@ def test_basic_init_ws_abs_different_case(self):
         self.assertNotEqual(pathobj.WorkspacePath, self.tmp)
 
     def test_get_containing_package_inside_workspace(self):
-        ''' test basic usage of GetContainingPackage with packages path nested
-        inside the workspace
+        '''Test basic usage of GetContainingPackage with packages path nested
+        inside the workspace.
 
         File layout:
 
@@ -248,8 +265,8 @@ def test_get_containing_package_inside_workspace(self):
         self.assertIsNone(pathobj.GetContainingPackage(p))
 
     def test_get_containing_package_outside_workspace(self):
-        ''' test basic usage of GetContainingPackage with packages path
-        outside the workspace
+        '''Test basic usage of GetContainingPackage with packages path
+        outside the workspace.
 
         File layout:
 
@@ -315,7 +332,7 @@ def test_get_containing_package_outside_workspace(self):
 
     @unittest.skipUnless(sys.platform.startswith("win"), "requires Windows")
     def test_get_containing_package_ws_abs_different_case(self):
-        ''' test basic usage of GetContainingPackage when the workspace path has different case for
+        '''Test basic usage of GetContainingPackage when the workspace path has different case for
         the drive letter then the incoming paths. This can happen on Windows
         if os.path.realpath is used.
 
@@ -411,7 +428,7 @@ def test_get_containing_modules_with_relative_path(self):
                             module2.inf
                             X64
                                 TestFile.c
-       """
+        """
         # Make the workspace directory: folder_ws/
         ws_rel = "folder_ws"
         ws_abs = os.path.join(self.tmp, ws_rel)
@@ -451,7 +468,7 @@ def test_get_containing_modules_with_relative_path(self):
         self.assertRaises(Exception, pathobj.GetContainingModules, p)
 
     def test_get_containing_module_with_infs_in_other_temp_dirs(self):
-        ''' test that GetContainingModule does not look outside the workspace
+        '''Test that GetContainingModule does not look outside the workspace
         root for modules. To do so, a temporary .inf file is placed in the
         user's temporary directory. Such a file could already exist and
         similarly impact test results. To ensure consistent test results, this
@@ -562,8 +579,8 @@ def test_get_containing_modules_path_format(self):
         self.assertEqual(expected_module_inf, actual_module_inf_list[0])
 
     def test_get_containing_module(self):
-        ''' test basic usage of GetContainingModule with packages path nested
-        inside the workspace
+        '''Test basic usage of GetContainingModule with packages path nested
+        inside the workspace.
 
         File layout:
 
@@ -742,8 +759,8 @@ def test_get_edk2_relative_path_from_absolute_path_posix(self):
         self.assertEqual(expected_rel_from_abs_path, actual_rel_from_abs_path)
 
     def test_get_edk2_relative_path_from_absolute_path(self):
-        ''' test basic usage of GetEdk2RelativePathFromAbsolutePath with packages path nested
-        inside the workspace
+        '''Test basic usage of GetEdk2RelativePathFromAbsolutePath with packages path nested
+        inside the workspace.
 
         File layout:
 
@@ -815,7 +832,7 @@ def test_get_edk2_relative_path_from_absolute_path(self):
         self.assertEqual(pathobj.GetEdk2RelativePathFromAbsolutePath(p), f"{ws_p_name}/Module2/x64/TESTFILE.C")
 
     def test_get_relative_path_when_packages_path_list_contains_substrings(self):
-        ''' test usage of GetEdk2RelativePathFromAbsolutePath when members of PackagePathList contain
+        '''Test usage of GetEdk2RelativePathFromAbsolutePath when members of PackagePathList contain
         substrings of themselves, for example "MU" and "MU_TIANO"
         File layout:
          root/                    <-- current working directory (self.tmp)
@@ -833,7 +850,7 @@ def test_get_relative_path_when_packages_path_list_contains_substrings(self):
                         module2/
                             module2.INF
                             X64/
-                                TestFile2.c
+                                TestFile2.c.
         '''
         ws_rel = "folder_ws"
         ws_abs = os.path.join(self.tmp, ws_rel)
@@ -853,7 +870,7 @@ def test_get_relative_path_when_packages_path_list_contains_substrings(self):
         self.assertEqual(pathobj.GetEdk2RelativePathFromAbsolutePath(p), f"{ws_p_name}/module2/X64/TestFile2.c")
 
     def test_get_relative_path_when_package_path_inside_package(self):
-        ''' test a package_path directory inside the subfolders of a package.
+        '''Test a package_path directory inside the subfolders of a package.
             Should not raise an exception.
 
         File layout:
@@ -890,7 +907,7 @@ def test_get_relative_path_when_package_path_inside_package(self):
         self.assertEqual(pathobj.GetEdk2RelativePathFromAbsolutePath(p), f"{pp1_name}/module2/X64/TestFile.c")
 
     def test_get_relative_path_with_nested_packages(self):
-        ''' test a two package paths that contain nested packages.
+        '''Test a two package paths that contain nested packages.
             Should raise an exception due to nested packages.
 
         File layout:
@@ -965,7 +982,7 @@ def test_get_relative_path_with_nested_packages(self):
         os.environ.pop("PYTOOL_IGNORE_KNOWN_BAD_NESTED_PACKAGES")
 
     def test_get_relative_path_when_folder_is_next_to_package(self):
-        ''' test usage of GetEdk2RelativePathFromAbsolutePath when a folder containing a package is in the same
+        '''Test usage of GetEdk2RelativePathFromAbsolutePath when a folder containing a package is in the same
         directory as a different package. This test ensures the correct value is returned regardless the order of
         the package paths.
         file layout:
@@ -974,7 +991,7 @@ def test_get_relative_path_when_folder_is_next_to_package(self):
                 folder_pp1/        <-- A Package Path
                     PPTestPkg1     <-- A Package
                     folder_pp2/    <-- A Package Path
-                        PPTestPkg2 <-- A Package
+                        PPTestPkg2 <-- A Package.
         '''
         folder_ws_rel = "folder_ws"
         folder_ws_abs = os.path.join(self.tmp, folder_ws_rel)
@@ -1005,8 +1022,8 @@ def test_get_relative_path_when_folder_is_next_to_package(self):
         self.assertEqual(pathobj.GetEdk2RelativePathFromAbsolutePath(p2), f'{pp2_name}/module2/X64/TestFile2.c')
 
     def test_get_relative_path_when_path_does_not_exist(self):
-        ''' test basic usage of GetEdk2RelativePathFromAbsolutePath with packages path nested
-        inside the workspace, but the absolute path is not a real path
+        '''Test basic usage of GetEdk2RelativePathFromAbsolutePath with packages path nested
+        inside the workspace, but the absolute path is not a real path.
 
         File layout:
 
@@ -1067,7 +1084,7 @@ def test_get_relative_path_when_path_does_not_exist(self):
         self.assertIsNone(pathobj.GetEdk2RelativePathFromAbsolutePath(p))
 
     def test_get_relative_path_when_package_is_not_directly_inside_packages_path(self):
-        ''' test basic usage of GetEdk2RelativePathFromAbsolutePath when the
+        '''Test basic usage of GetEdk2RelativePathFromAbsolutePath when the
         package is not a direct subfolder of a packagespath, but atleast one
         folder away.
 
@@ -1101,8 +1118,8 @@ def test_get_relative_path_when_package_is_not_directly_inside_packages_path(sel
                          f"{folder_extra_rel}/{ws_p_name}/{ws_p_name}.dec")
 
     def test_get_absolute_path_on_this_system_from_edk2_relative_path(self):
-        ''' test basic usage of GetAbsolutePathOnThisSystemFromEdk2RelativePath with packages path nested
-        inside the workspace
+        '''Test basic usage of GetAbsolutePathOnThisSystemFromEdk2RelativePath with packages path nested
+        inside the workspace.
 
         File layout:
 
@@ -1156,7 +1173,7 @@ def test_get_absolute_path_on_this_system_from_edk2_relative_path(self):
         self.assertIsNone(pathobj.GetAbsolutePathOnThisSystemFromEdk2RelativePath(None))
 
     def test_get_absolute_path_then_relative_path_when_path_contains_repeated_packagepath_name(self):
-        ''' Test the back and forth between GetAbsolutePath and GetRelativeFromAbsolute when the
+        '''Test the back and forth between GetAbsolutePath and GetRelativeFromAbsolute when the
         path structure has multiple instances of a package path
         File layout:
          root/                  <-- current working directory (self.tmp)
@@ -1169,7 +1186,7 @@ def test_get_absolute_path_then_relative_path_when_path_contains_repeated_packag
                         module2/
                             module2.INF
                             x64/
-                               TestFile.c
+                               TestFile.c.
         '''
         ws_rel = "PlatformClientPkg"
         ws_abs = os.path.join(self.tmp, ws_rel)