Re: [edk2-staging/EdkRepo] [PATCH 5/7] EdkRepo: Add ability to find projects across all manifest repositories

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Hi Erik,

Sure, sounds good.

Thanks,
Nate

On 4/30/20, 2:40 PM, "Bjorge, Erik C" <erik.c.bjorge@intel.com> wrote:

    I agree with your comment about moving magic strings to a common location.  I would suggest that we put it someplace other than humble.py.  I think we should add a new file for configuration settings like this.  I think something like tool_config.py would be good.

    Thanks,
    -Erik

    -----Original Message-----
    From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> 
    Sent: Thursday, April 30, 2020 2:29 PM
    To: Desimone, Ashley E <ashley.e.desimone@intel.com>; devel@edk2.groups.io
    Cc: Pandya, Puja <puja.pandya@intel.com>; Bjorge, Erik C <erik.c.bjorge@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
    Subject: RE: [edk2-staging/EdkRepo] [PATCH 5/7] EdkRepo: Add ability to find projects across all manifest repositories

    Hi Ashley,

    Please see comments inline.

    Thanks,
    Nate

    > -----Original Message-----
    > From: Desimone, Ashley E <ashley.e.desimone@intel.com>
    > Sent: Tuesday, April 28, 2020 2:57 PM
    > To: devel@edk2.groups.io
    > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, 
    > Puja <puja.pandya@intel.com>; Bjorge, Erik C 
    > <erik.c.bjorge@intel.com>; Bret Barkelew 
    > <Bret.Barkelew@microsoft.com>; Agyeman, Prince 
    > <prince.agyeman@intel.com>
    > Subject: [edk2-staging/EdkRepo] [PATCH 5/7] EdkRepo: Add ability to 
    > find projects across all manifest repositories
    > 
    > Add find_project_in_all_indicies() to search for and return a tuple 
    > (source repo, source config, path to manifest) if a matching project is found.
    > 
    > Add find_project_in_single_index() to find the path to a project 
    > within a single manifest repositories index file and return a whether 
    > the specified project was found and if so its path within the manifest repository.
    > 
    > Signed-off-by: Ashley E Desimone <ashley.e.desimone@intel.com>
    > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
    > Cc: Puja Pandya <puja.pandya@intel.com>
    > Cc: Erik Bjorge <erik.c.bjorge@intel.com>
    > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
    > Cc: Prince Agyeman <prince.agyeman@intel.com>
    > ---
    >  .../manifest_repos_maintenance.py                  | 70
    > +++++++++++++++++++++-
    >  .../workspace_maintenance/workspace_maintenance.py | 17 +++++-
    >  2 files changed, 84 insertions(+), 3 deletions(-)
    > 
    > diff --git
    > a/edkrepo/common/workspace_maintenance/manifest_repos_maintenanc
    > e.py
    > b/edkrepo/common/workspace_maintenance/manifest_repos_maintenanc
    > e.py
    > index 4bded46..9b441ac 100644
    > ---
    > a/edkrepo/common/workspace_maintenance/manifest_repos_maintenanc
    > e.py
    > +++
    > b/edkrepo/common/workspace_maintenance/manifest_repos_maintenanc
    > e.py
    > @@ -15,11 +15,12 @@ import git
    >  from git import Repo
    > 
    >  import edkrepo.config.config_factory as cfg -from 
    > edkrepo.common.edkrepo_exception import 
    > EdkrepoUncommitedChangesException
    > +from edkrepo.common.edkrepo_exception import 
    > +EdkrepoUncommitedChangesException,
    > EdkrepoInvalidParametersException
    >  from edkrepo.common.progress_handler import GitProgressHandler import 
    > edkrepo.common.workspace_maintenance.humble.manifest_repos_maint
    > enance_humble as humble  from
    > edkrepo.common.workspace_maintenance.workspace_maintenance import 
    > generate_name_for_obsolete_backup
    > -
    > +from edkrepo.common.workspace_maintenance.workspace_maintenance
    > import
    > +case_insensitive_single_match from
    > edkrepo_manifest_parser.edk_manifest
    > +import CiIndexXml, ManifestXml
    > 
    >  def pull_single_manifest_repo(url, branch, local_path, reset_hard=False):
    >      '''
    > @@ -135,3 +136,68 @@ def list_available_man_repos(edkrepo_cfg,
    > edkrepo_user_cfg):
    >      return cfg_man_repos, user_cfg_man_repos, conflicts
    > 
    > 
    > +def find_project_in_single_index (project, index_file, manifest_dir):
    > +    '''
    > +    Finds a project in a single global manifest repositories index file. If found
    > +    returns (True, path to file) if not returns (False, None)
    > +    '''
    > +    global_manifest_path = None
    > +    try:
    > +        proj_name = case_insensitive_single_match(project,
    > index_file.project_list)
    > +    except:
    > +        proj_name = None
    > +    if proj_name:
    > +        ci_index_xml_rel_path =
    > os.path.normpath(index_file.get_project_xml(proj_name))
    > +        global_manifest_path = os.path.join(manifest_dir,
    > ci_index_xml_rel_path)
    > +        return True, global_manifest_path
    > +    else:
    > +        return False, global_manifest_path
    > +
    > +
    > +def find_project_in_all_indices (project, edkrepo_cfg, 
    > +edkrepo_user_cfg,
    > except_msg_man_repo, except_msg_not_found, man_repo=None):
    > +    '''
    > +    Finds the project in all manifest repositories listed in the edkrepo.efg and
    > +    edkrepo_user.cfg. If a project with the same name is found uses
    > man_repo to select
    > +    the correct entry
    > +    '''
    > +    cfg_man_repos, user_cfg_man_repos, conflicts =
    > list_available_man_repos(edkrepo_cfg, edkrepo_user_cfg)
    > +    projects = {}
    > +    for repo in cfg_man_repos:
    > +        manifest_dir = edkrepo_cfg.manifest_repo_abs_path(repo)
    > +        index_file = CiIndexXml(os.path.join(manifest_dir, 
    > + 'CiIndex.xml'))

    The magic string 'CiIndex.xml' is repeated several times. It would be good to have something like humble.py that stores the magic string, so that if we ever change it that can be done quickly.

    > +        found, man_path = find_project_in_single_index(project, 
    > + index_file,
    > manifest_dir)
    > +        if found:
    > +            projects[repo] = ('edkrepo_cfg', man_path)
    > +    for repo in user_cfg_man_repos:
    > +        manifest_dir = edkrepo_user_cfg.manifest_repo_abs_path(repo)
    > +        index_file = CiIndexXml(os.path.join(manifest_dir, 'CiIndex.xml'))
    > +        found, man_path = find_project_in_single_index(project, 
    > + index_file,
    > manifest_dir)
    > +        if found:
    > +            projects[repo] = ('edkrepo_user_cfg', man_path)
    > +    if len(projects.keys()) == 1:
    > +        repo = list(projects.keys())[0]
    > +        return repo, projects[repo][0], projects[repo][1]
    > +    elif len(projects.keys()) > 1 and man_repo:
    > +        try:
    > +            return man_repo, projects[man_repo][0], projects[man_repo][1]
    > +        except KeyError:
    > +            raise EdkrepoInvalidParametersException(except_msg_man_repo)
    > +    elif os.path.isabs(project):
    > +        return None, None, project
    > +    elif os.path.isfile(os.path.join(os.getcwd(), project)):
    > +        return None, None, os.path.join(os.getcwd(), project)
    > +    elif not os.path.dirname(project):
    > +        for repo in cfg_man_repos:
    > +            if (man_repo and (repo == man_repo)) or not man_repo:
    > +                for dirpath, dirname, filenames in
    > os.walk(edkrepo_cfg.manifest_repo_abs_path(repo)):
    > +                    if project in filenames:
    > +                        return repo, 'edkrepo_cfg', os.path.join(dirpath, project)
    > +        for repo in user_cfg_man_repos:
    > +            if (man_repo and (repo == man_repo)) or not man_repo:
    > +                for dirpath, dirname, filenames in
    > os.walk(edkrepo_user_cfg.manifest_repo_abs_path(repo)):
    > +                    if project in filenames:
    > +                        return repo, 'edkrepo_user_cfg', 
    > +os.path.join(dirpath, project)

    If man_repo is None, and the project name exists in more than 1 manifest repository, then you will reach the end of this function and return None. It seems like that should be a raised exception instead, telling the user that they need to specific a specific manifest repository to disambiguate.

    > +
    > +
    > +
    > +
    > diff --git
    > a/edkrepo/common/workspace_maintenance/workspace_maintenance.py
    > b/edkrepo/common/workspace_maintenance/workspace_maintenance.py
    > index 6e20d43..ba62f6d 100644
    > ---
    > a/edkrepo/common/workspace_maintenance/workspace_maintenance.py
    > +++
    > b/edkrepo/common/workspace_maintenance/workspace_maintenance.py
    > @@ -10,6 +10,10 @@
    >  ''' Contains shared workspace maintenance functions. '''
    > 
    >  import os
    > +import unicodedata
    > +
    > +from edkrepo.common.edkrepo_exception import 
    > +EdkrepoFoundMultipleException, EdkrepoNotFoundException from 
    > +edkrepo.common.humble import GEN_A_NOT_IN_B,
    > GEN_FOUND_MULT_A_IN_B
    > 
    >  def generate_name_for_obsolete_backup(absolute_path):
    >      if not os.path.exists(absolute_path):
    > @@ -27,4 +31,15 @@ def
    > generate_name_for_obsolete_backup(absolute_path):
    >          if not os.path.exists(os.path.join(dir_name, unique_name)):
    >              unique_name_found = True
    >          index += 1
    > -    return unique_name
    > \ No newline at end of file
    > +    return unique_name
    > +
    > +def case_insensitive_equal(str1, str2):
    > +    return unicodedata.normalize("NFKD", str1.casefold()) == 
    > +unicodedata.normalize("NFKD", str2.casefold())
    > +
    > +def case_insensitive_single_match(str1, str_list):
    > +    matches = [x for x in str_list if case_insensitive_equal(str1, x)]
    > +    if len(matches) == 0:
    > +        raise EdkrepoNotFoundException(GEN_A_NOT_IN_B.format(str1,
    > str_list))
    > +    elif len(matches) > 1:
    > +        raise
    > EdkrepoFoundMultipleException(GEN_FOUND_MULT_A_IN_B.format(str1,
    > str_list))
    > +    return matches[0]
    > --
    > 2.16.2.windows.1