public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-staging/EdkRepo] [PATCH v1 0/2] Enabling selective submodule initialization
@ 2020-05-17  1:29 Bjorge, Erik C
  2020-05-17  1:29 ` [edk2-staging/EdkRepo] [PATCH v1 1/2] EdkRepo: Adding selective submodule init script Bjorge, Erik C
  2020-05-17  1:29 ` [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code Bjorge, Erik C
  0 siblings, 2 replies; 8+ messages in thread
From: Bjorge, Erik C @ 2020-05-17  1:29 UTC (permalink / raw)
  To: devel
  Cc: Ashley E Desimone, Nate DeSimone, Puja Pandya, Bret Barkelew,
	Prince Agyeman

Enabling the ability to select the submodules to be initialized and maintained via the manifest file.

project_utils.submodule contains the submodule logic and also functions as a command line script

Signed-off-by: Erik Bjorge <erik.c.bjorge@intel.com>
Cc: Ashley E Desimone <ashley.e.desimone@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Puja Pandya <puja.pandya@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Prince Agyeman <prince.agyeman@intel.com>
Cc: Erik Bjorge <erik.c.bjorge@intel.com>

Erik Bjorge (2):
  EdkRepo: Adding selective submodule init script
  EdkRepo: Update commands to use new submodule code

 edkrepo/commands/checkout_pin_command.py  |   6 +
 edkrepo/commands/clone_command.py         |  20 +-
 edkrepo/commands/sync_command.py          |  23 +-
 edkrepo/common/common_repo_functions.py   |  43 +--
 project_utils/arguments/__init__.py       |   8 +
 project_utils/arguments/submodule_args.py |  24 ++
 project_utils/project_utils_strings.py    |  24 ++
 project_utils/submodule.py                | 389 ++++++++++++++++++++++
 setup.py                                  |  50 +--
 9 files changed, 514 insertions(+), 73 deletions(-)
 create mode 100644 project_utils/arguments/__init__.py
 create mode 100644 project_utils/arguments/submodule_args.py
 create mode 100644 project_utils/project_utils_strings.py
 create mode 100644 project_utils/submodule.py

--
2.21.0.windows.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [edk2-staging/EdkRepo] [PATCH v1 1/2] EdkRepo: Adding selective submodule init script
  2020-05-17  1:29 [edk2-staging/EdkRepo] [PATCH v1 0/2] Enabling selective submodule initialization Bjorge, Erik C
@ 2020-05-17  1:29 ` Bjorge, Erik C
  2020-05-19  2:51   ` Ashley E Desimone
  2020-05-17  1:29 ` [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code Bjorge, Erik C
  1 sibling, 1 reply; 8+ messages in thread
From: Bjorge, Erik C @ 2020-05-17  1:29 UTC (permalink / raw)
  To: devel
  Cc: Ashley E Desimone, Nate DeSimone, Puja Pandya, Bret Barkelew,
	Prince Agyeman

Adds selective submodule support functions and command line scripting
support.  The existing support will be ported over to use this
functionality.

Signed-off-by: Erik Bjorge <erik.c.bjorge@intel.com>
Cc: Ashley E Desimone <ashley.e.desimone@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Puja Pandya <puja.pandya@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Prince Agyeman <prince.agyeman@intel.com>
Cc: Erik Bjorge <erik.c.bjorge@intel.com>
---
 project_utils/arguments/__init__.py       |   8 +
 project_utils/arguments/submodule_args.py |  24 ++
 project_utils/project_utils_strings.py    |  24 ++
 project_utils/submodule.py                | 389 ++++++++++++++++++++++
 setup.py                                  |  50 +--
 5 files changed, 470 insertions(+), 25 deletions(-)
 create mode 100644 project_utils/arguments/__init__.py
 create mode 100644 project_utils/arguments/submodule_args.py
 create mode 100644 project_utils/project_utils_strings.py
 create mode 100644 project_utils/submodule.py

diff --git a/project_utils/arguments/__init__.py b/project_utils/arguments/__init__.py
new file mode 100644
index 0000000..0486261
--- /dev/null
+++ b/project_utils/arguments/__init__.py
@@ -0,0 +1,8 @@
+#!/usr/bin/env python3
+#
+## @file
+# __init__.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
diff --git a/project_utils/arguments/submodule_args.py b/project_utils/arguments/submodule_args.py
new file mode 100644
index 0000000..804ce98
--- /dev/null
+++ b/project_utils/arguments/submodule_args.py
@@ -0,0 +1,24 @@
+#!/usr/bin/env python3
+#
+## @file
+# submodule.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+"""
+Strings for command line arguments.
+"""
+
+SUBMOD_MANIFEST_HELP = 'The current manifest file.'
+SUBMOD_COMBO_HELP = 'The current branch combination in use. If a combo is not specified, the current ' \
+                    'combo will be used.'
+SUBMOD_NEW_MANIFEST_HELP = 'The new manifest file.  Used to test manifest upgrade paths.'
+SUBMOD_NEW_COMBO_HELP = 'The new branch combination to use.  Used to test switching combos.'
+SUBMOD_DEINIT_HELP = 'Performs submodule deinitialization before initializing submodules.'
+SUBMOD_INIT_FULL_HELP = 'Initialize all submodules (recursive).'
+SUBMOD_DEINIT_FULL_HELP = 'Deinitialize all submodules.'
+SUBMOD_WORKSPACE_HELP = 'The project workspace root.  If not specified the current working directory will ' \
+                        'be used.'
+SUBMOD_VERBOSE_HELP = 'Enables verbose messaging.'
diff --git a/project_utils/project_utils_strings.py b/project_utils/project_utils_strings.py
new file mode 100644
index 0000000..33c22d2
--- /dev/null
+++ b/project_utils/project_utils_strings.py
@@ -0,0 +1,24 @@
+#!/usr/bin/env python3
+#
+## @file
+# humble.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+"""
+Contains strings used by the project_utils modules.
+"""
+# Status update messages
+SUBMOD_DEINIT = 'Deinitializing submodules'
+SUBMOD_DEINIT_FULL = 'Deinitializing all submodules'
+SUBMOD_INIT_UPDATE = 'Initializing/Updating submodules'
+SUBMOD_INIT_FULL = 'Initializing/Updating all submodules (recursive)'
+
+# Verbose messages
+SUBMOD_INIT_PATH = 'Submodule init: {}'
+SUBMOD_DEINIT_PATH = 'Submodule deinit: {}'
+SUBMOD_SYNC_PATH = 'Submodule sync: {}'
+SUBMOD_UPDATE_PATH = 'Submodule update: {}'
+SUBMOD_EXCEPTION = '- Exception: {}'
diff --git a/project_utils/submodule.py b/project_utils/submodule.py
new file mode 100644
index 0000000..6534f2c
--- /dev/null
+++ b/project_utils/submodule.py
@@ -0,0 +1,389 @@
+#!/usr/bin/env python3
+#
+## @file
+# submodule.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+import argparse
+import git
+import os
+import sys
+import traceback
+
+from edkrepo_manifest_parser.edk_manifest import ManifestXml
+import project_utils.project_utils_strings as strings
+import project_utils.arguments.submodule_args as arguments
+
+
+def _init(repo, submodules=None, verbose=False):
+    """
+    Performs submodule initialization.  Note that if no selective submodule initialization
+    is enabled the init will be handled in _update.
+
+    repo       - GitPython Repo object
+    submodules - The list of selective submodule init objects
+    verbose    - Enable verbose messages
+    """
+    # Only handle selective initialization with this method.  For fully recursive init
+    # it is cleaner to do it as part of the update process.
+    if submodules is not None:
+        for sub in submodules:
+            if verbose:
+                print(strings.SUBMOD_INIT_PATH.format(sub.path))
+            output_data = repo.git.execute(['git', 'submodule', 'init', '--', sub.path],
+                                           with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+    return
+
+
+def _deinit(repo, submodules=None, verbose=False):
+    """
+    Performs deinitialization of submodules.
+
+    repo       - GitPython Repo object
+    submodules - The list of selective submodule init objects
+    verbose    - Enable verbose messages
+    """
+    if submodules is None:
+        output_data = repo.git.execute(['git', 'submodule', 'deinit', '--all'],
+                                       with_extended_output=True, with_stdout=True)
+        _display_git_outupt(output_data, verbose)
+    else:
+        for sub in submodules:
+            if verbose:
+                print(strings.SUBMOD_DEINIT_PATH.format(sub.path))
+            output_data = repo.git.execute(['git', 'submodule', 'deinit', '--', sub.path],
+                                           with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+    return
+
+
+def _update(repo, submodules=None, verbose=False, recursive=False):
+    """
+    Performs the update of submodules.  This includes the sync and update operations.
+
+    repo       - GitPython Repo object
+    submodules - The list of selective submodule init objects
+    verbose    - Enable verbose messages
+    recursive  - If submodules is None then use this parameter to determine if initialization
+                 should be recursive
+    """
+    # Now perform the update of the submodules.  For a fully recursive submodule init
+    # this code will update and initialize at the same time.
+    if submodules is None:
+        cmd = ['git', 'submodule', 'sync']
+        if recursive:
+            cmd.append('--recursive')
+        output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+        _display_git_outupt(output_data, verbose)
+        cmd = ['git', 'submodule', 'update', '--init']
+        if recursive:
+            cmd.append('--recursive')
+        output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+        _display_git_outupt(output_data, verbose)
+    else:
+        for sub in submodules:
+            if verbose:
+                print(strings.SUBMOD_SYNC_PATH.format(sub.path))
+            cmd = ['git', 'submodule', 'sync']
+            if sub.recursive:
+                cmd.append('--recursive')
+            cmd.extend(['--', sub.path])
+            output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+            if verbose:
+                print(strings.SUBMOD_UPDATE_PATH.format(sub.path))
+            cmd = ['git', 'submodule', 'update', '--init']
+            if sub.recursive:
+                cmd.append('--recursive')
+            cmd.extend(['--', sub.path])
+            output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+    return
+
+
+def _display_git_outupt(output_data, verbose=False):
+    """
+    Displays output from GitPython git commands
+
+    output_data - Output from the git.execute method
+    verbose     - Enable verbose messages
+    """
+    if verbose and output_data[0]:
+        print(output_data[0])
+    if output_data[1]:
+        print(output_data[1])
+    if verbose and output_data[2]:
+        print(output_data[2])
+
+
+def _compute_change(current_subs, new_subs):
+    """
+    Determines the list of submodules that have been removed.  Also needs to
+    determine if any submodules need to have recursive init disabled
+
+    current_subs - List of selective submodule init entries for the current combo
+    new_subs     - List of selective submodule init entries for the new combo
+    """
+    # Create data objects for determining what submodules need to be deinitialized
+    tmp_current = {x.path: x for x in current_subs}
+    tmp_new = {x.path: x for x in new_subs}
+
+    # Initial deinitialization list
+    deinit_paths = list(set(tmp_current).difference(set(tmp_new)))
+
+    # Check for change in recursive initialization for the specific submodules
+    for path in tmp_new:
+        if path in tmp_new and path in tmp_current:
+            if tmp_current[path].recursive and not tmp_new[path].recursive:
+                deinit_paths.append(path)
+
+    # Create the final list of submodules that need to be deinitialized
+    return [x for x in current_subs if x.path in deinit_paths]
+
+
+def _get_submodule_enable(manifest, remote_name, combo):
+    """
+    Determines if submodules are enabled for the current repo and combo
+
+    manifest    - Manifest object
+    remote_name - The name of the current remote being processed
+    combo       - The current combo name being processed
+    """
+    repo_sources = manifest.get_repo_sources(combo)
+    for source in repo_sources:
+        if source.remote_name == remote_name:
+            return source.enable_submodule
+    return False
+
+
+def _get_submodule_state(remote_name, start_manifest, start_combo, end_manifest=None, end_combo=None):
+    """
+    Determines the state of submodules across manifest and combo changes.
+
+        remote_name    - The name of the current remote being processed.
+        start_manifest - Initial manifest parser object.
+        start_combo    - Initial combo.
+        end_manifest   - The manifest parser object for the project if the manifest is being updated.
+        end_combo      - The combination name at the end of the operation if being modified.
+    """
+    start_subs = start_manifest.get_submodule_init_paths(remote_name, start_combo)
+    start_subs_enabled = _get_submodule_enable(start_manifest, remote_name, start_combo)
+    if end_combo is not None:
+        if end_manifest is not None:
+            end_subs = end_manifest.get_submodule_init_paths(remote_name, end_combo)
+            end_subs_enabled = _get_submodule_enable(end_manifest, remote_name, end_combo)
+        else:
+            end_subs = start_manifest.get_submodule_init_paths(remote_name, end_combo)
+            end_subs_enabled = _get_submodule_enable(start_manifest, remote_name, end_combo)
+    else:
+        if end_manifest is not None:
+            end_subs = end_manifest.get_submodule_init_paths(remote_name, start_combo)
+            end_subs_enabled = _get_submodule_enable(end_manifest, remote_name, start_combo)
+        else:
+            end_subs = start_subs
+            end_subs_enabled = start_subs_enabled
+    return start_subs, start_subs_enabled, end_subs, end_subs_enabled
+
+
+def deinit_full(workspace, manifest, verbose=False):
+    """
+    Does full submodule deinit based on the current combo.
+
+        workspace - Path to the current workspace.
+        manifest  - The current manifest parser object.
+    """
+    print(strings.SUBMOD_DEINIT_FULL)
+    current_combo = manifest.general_config.current_combo
+    repo_sources = manifest.get_repo_sources(current_combo)
+    for source in repo_sources:
+        if _get_submodule_enable(manifest, source.remote_name, current_combo):
+            # Open the repo and process submodules
+            try:
+                repo = git.Repo(os.path.join(workspace, source.root))
+            except Exception as repo_error:
+                if args.verbose:
+                    print(strings.SUBMOD_EXCEPTION.format(repo_error))
+                continue
+            _deinit(repo, None, verbose)
+
+
+def init_full(workspace, manifest, verbose=False):
+    """
+    Does full submodule init based on the current combo.
+
+        workspace - Path to the current workspace.
+        manifest  - The current manifest parser object.
+    """
+    print(strings.SUBMOD_INIT_FULL)
+    current_combo = manifest.general_config.current_combo
+    repo_sources = manifest.get_repo_sources(current_combo)
+    for source in repo_sources:
+        if _get_submodule_enable(manifest, source.remote_name, current_combo):
+            # Open the repo and process submodules
+            try:
+                repo = git.Repo(os.path.join(workspace, source.root))
+            except Exception as repo_error:
+                if args.verbose:
+                    print(strings.SUBMOD_EXCEPTION.format(repo_error))
+                continue
+            _update(repo, None, verbose, True)
+
+
+def deinit_submodules(workspace, start_manifest, start_combo,
+                      end_manifest=None, end_combo=None,
+                      verbose=False):
+    """
+    Deinitializes the submodules for a project.
+
+        workspace      - Path to the current workspace.
+        start_manifest - The manifest parser object for the project.
+        start_combo    - The combination name at the start of the operation.
+        end_manifest   - The manifest parser object for the project if the manifest is being updated.  If the
+                         manifest file is not being updated us the value None.
+        end_combo      - The combination name at the end of the operation.  If the combo will not change
+                         use the value None.
+        verbose        - Enable verbose messages.
+    """
+    # Process each repo that may have submodules enabled
+    print(strings.SUBMOD_DEINIT)
+    repo_sources = start_manifest.get_repo_sources(start_combo)
+    for source in repo_sources:
+        # Open the repo and process submodules
+        try:
+            repo = git.Repo(os.path.join(workspace, source.root))
+        except Exception as repo_error:
+            if args.verbose:
+                print(strings.SUBMOD_EXCEPTION.format(repo_error))
+            continue
+
+        # Collect the submodule initialization data from manifest as well as if submodules
+        # should be processed for the repo.
+        start_subs, start_subs_enabled, end_subs, end_subs_enabled = _get_submodule_state(source.remote_name,
+                                                                                          start_manifest,
+                                                                                          start_combo,
+                                                                                          end_manifest,
+                                                                                          end_combo)
+        if not start_subs_enabled and not end_subs_enabled:
+            # At this point submodules are not enabled on this repo
+            continue
+
+        # Compute the list of submodules that need to be removed and added
+        deinit_list = _compute_change(start_subs, end_subs)
+
+        # Deinitialize submodules
+        if (start_subs_enabled and not end_subs_enabled) or (len(start_subs) == 0 and len(end_subs) != 0):
+            # Submodules are being disabled for the entire repo so do a
+            # full deinit on the repo.
+            _deinit(repo, None, verbose)
+        else:
+            # Do the deinit based on the list
+            _deinit(repo, deinit_list, verbose)
+
+
+def init_submodules(workspace, manifest, combo_name, verbose=False):
+    """
+    Updates the submodules for a specific repo.  Note that operations like checkout will need to use
+    the start_combo and end_combo parameter.  Operations like clone will just need to use the
+    start_combo parameter.
+
+        workspace  - Path to the current workspace.
+        manifest   - The manifest parser object for the project.
+        combo_name - The combination name at the start of the operation.
+        verbose    - Enable verbose messages.
+    """
+    # Process each repo that may have submodules enabled
+    print(strings.SUBMOD_INIT_UPDATE)
+    repo_sources = manifest.get_repo_sources(combo_name)
+    for source in repo_sources:
+        # Open the repo and process submodules
+        try:
+            repo = git.Repo(os.path.join(workspace, source.root))
+        except Exception as repo_error:
+            if args.verbose:
+                print(strings.SUBMOD_EXCEPTION.format(repo_error))
+            continue
+
+        # Collect the submodule initialization data from manifest as well as if submodules
+        # should be processed for the repo.
+        repo_subs = manifest.get_submodule_init_paths(source.remote_name, combo_name)
+        repo_subs_enabled = _get_submodule_enable(manifest, source.remote_name, combo_name)
+        if not repo_subs_enabled:
+            continue
+
+        # Initialize submodules
+        if len(repo_subs) > 0:
+            _init(repo, repo_subs, verbose)
+
+        # Perform sync/update
+        if len(repo_subs) == 0:
+            _update(repo, None, verbose)
+        else:
+            _update(repo, repo_subs, verbose)
+
+
+if __name__ == '__main__':
+    def parse_args():
+        parser = argparse.ArgumentParser()
+        parser.add_argument('manifest_file', metavar='MANIFEST', help=arguments.SUBMOD_MANIFEST_HELP)
+        parser.add_argument('--combo', default=None, help=arguments.SUBMOD_COMBO_HELP)
+        parser.add_argument('--new-manifest', default=None, help=arguments.SUBMOD_NEW_MANIFEST_HELP)
+        parser.add_argument('--new-combo', default=None, help=arguments.SUBMOD_NEW_COMBO_HELP)
+        parser.add_argument('--deinit', action='store_true', help=arguments.SUBMOD_DEINIT_HELP)
+        parser.add_argument('--init-full', action='store_true', help=arguments.SUBMOD_INIT_FULL_HELP)
+        parser.add_argument('--deinit-full', action='store_true', help=arguments.SUBMOD_DEINIT_FULL_HELP)
+        parser.add_argument('--workspace', default='.', help=arguments.SUBMOD_WORKSPACE_HELP)
+        parser.add_argument('--verbose', action='store_true', help=arguments.SUBMOD_VERBOSE_HELP)
+        return parser.parse_args()
+
+    def main(args):
+        # Optional data init
+        new_manifest = None
+        all_new_combos = None
+
+        # Extract basic manifest data
+        manifest = ManifestXml(args.manifest_file)
+        init_manifest = manifest
+        all_combos = manifest.combinations
+        all_combos.extend(manifest.archived_combinations)
+        if args.new_manifest is not None:
+            new_manifest = ManifestXml(args.new_manifest)
+            all_new_combos = new_manifest.combinations
+            all_new_combos.extend(new_manifest.archived_combinations)
+            init_manifest = new_manifest
+
+        # Determine current and new combo information
+        current_combo = manifest.general_config.current_combo
+        new_combo = args.new_combo
+        for combo in all_combos:
+            if args.combo is not None:
+                if args.combo.lower() == combo.name.lower():
+                    current_combo = combo.name
+            if args.new_combo is not None:
+                if args.new_combo.lower() == combo.name.lower():
+                    new_combo = combo.name
+        init_combo = current_combo
+        if new_combo is not None:
+            init_combo = new_combo
+
+        if args.deinit_full:
+            deinit_full(args.workspace, manifest, args.verbose)
+        elif args.init_full:
+            init_full(args.workspace, manifest, args.verbose)
+        else:
+            if args.deinit:
+                deinit_submodules(args.workspace, manifest, current_combo,
+                                  new_manifest, new_combo, args.verbose)
+            init_submodules(args.workspace, init_manifest, init_combo, args.verbose)
+
+        return 0
+
+    try:
+        args = parse_args()
+        sys.exit(main(args))
+    except Exception:
+        if args.verbose:
+            traceback.print_exc()
+        sys.exit(1)
diff --git a/setup.py b/setup.py
index 5d2f55a..a855000 100755
--- a/setup.py
+++ b/setup.py
@@ -1,27 +1,27 @@
-#!/usr/bin/env python3
-#
-## @file setup.py
-#
-# Copyright (c) 2017 - 2020, Intel Corporation. All rights reserved.<BR>
-# SPDX-License-Identifier: BSD-2-Clause-Patent
-#
-
-from setuptools import setup
-
-setup(name='edkrepo',
-      version='2.0.0',
-      description='The edkrepo tools',
-      packages=['edkrepo', 'edkrepo.commands', 'edkrepo.commands.arguments', 'edkrepo.commands.humble',
+#!/usr/bin/env python3
+#
+## @file setup.py
+#
+# Copyright (c) 2017 - 2020, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+from setuptools import setup
+
+setup(name='edkrepo',
+      version='2.0.0',
+      description='The edkrepo tools',
+      packages=['edkrepo', 'edkrepo.commands', 'edkrepo.commands.arguments', 'edkrepo.commands.humble',
                 'edkrepo.git_automation', 'edkrepo.common', 'edkrepo.common.workspace_maintenance',
                 'edkrepo.common.workspace_maintenance.humble', 'edkrepo.config', 'edkrepo.config.humble',
-                'edkrepo_manifest_parser', 'project_utils'],
-      package_data={
-         },
-      include_package_data=True,
-      entry_points={
-          'console_scripts': [
-              'edkrepo = edkrepo.edkrepo_entry_point:main',
-              'command_completion_edkrepo = edkrepo.command_completion_edkrepo:main'
-              ]
-          }
-      )
+                'edkrepo_manifest_parser', 'project_utils', 'project_utils.arguments'],
+      package_data={
+         },
+      include_package_data=True,
+      entry_points={
+          'console_scripts': [
+              'edkrepo = edkrepo.edkrepo_entry_point:main',
+              'command_completion_edkrepo = edkrepo.command_completion_edkrepo:main'
+              ]
+          }
+      )
-- 
2.21.0.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code
  2020-05-17  1:29 [edk2-staging/EdkRepo] [PATCH v1 0/2] Enabling selective submodule initialization Bjorge, Erik C
  2020-05-17  1:29 ` [edk2-staging/EdkRepo] [PATCH v1 1/2] EdkRepo: Adding selective submodule init script Bjorge, Erik C
@ 2020-05-17  1:29 ` Bjorge, Erik C
  2020-05-19  3:01   ` Ashley E Desimone
  1 sibling, 1 reply; 8+ messages in thread
From: Bjorge, Erik C @ 2020-05-17  1:29 UTC (permalink / raw)
  To: devel
  Cc: Ashley E Desimone, Nate DeSimone, Puja Pandya, Bret Barkelew,
	Prince Agyeman

Replaced use of maintain_submodules to use new common code.  This allows
for selective submodule initialization via the manifest file.

Cc: Ashley E Desimone <ashley.e.desimone@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Puja Pandya <puja.pandya@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Prince Agyeman <prince.agyeman@intel.com>
Cc: Erik Bjorge <erik.c.bjorge@intel.com>
Signed-off-by: Erik Bjorge <erik.c.bjorge@intel.com>
---
 edkrepo/commands/checkout_pin_command.py |  6 ++++
 edkrepo/commands/clone_command.py        | 20 +++++++----
 edkrepo/commands/sync_command.py         | 23 +++++++++----
 edkrepo/common/common_repo_functions.py  | 43 +++++-------------------
 4 files changed, 44 insertions(+), 48 deletions(-)

diff --git a/edkrepo/commands/checkout_pin_command.py b/edkrepo/commands/checkout_pin_command.py
index 4aaf2b5..c250c49 100644
--- a/edkrepo/commands/checkout_pin_command.py
+++ b/edkrepo/commands/checkout_pin_command.py
@@ -22,6 +22,8 @@ from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import list
 from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import find_source_manifest_repo
 from edkrepo.config.config_factory import get_workspace_path, get_workspace_manifest
 from edkrepo_manifest_parser.edk_manifest import ManifestXml
+from project_utils.submodule import deinit_submodules, init_submodules
+
 
 class CheckoutPinCommand(EdkrepoCommand):
     def __init__(self):
@@ -68,11 +70,15 @@ class CheckoutPinCommand(EdkrepoCommand):
         if sparse_enabled:
             print(SPARSE_RESET)
             reset_sparse_checkout(workspace_path, manifest_sources)
+        submodule_combo = pin.general_config.current_combo
+        deinit_submodules(workspace_path, manifest, manifest.general_config.current_combo,
+                          pin, submodule_combo, args.verbose)
         pin_repo_sources = pin.get_repo_sources(pin.general_config.current_combo)
         try:
             checkout_repos(args.verbose, args.override, pin_repo_sources, workspace_path, manifest)
             manifest.write_current_combo(humble.PIN_COMBO.format(args.pinfile))
         finally:
+            init_submodules(workspace_path, pin, submodule_combo, args.verbose)
             if sparse_enabled:
                 print(SPARSE_CHECKOUT)
                 sparse_checkout(workspace_path, pin_repo_sources, manifest)
diff --git a/edkrepo/commands/clone_command.py b/edkrepo/commands/clone_command.py
index 179aaf7..653e10b 100644
--- a/edkrepo/commands/clone_command.py
+++ b/edkrepo/commands/clone_command.py
@@ -24,6 +24,7 @@ from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import pull
 from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import list_available_manifest_repos
 from edkrepo.common.workspace_maintenance.humble.manifest_repos_maintenance_humble import PROJ_NOT_IN_REPO, SOURCE_MANIFEST_REPO_NOT_FOUND
 from edkrepo_manifest_parser.edk_manifest import CiIndexXml, ManifestXml
+from project_utils.submodule import init_submodules
 
 
 class CloneCommand(EdkrepoCommand):
@@ -104,7 +105,6 @@ class CloneCommand(EdkrepoCommand):
         manifest = ManifestXml(local_manifest_path)
 
         # Process the combination name and make sure it can be found in the manifest
-        combo_name = None
         if args.Combination is not None:
             try:
                 combo_name = case_insensitive_single_match(args.Combination, combinations_in_manifest(manifest))
@@ -118,13 +118,15 @@ class CloneCommand(EdkrepoCommand):
             # Since pin files are subset of manifest files they do not have a "default combo" it is set to None. In this
             # case use the current_combo instead.
             combo_name = manifest.general_config.current_combo
+        else:
+            # If a combo was not specified or a pin file used the default combo should be cloned.  Also ensure that the
+            # current combo is updated to match.
+            combo_name = manifest.general_config.default_combo
+            manifest.write_current_combo(combo_name)
 
         # Get the list of repos to clone and clone them
-        manifest_config = manifest.general_config
-        if combo_name:
-            repo_sources_to_clone = manifest.get_repo_sources(combo_name)
-        else:
-            repo_sources_to_clone = manifest.get_repo_sources(manifest_config.default_combo)
+        repo_sources_to_clone = manifest.get_repo_sources(combo_name)
+
         #check that the repo sources do not contain duplicated local roots
         local_roots = [r.root for r in repo_sources_to_clone]
         for root in local_roots:
@@ -137,7 +139,11 @@ class CloneCommand(EdkrepoCommand):
         # Set up submodule alt url config settings prior to cloning any repos
         submodule_included_configs = write_included_config(manifest.remotes, manifest.submodule_alternate_remotes, local_manifest_dir)
         write_conditional_include(workspace_dir, repo_sources_to_clone, submodule_included_configs)
-        clone_repos(args, workspace_dir, repo_sources_to_clone, project_client_side_hooks, config, args.skip_submodule, manifest)
+        clone_repos(args, workspace_dir, repo_sources_to_clone, project_client_side_hooks, config, manifest)
+
+        # Init submodules
+        if not args.skip_submodule:
+            init_submodules(workspace_dir, manifest, combo_name, args.verbose)
 
         # Perform a sparse checkout if requested.
         use_sparse = args.sparse
diff --git a/edkrepo/commands/sync_command.py b/edkrepo/commands/sync_command.py
index da99397..d3c724f 100644
--- a/edkrepo/commands/sync_command.py
+++ b/edkrepo/commands/sync_command.py
@@ -38,7 +38,7 @@ from edkrepo.common.common_repo_functions import checkout_repos, check_dirty_rep
 from edkrepo.common.common_repo_functions import update_editor_config
 from edkrepo.common.common_repo_functions import update_repo_commit_template, get_latest_sha
 from edkrepo.common.common_repo_functions import has_primary_repo_remote, fetch_from_primary_repo, in_sync_with_primary
-from edkrepo.common.common_repo_functions import update_hooks, maintain_submodules, combinations_in_manifest
+from edkrepo.common.common_repo_functions import update_hooks, combinations_in_manifest
 from edkrepo.common.common_repo_functions import write_included_config, remove_included_config
 from edkrepo.common.workspace_maintenance.workspace_maintenance import generate_name_for_obsolete_backup
 from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import pull_workspace_manifest_repo
@@ -48,6 +48,7 @@ from edkrepo.common.ui_functions import init_color_console
 from edkrepo.config.config_factory import get_workspace_path, get_workspace_manifest, get_edkrepo_global_data_directory
 from edkrepo.config.config_factory import get_workspace_manifest_file
 from edkrepo_manifest_parser.edk_manifest import CiIndexXml, ManifestXml
+from project_utils.submodule import deinit_submodules, init_submodules
 
 
 class SyncCommand(EdkrepoCommand):
@@ -84,6 +85,7 @@ class SyncCommand(EdkrepoCommand):
         current_combo = initial_manifest.general_config.current_combo
         initial_sources = initial_manifest.get_repo_sources(current_combo)
         initial_hooks = initial_manifest.repo_hooks
+        initial_combo = current_combo
 
         source_global_manifest_repo = find_source_manifest_repo(initial_manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo)
         pull_workspace_manifest_repo(initial_manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo, False)
@@ -100,6 +102,7 @@ class SyncCommand(EdkrepoCommand):
         if not args.update_local_manifest:
             self.__check_for_new_manifest(args, config, initial_manifest, workspace_path, global_manifest_directory)
         check_dirty_repos(initial_manifest, workspace_path)
+
         # Determine if sparse checkout needs to be disabled for this operation
         sparse_settings = initial_manifest.sparse_settings
         sparse_enabled = sparse_checkout_enabled(workspace_path, initial_sources)
@@ -113,7 +116,7 @@ class SyncCommand(EdkrepoCommand):
             reset_sparse_checkout(workspace_path, initial_sources)
 
         # Get the latest manifest if requested
-        if args.update_local_manifest: #NOTE: hyphens in arg name replaced with underscores due to argparse
+        if args.update_local_manifest:  # NOTE: hyphens in arg name replaced with underscores due to argparse
             self.__update_local_manifest(args, config, initial_manifest, workspace_path, global_manifest_directory)
         manifest = get_workspace_manifest()
         if args.update_local_manifest:
@@ -127,6 +130,12 @@ class SyncCommand(EdkrepoCommand):
             repo_sources_to_sync = manifest.get_repo_sources(current_combo)
         manifest.write_current_combo(current_combo)
 
+        # At this point both new and old manifest files are ready so we can deinit any
+        # submodules that are removed due to a manifest update.
+        if not args.skip_submodule:
+            deinit_submodules(workspace_path, initial_manifest, initial_combo,
+                              manifest, current_combo, args.verbose)
+
         sync_error = False
         # Calculate the hooks which need to be updated, added or removed for the sync
         if args.update_local_manifest:
@@ -195,16 +204,16 @@ class SyncCommand(EdkrepoCommand):
             elif args.verbose:
                 print(NO_SYNC_DETACHED_HEAD.format(repo_to_sync.root))
 
-            if not args.skip_submodule:
-                if repo_to_sync.enable_submodule:
-                    # Perform submodule updates and url redirection
-                    maintain_submodules(repo_to_sync, repo)
             # Update commit message templates
             update_repo_commit_template(workspace_path, repo, repo_to_sync, config, source_global_manifest_repo)
 
         if sync_error:
             print(SYNC_ERROR)
 
+        # Initialize submodules
+        if not args.skip_submodule:
+            init_submodules(workspace_path, manifest, current_combo, args.verbose)
+
         # Restore sparse checkout state
         if sparse_enabled:
             print(SPARSE_CHECKOUT)
@@ -326,7 +335,7 @@ class SyncCommand(EdkrepoCommand):
                 print(path_to_source)
             if len(sources_to_remove) > 0:
                 print(SYNC_REMOVE_LIST_END_FORMATTING)
-            clone_repos(args, workspace_path, sources_to_clone, new_manifest_to_check.repo_hooks, config, args.skip_submodule, new_manifest_to_check)
+            clone_repos(args, workspace_path, sources_to_clone, new_manifest_to_check.repo_hooks, config, new_manifest_to_check)
             # Make a list of and only checkout repos that were newly cloned. Sync keeps repos on their initial active branches
             # cloning the entire combo can prevent existing repos from correctly being returned to their proper branch
             repos_to_checkout = []
diff --git a/edkrepo/common/common_repo_functions.py b/edkrepo/common/common_repo_functions.py
index 61133f1..18e545b 100644
--- a/edkrepo/common/common_repo_functions.py
+++ b/edkrepo/common/common_repo_functions.py
@@ -68,12 +68,13 @@ from edkrepo_manifest_parser.edk_manifest_validation import validate_manifestrep
 from edkrepo_manifest_parser.edk_manifest_validation import get_manifest_validation_status
 from edkrepo_manifest_parser.edk_manifest_validation import print_manifest_errors
 from edkrepo_manifest_parser.edk_manifest_validation import validate_manifestfiles
+from project_utils.submodule import deinit_submodules, init_submodules
 
 CLEAR_LINE = '\x1b[K'
 DEFAULT_REMOTE_NAME = 'origin'
 PRIMARY_REMOTE_NAME = 'primary'
 
-def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, skip_submodule, manifest):
+def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, manifest):
     for repo_to_clone in repos_to_clone:
         local_repo_path = os.path.join(workspace_dir, repo_to_clone.root)
         local_repo_url = repo_to_clone.remote_url
@@ -108,10 +109,6 @@ def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks,
         else:
             raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-        if not skip_submodule:
-            if repo_to_clone.enable_submodule:
-                maintain_submodules(repo_to_clone, repo, args.verbose)
-
         try:
             if 'source_manifest_repo' in vars(args).keys():
                 src_manifest_repo = find_source_manifest_repo(manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo)
@@ -184,32 +181,6 @@ def write_conditional_include(workspace_path, repo_sources, included_configs):
                     gitglobalconfig.add_section(section)
                     gitglobalconfig.set(section, 'path', path)
 
-def maintain_submodules(repo_sources, repo, verbose = False):
-    try:
-        output_data = repo.git.execute(['git', 'submodule', 'init'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-        output_data = repo.git.execute(['git', 'submodule', 'sync', '--recursive'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-        output_data = repo.git.execute(['git', 'submodule', 'update', '--recursive'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-    except:
-        raise EdkrepoGitException(SUBMODULE_FAILURE.format(repo_sources.remote_name))
-
 def install_hooks(hooks, local_repo_path, repo_for_install, config, global_manifest_directory):
     # Determine the which hooks are for the repo in question and which are from a URL based source or are in a global
     # manifest repo relative path
@@ -399,9 +370,6 @@ def checkout_repos(verbose, override, repos_to_checkout, workspace_path, manifes
         else:
             raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-        if repo_to_checkout.enable_submodule:
-            maintain_submodules(repo_to_checkout, repo, verbose)
-
 def validate_manifest_repo(manifest_repo, verbose=False, archived=False):
     print(VERIFY_GLOBAL)
     if archived:
@@ -514,9 +482,11 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
     # Create combo_or_sha so we have original input and do not introduce any
     # unintended behavior by messing with parameters.
     combo_or_sha = combination_or_sha
+    submodule_combo = manifest.general_config.current_combo
     try:
         # Try to handle normalize combo name to match the manifest file.
         combo_or_sha = case_insensitive_single_match(combo_or_sha, combinations_in_manifest(manifest))
+        submodule_combo = combo_or_sha
     except:
         # No match so leave it alone.  It must be a SHA1 or a bad combo name.
         pass
@@ -528,6 +498,10 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
         log=log)
     initial_repo_sources = manifest.get_repo_sources(manifest.general_config.current_combo)
 
+    # Deinit any submodules that have been removed.
+    deinit_submodules(workspace_path, manifest, manifest.general_config.current_combo,
+                      manifest, submodule_combo, verbose)
+
     # Disable sparse checkout
     current_repos = initial_repo_sources
     sparse_enabled = sparse_checkout_enabled(workspace_path, initial_repo_sources)
@@ -566,6 +540,7 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
         # Return to the initial combo, since there was an issue with cheking out the selected combo
         checkout_repos(verbose, override, initial_repo_sources, workspace_path, manifest)
     finally:
+        init_submodules(workspace_path, manifest, submodule_combo, verbose)
         if sparse_enabled or sparse_diff:
             print(SPARSE_CHECKOUT)
             sparse_checkout(workspace_path, current_repos, manifest)
-- 
2.21.0.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [edk2-staging/EdkRepo] [PATCH v1 1/2] EdkRepo: Adding selective submodule init script
  2020-05-17  1:29 ` [edk2-staging/EdkRepo] [PATCH v1 1/2] EdkRepo: Adding selective submodule init script Bjorge, Erik C
@ 2020-05-19  2:51   ` Ashley E Desimone
  2020-05-19 17:46     ` Bjorge, Erik C
  0 siblings, 1 reply; 8+ messages in thread
From: Ashley E Desimone @ 2020-05-19  2:51 UTC (permalink / raw)
  To: Bjorge, Erik C, devel@edk2.groups.io
  Cc: Desimone, Nathaniel L, Pandya, Puja, Bret Barkelew,
	Agyeman, Prince

Hi Erik,

Please see feedback inline below.

Thanks,
Ashley

-----Original Message-----
From: Bjorge, Erik C <erik.c.bjorge@intel.com> 
Sent: Saturday, May 16, 2020 6:29 PM
To: devel@edk2.groups.io
Cc: Desimone, Ashley E <ashley.e.desimone@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, Puja <puja.pandya@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
Subject: [edk2-staging/EdkRepo] [PATCH v1 1/2] EdkRepo: Adding selective submodule init script

Adds selective submodule support functions and command line scripting support.  The existing support will be ported over to use this functionality.

Signed-off-by: Erik Bjorge <erik.c.bjorge@intel.com>
Cc: Ashley E Desimone <ashley.e.desimone@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Puja Pandya <puja.pandya@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Prince Agyeman <prince.agyeman@intel.com>
Cc: Erik Bjorge <erik.c.bjorge@intel.com>
---
 project_utils/arguments/__init__.py       |   8 +
 project_utils/arguments/submodule_args.py |  24 ++
 project_utils/project_utils_strings.py    |  24 ++
 project_utils/submodule.py                | 389 ++++++++++++++++++++++
 setup.py                                  |  50 +--
 5 files changed, 470 insertions(+), 25 deletions(-)  create mode 100644 project_utils/arguments/__init__.py
 create mode 100644 project_utils/arguments/submodule_args.py
 create mode 100644 project_utils/project_utils_strings.py
 create mode 100644 project_utils/submodule.py

diff --git a/project_utils/arguments/__init__.py b/project_utils/arguments/__init__.py
new file mode 100644
index 0000000..0486261
--- /dev/null
+++ b/project_utils/arguments/__init__.py
@@ -0,0 +1,8 @@
+#!/usr/bin/env python3
+#
+## @file
+# __init__.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> # 
+SPDX-License-Identifier: BSD-2-Clause-Patent #
diff --git a/project_utils/arguments/submodule_args.py b/project_utils/arguments/submodule_args.py
new file mode 100644
index 0000000..804ce98
--- /dev/null
+++ b/project_utils/arguments/submodule_args.py
@@ -0,0 +1,24 @@
+#!/usr/bin/env python3
+#
+## @file
+# submodule.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> # 
+SPDX-License-Identifier: BSD-2-Clause-Patent #
+
+"""
+Strings for command line arguments.
+"""
+
+SUBMOD_MANIFEST_HELP = 'The current manifest file.'
+SUBMOD_COMBO_HELP = 'The current branch combination in use. If a combo is not specified, the current ' \
+                    'combo will be used.'
+SUBMOD_NEW_MANIFEST_HELP = 'The new manifest file.  Used to test manifest upgrade paths.'
+SUBMOD_NEW_COMBO_HELP = 'The new branch combination to use.  Used to test switching combos.'
+SUBMOD_DEINIT_HELP = 'Performs submodule deinitialization before initializing submodules.'
+SUBMOD_INIT_FULL_HELP = 'Initialize all submodules (recursive).'
+SUBMOD_DEINIT_FULL_HELP = 'Deinitialize all submodules.'
+SUBMOD_WORKSPACE_HELP = 'The project workspace root.  If not specified the current working directory will ' \
+                        'be used.'
+SUBMOD_VERBOSE_HELP = 'Enables verbose messaging.'
diff --git a/project_utils/project_utils_strings.py b/project_utils/project_utils_strings.py
new file mode 100644
index 0000000..33c22d2
--- /dev/null
+++ b/project_utils/project_utils_strings.py
@@ -0,0 +1,24 @@
+#!/usr/bin/env python3
+#
+## @file
+# humble.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> # 
+SPDX-License-Identifier: BSD-2-Clause-Patent #
+
+"""
+Contains strings used by the project_utils modules.
+"""
+# Status update messages
+SUBMOD_DEINIT = 'Deinitializing submodules'
+SUBMOD_DEINIT_FULL = 'Deinitializing all submodules'
+SUBMOD_INIT_UPDATE = 'Initializing/Updating submodules'
+SUBMOD_INIT_FULL = 'Initializing/Updating all submodules (recursive)'
+
+# Verbose messages
+SUBMOD_INIT_PATH = 'Submodule init: {}'
+SUBMOD_DEINIT_PATH = 'Submodule deinit: {}'
+SUBMOD_SYNC_PATH = 'Submodule sync: {}'
+SUBMOD_UPDATE_PATH = 'Submodule update: {}'
+SUBMOD_EXCEPTION = '- Exception: {}'
diff --git a/project_utils/submodule.py b/project_utils/submodule.py new file mode 100644 index 0000000..6534f2c
--- /dev/null
+++ b/project_utils/submodule.py
@@ -0,0 +1,389 @@
+#!/usr/bin/env python3
+#
+## @file
+# submodule.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> # 
+SPDX-License-Identifier: BSD-2-Clause-Patent # import argparse import 
+git import os import sys import traceback
+
+from edkrepo_manifest_parser.edk_manifest import ManifestXml import 
+project_utils.project_utils_strings as strings import 
+project_utils.arguments.submodule_args as arguments
+
+
+def _init(repo, submodules=None, verbose=False):
+    """
+    Performs submodule initialization.  Note that if no selective submodule initialization
+    is enabled the init will be handled in _update.
+
+    repo       - GitPython Repo object
+    submodules - The list of selective submodule init objects
+    verbose    - Enable verbose messages
+    """
+    # Only handle selective initialization with this method.  For fully recursive init
+    # it is cleaner to do it as part of the update process.
+    if submodules is not None:
+        for sub in submodules:
+            if verbose:
+                print(strings.SUBMOD_INIT_PATH.format(sub.path))
+            output_data = repo.git.execute(['git', 'submodule', 'init', '--', sub.path],
+                                           with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+    return
+
+
+def _deinit(repo, submodules=None, verbose=False):
+    """
+    Performs deinitialization of submodules.
+
+    repo       - GitPython Repo object
+    submodules - The list of selective submodule init objects
+    verbose    - Enable verbose messages
+    """
+    if submodules is None:
+        output_data = repo.git.execute(['git', 'submodule', 'deinit', '--all'],
+                                       with_extended_output=True, with_stdout=True)
+        _display_git_outupt(output_data, verbose)
+    else:
+        for sub in submodules:
+            if verbose:
+                print(strings.SUBMOD_DEINIT_PATH.format(sub.path))
+            output_data = repo.git.execute(['git', 'submodule', 'deinit', '--', sub.path],
+                                           with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+    return
+
+
+def _update(repo, submodules=None, verbose=False, recursive=False):
+    """
+    Performs the update of submodules.  This includes the sync and update operations.
+
+    repo       - GitPython Repo object
+    submodules - The list of selective submodule init objects
+    verbose    - Enable verbose messages
+    recursive  - If submodules is None then use this parameter to determine if initialization
+                 should be recursive
+    """
+    # Now perform the update of the submodules.  For a fully recursive submodule init
+    # this code will update and initialize at the same time.
+    if submodules is None:
+        cmd = ['git', 'submodule', 'sync']
+        if recursive:
+            cmd.append('--recursive')
+        output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+        _display_git_outupt(output_data, verbose)
+        cmd = ['git', 'submodule', 'update', '--init']
+        if recursive:
+            cmd.append('--recursive')
+        output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+        _display_git_outupt(output_data, verbose)
+    else:
+        for sub in submodules:
+            if verbose:
+                print(strings.SUBMOD_SYNC_PATH.format(sub.path))
+            cmd = ['git', 'submodule', 'sync']
+            if sub.recursive:
+                cmd.append('--recursive')
+            cmd.extend(['--', sub.path])
+            output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+            if verbose:
+                print(strings.SUBMOD_UPDATE_PATH.format(sub.path))
+            cmd = ['git', 'submodule', 'update', '--init']
+            if sub.recursive:
+                cmd.append('--recursive')
+            cmd.extend(['--', sub.path])
+            output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+    return
+

This is a common pattern used for parsing git output throughout EdkRepo I would like to see it placed somewhere where it can be consumed without importing all of the submodule content as well. 
+def _display_git_outupt(output_data, verbose=False):
+    """
+    Displays output from GitPython git commands
+
+    output_data - Output from the git.execute method
+    verbose     - Enable verbose messages
+    """
+    if verbose and output_data[0]:
+        print(output_data[0])
+    if output_data[1]:
+        print(output_data[1])
+    if verbose and output_data[2]:
+        print(output_data[2])
+
+
+def _compute_change(current_subs, new_subs):
+    """
+    Determines the list of submodules that have been removed.  Also needs to
+    determine if any submodules need to have recursive init disabled
+
+    current_subs - List of selective submodule init entries for the current combo
+    new_subs     - List of selective submodule init entries for the new combo
+    """
+    # Create data objects for determining what submodules need to be deinitialized
+    tmp_current = {x.path: x for x in current_subs}
+    tmp_new = {x.path: x for x in new_subs}
+
+    # Initial deinitialization list
+    deinit_paths = list(set(tmp_current).difference(set(tmp_new)))
+
+    # Check for change in recursive initialization for the specific submodules
+    for path in tmp_new:
+        if path in tmp_new and path in tmp_current:
+            if tmp_current[path].recursive and not tmp_new[path].recursive:
+                deinit_paths.append(path)
+
+    # Create the final list of submodules that need to be deinitialized
+    return [x for x in current_subs if x.path in deinit_paths]
+
+
+def _get_submodule_enable(manifest, remote_name, combo):
+    """
+    Determines if submodules are enabled for the current repo and combo
+
+    manifest    - Manifest object
+    remote_name - The name of the current remote being processed
+    combo       - The current combo name being processed
+    """
+    repo_sources = manifest.get_repo_sources(combo)
+    for source in repo_sources:
+        if source.remote_name == remote_name:
+            return source.enable_submodule
+    return False
+
+
+def _get_submodule_state(remote_name, start_manifest, start_combo, end_manifest=None, end_combo=None):
+    """
+    Determines the state of submodules across manifest and combo changes.
+
+        remote_name    - The name of the current remote being processed.
+        start_manifest - Initial manifest parser object.
+        start_combo    - Initial combo.
+        end_manifest   - The manifest parser object for the project if the manifest is being updated.
+        end_combo      - The combination name at the end of the operation if being modified.
+    """
+    start_subs = start_manifest.get_submodule_init_paths(remote_name, start_combo)
+    start_subs_enabled = _get_submodule_enable(start_manifest, remote_name, start_combo)
+    if end_combo is not None:
+        if end_manifest is not None:
+            end_subs = end_manifest.get_submodule_init_paths(remote_name, end_combo)
+            end_subs_enabled = _get_submodule_enable(end_manifest, remote_name, end_combo)
+        else:
+            end_subs = start_manifest.get_submodule_init_paths(remote_name, end_combo)
+            end_subs_enabled = _get_submodule_enable(start_manifest, remote_name, end_combo)
+    else:
+        if end_manifest is not None:
+            end_subs = end_manifest.get_submodule_init_paths(remote_name, start_combo)
+            end_subs_enabled = _get_submodule_enable(end_manifest, remote_name, start_combo)
+        else:
+            end_subs = start_subs
+            end_subs_enabled = start_subs_enabled
+    return start_subs, start_subs_enabled, end_subs, end_subs_enabled
+
+
+def deinit_full(workspace, manifest, verbose=False):
+    """
+    Does full submodule deinit based on the current combo.
+
+        workspace - Path to the current workspace.
+        manifest  - The current manifest parser object.
+    """
+    print(strings.SUBMOD_DEINIT_FULL)
+    current_combo = manifest.general_config.current_combo
+    repo_sources = manifest.get_repo_sources(current_combo)
+    for source in repo_sources:
+        if _get_submodule_enable(manifest, source.remote_name, current_combo):
+            # Open the repo and process submodules
+            try:
+                repo = git.Repo(os.path.join(workspace, source.root))
+            except Exception as repo_error:
+                if args.verbose:
+                    print(strings.SUBMOD_EXCEPTION.format(repo_error))
+                continue
+            _deinit(repo, None, verbose)
+
+
+def init_full(workspace, manifest, verbose=False):
+    """
+    Does full submodule init based on the current combo.
+
+        workspace - Path to the current workspace.
+        manifest  - The current manifest parser object.
+    """
+    print(strings.SUBMOD_INIT_FULL)
+    current_combo = manifest.general_config.current_combo
+    repo_sources = manifest.get_repo_sources(current_combo)
+    for source in repo_sources:
+        if _get_submodule_enable(manifest, source.remote_name, current_combo):
+            # Open the repo and process submodules
+            try:
+                repo = git.Repo(os.path.join(workspace, source.root))
+            except Exception as repo_error:
+                if args.verbose:
+                    print(strings.SUBMOD_EXCEPTION.format(repo_error))
+                continue
+            _update(repo, None, verbose, True)
+
+
+def deinit_submodules(workspace, start_manifest, start_combo,
+                      end_manifest=None, end_combo=None,
+                      verbose=False):
+    """
+    Deinitializes the submodules for a project.
+
+        workspace      - Path to the current workspace.
+        start_manifest - The manifest parser object for the project.
+        start_combo    - The combination name at the start of the operation.
+        end_manifest   - The manifest parser object for the project if the manifest is being updated.  If the
+                         manifest file is not being updated us the value None.
+        end_combo      - The combination name at the end of the operation.  If the combo will not change
+                         use the value None.
+        verbose        - Enable verbose messages.
+    """
+    # Process each repo that may have submodules enabled
+    print(strings.SUBMOD_DEINIT)
+    repo_sources = start_manifest.get_repo_sources(start_combo)
+    for source in repo_sources:
+        # Open the repo and process submodules
+        try:
+            repo = git.Repo(os.path.join(workspace, source.root))
+        except Exception as repo_error:
+            if args.verbose:
+                print(strings.SUBMOD_EXCEPTION.format(repo_error))
+            continue
+
+        # Collect the submodule initialization data from manifest as well as if submodules
+        # should be processed for the repo.
+        start_subs, start_subs_enabled, end_subs, end_subs_enabled = _get_submodule_state(source.remote_name,
+                                                                                          start_manifest,
+                                                                                          start_combo,
+                                                                                          end_manifest,
+                                                                                          end_combo)
+        if not start_subs_enabled and not end_subs_enabled:
+            # At this point submodules are not enabled on this repo
+            continue
+
+        # Compute the list of submodules that need to be removed and added
+        deinit_list = _compute_change(start_subs, end_subs)
+
+        # Deinitialize submodules
+        if (start_subs_enabled and not end_subs_enabled) or (len(start_subs) == 0 and len(end_subs) != 0):
+            # Submodules are being disabled for the entire repo so do a
+            # full deinit on the repo.
+            _deinit(repo, None, verbose)
+        else:
+            # Do the deinit based on the list
+            _deinit(repo, deinit_list, verbose)
+
+

The comments mention needing to use the start combo/end combo parameters but I only see one combo_name parameter in the definition of this function. Additionally  init_submodules implies that this will initialize them not update them as described in the comment.
+def init_submodules(workspace, manifest, combo_name, verbose=False):
+    """
+    Updates the submodules for a specific repo.  Note that operations like checkout will need to use
+    the start_combo and end_combo parameter.  Operations like clone will just need to use the
+    start_combo parameter.
+
+        workspace  - Path to the current workspace.
+        manifest   - The manifest parser object for the project.
+        combo_name - The combination name at the start of the operation.
+        verbose    - Enable verbose messages.
+    """
+    # Process each repo that may have submodules enabled
+    print(strings.SUBMOD_INIT_UPDATE)
+    repo_sources = manifest.get_repo_sources(combo_name)
+    for source in repo_sources:
+        # Open the repo and process submodules
+        try:
+            repo = git.Repo(os.path.join(workspace, source.root))
+        except Exception as repo_error:
+            if args.verbose:
+                print(strings.SUBMOD_EXCEPTION.format(repo_error))
+            continue
+
+        # Collect the submodule initialization data from manifest as well as if submodules
+        # should be processed for the repo.
+        repo_subs = manifest.get_submodule_init_paths(source.remote_name, combo_name)
+        repo_subs_enabled = _get_submodule_enable(manifest, source.remote_name, combo_name)
+        if not repo_subs_enabled:
+            continue
+
+        # Initialize submodules
+        if len(repo_subs) > 0:
+            _init(repo, repo_subs, verbose)
+
+        # Perform sync/update
+        if len(repo_subs) == 0:
+            _update(repo, None, verbose)
+        else:
+            _update(repo, repo_subs, verbose)
+
+
+if __name__ == '__main__':
+    def parse_args():
+        parser = argparse.ArgumentParser()
+        parser.add_argument('manifest_file', metavar='MANIFEST', help=arguments.SUBMOD_MANIFEST_HELP)
+        parser.add_argument('--combo', default=None, help=arguments.SUBMOD_COMBO_HELP)
+        parser.add_argument('--new-manifest', default=None, help=arguments.SUBMOD_NEW_MANIFEST_HELP)
+        parser.add_argument('--new-combo', default=None, help=arguments.SUBMOD_NEW_COMBO_HELP)
+        parser.add_argument('--deinit', action='store_true', help=arguments.SUBMOD_DEINIT_HELP)
+        parser.add_argument('--init-full', action='store_true', help=arguments.SUBMOD_INIT_FULL_HELP)
+        parser.add_argument('--deinit-full', action='store_true', help=arguments.SUBMOD_DEINIT_FULL_HELP)
+        parser.add_argument('--workspace', default='.', help=arguments.SUBMOD_WORKSPACE_HELP)
+        parser.add_argument('--verbose', action='store_true', help=arguments.SUBMOD_VERBOSE_HELP)
+        return parser.parse_args()
+
+    def main(args):
+        # Optional data init
+        new_manifest = None
+        all_new_combos = None
+
+        # Extract basic manifest data
+        manifest = ManifestXml(args.manifest_file)
+        init_manifest = manifest
+        all_combos = manifest.combinations
+        all_combos.extend(manifest.archived_combinations)
+        if args.new_manifest is not None:
+            new_manifest = ManifestXml(args.new_manifest)
+            all_new_combos = new_manifest.combinations
+            all_new_combos.extend(new_manifest.archived_combinations)
+            init_manifest = new_manifest
+
+        # Determine current and new combo information
+        current_combo = manifest.general_config.current_combo
+        new_combo = args.new_combo
+        for combo in all_combos:
+            if args.combo is not None:
+                if args.combo.lower() == combo.name.lower():
+                    current_combo = combo.name
+            if args.new_combo is not None:
+                if args.new_combo.lower() == combo.name.lower():
+                    new_combo = combo.name
+        init_combo = current_combo
+        if new_combo is not None:
+            init_combo = new_combo
+
+        if args.deinit_full:
+            deinit_full(args.workspace, manifest, args.verbose)
+        elif args.init_full:
+            init_full(args.workspace, manifest, args.verbose)
+        else:
+            if args.deinit:
+                deinit_submodules(args.workspace, manifest, current_combo,
+                                  new_manifest, new_combo, args.verbose)
+            init_submodules(args.workspace, init_manifest, init_combo, 
+ args.verbose)
+
+        return 0
+
+    try:
+        args = parse_args()
+        sys.exit(main(args))
+    except Exception:
+        if args.verbose:
+            traceback.print_exc()
+        sys.exit(1)
diff --git a/setup.py b/setup.py
index 5d2f55a..a855000 100755
--- a/setup.py
+++ b/setup.py
@@ -1,27 +1,27 @@
-#!/usr/bin/env python3
-#
-## @file setup.py
-#
-# Copyright (c) 2017 - 2020, Intel Corporation. All rights reserved.<BR> -# SPDX-License-Identifier: BSD-2-Clause-Patent -#
-
-from setuptools import setup
-
-setup(name='edkrepo',
-      version='2.0.0',
-      description='The edkrepo tools',
-      packages=['edkrepo', 'edkrepo.commands', 'edkrepo.commands.arguments', 'edkrepo.commands.humble',
+#!/usr/bin/env python3
+#
+## @file setup.py
+#
+# Copyright (c) 2017 - 2020, Intel Corporation. All rights 
+reserved.<BR> # SPDX-License-Identifier: BSD-2-Clause-Patent #
+
+from setuptools import setup
+
+setup(name='edkrepo',
+      version='2.0.0',
+      description='The edkrepo tools',
+      packages=['edkrepo', 'edkrepo.commands', 
+'edkrepo.commands.arguments', 'edkrepo.commands.humble',
                 'edkrepo.git_automation', 'edkrepo.common', 'edkrepo.common.workspace_maintenance',
                 'edkrepo.common.workspace_maintenance.humble', 'edkrepo.config', 'edkrepo.config.humble',
-                'edkrepo_manifest_parser', 'project_utils'],
-      package_data={
-         },
-      include_package_data=True,
-      entry_points={
-          'console_scripts': [
-              'edkrepo = edkrepo.edkrepo_entry_point:main',
-              'command_completion_edkrepo = edkrepo.command_completion_edkrepo:main'
-              ]
-          }
-      )
+                'edkrepo_manifest_parser', 'project_utils', 'project_utils.arguments'],
+      package_data={
+         },
+      include_package_data=True,
+      entry_points={
+          'console_scripts': [
+              'edkrepo = edkrepo.edkrepo_entry_point:main',
+              'command_completion_edkrepo = edkrepo.command_completion_edkrepo:main'
+              ]
+          }
+      )
--
2.21.0.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code
  2020-05-17  1:29 ` [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code Bjorge, Erik C
@ 2020-05-19  3:01   ` Ashley E Desimone
  2020-05-19  3:13     ` Bjorge, Erik C
  0 siblings, 1 reply; 8+ messages in thread
From: Ashley E Desimone @ 2020-05-19  3:01 UTC (permalink / raw)
  To: Bjorge, Erik C, devel@edk2.groups.io
  Cc: Desimone, Nathaniel L, Pandya, Puja, Bret Barkelew,
	Agyeman, Prince

Hi Erik,

You have not updated the definition of clone_repos() in common_repo_functions.py to reflect that you are no longer passing the skip_submodule parameter which is not set to a default value. Additionally you have not removed the calls to the previous submodule initialization code from clone_repos(). 

Thanks,
Ashley

-----Original Message-----
From: Bjorge, Erik C <erik.c.bjorge@intel.com> 
Sent: Saturday, May 16, 2020 6:29 PM
To: devel@edk2.groups.io
Cc: Desimone, Ashley E <ashley.e.desimone@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, Puja <puja.pandya@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
Subject: [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code

Replaced use of maintain_submodules to use new common code.  This allows for selective submodule initialization via the manifest file.

Cc: Ashley E Desimone <ashley.e.desimone@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Puja Pandya <puja.pandya@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Prince Agyeman <prince.agyeman@intel.com>
Cc: Erik Bjorge <erik.c.bjorge@intel.com>
Signed-off-by: Erik Bjorge <erik.c.bjorge@intel.com>
---
 edkrepo/commands/checkout_pin_command.py |  6 ++++
 edkrepo/commands/clone_command.py        | 20 +++++++----
 edkrepo/commands/sync_command.py         | 23 +++++++++----
 edkrepo/common/common_repo_functions.py  | 43 +++++-------------------
 4 files changed, 44 insertions(+), 48 deletions(-)

diff --git a/edkrepo/commands/checkout_pin_command.py b/edkrepo/commands/checkout_pin_command.py
index 4aaf2b5..c250c49 100644
--- a/edkrepo/commands/checkout_pin_command.py
+++ b/edkrepo/commands/checkout_pin_command.py
@@ -22,6 +22,8 @@ from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import list  from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import find_source_manifest_repo  from edkrepo.config.config_factory import get_workspace_path, get_workspace_manifest  from edkrepo_manifest_parser.edk_manifest import ManifestXml
+from project_utils.submodule import deinit_submodules, init_submodules
+
 
 class CheckoutPinCommand(EdkrepoCommand):
     def __init__(self):
@@ -68,11 +70,15 @@ class CheckoutPinCommand(EdkrepoCommand):
         if sparse_enabled:
             print(SPARSE_RESET)
             reset_sparse_checkout(workspace_path, manifest_sources)
+        submodule_combo = pin.general_config.current_combo
+        deinit_submodules(workspace_path, manifest, manifest.general_config.current_combo,
+                          pin, submodule_combo, args.verbose)
         pin_repo_sources = pin.get_repo_sources(pin.general_config.current_combo)
         try:
             checkout_repos(args.verbose, args.override, pin_repo_sources, workspace_path, manifest)
             manifest.write_current_combo(humble.PIN_COMBO.format(args.pinfile))
         finally:
+            init_submodules(workspace_path, pin, submodule_combo, 
+ args.verbose)
             if sparse_enabled:
                 print(SPARSE_CHECKOUT)
                 sparse_checkout(workspace_path, pin_repo_sources, manifest) diff --git a/edkrepo/commands/clone_command.py b/edkrepo/commands/clone_command.py
index 179aaf7..653e10b 100644
--- a/edkrepo/commands/clone_command.py
+++ b/edkrepo/commands/clone_command.py
@@ -24,6 +24,7 @@ from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import pull  from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import list_available_manifest_repos  from edkrepo.common.workspace_maintenance.humble.manifest_repos_maintenance_humble import PROJ_NOT_IN_REPO, SOURCE_MANIFEST_REPO_NOT_FOUND  from edkrepo_manifest_parser.edk_manifest import CiIndexXml, ManifestXml
+from project_utils.submodule import init_submodules
 
 
 class CloneCommand(EdkrepoCommand):
@@ -104,7 +105,6 @@ class CloneCommand(EdkrepoCommand):
         manifest = ManifestXml(local_manifest_path)
 
         # Process the combination name and make sure it can be found in the manifest
-        combo_name = None
         if args.Combination is not None:
             try:
                 combo_name = case_insensitive_single_match(args.Combination, combinations_in_manifest(manifest))
@@ -118,13 +118,15 @@ class CloneCommand(EdkrepoCommand):
             # Since pin files are subset of manifest files they do not have a "default combo" it is set to None. In this
             # case use the current_combo instead.
             combo_name = manifest.general_config.current_combo
+        else:
+            # If a combo was not specified or a pin file used the default combo should be cloned.  Also ensure that the
+            # current combo is updated to match.
+            combo_name = manifest.general_config.default_combo
+            manifest.write_current_combo(combo_name)
 
         # Get the list of repos to clone and clone them
-        manifest_config = manifest.general_config
-        if combo_name:
-            repo_sources_to_clone = manifest.get_repo_sources(combo_name)
-        else:
-            repo_sources_to_clone = manifest.get_repo_sources(manifest_config.default_combo)
+        repo_sources_to_clone = manifest.get_repo_sources(combo_name)
+
         #check that the repo sources do not contain duplicated local roots
         local_roots = [r.root for r in repo_sources_to_clone]
         for root in local_roots:
@@ -137,7 +139,11 @@ class CloneCommand(EdkrepoCommand):
         # Set up submodule alt url config settings prior to cloning any repos
         submodule_included_configs = write_included_config(manifest.remotes, manifest.submodule_alternate_remotes, local_manifest_dir)
         write_conditional_include(workspace_dir, repo_sources_to_clone, submodule_included_configs)
-        clone_repos(args, workspace_dir, repo_sources_to_clone, project_client_side_hooks, config, args.skip_submodule, manifest)
+        clone_repos(args, workspace_dir, repo_sources_to_clone, 
+ project_client_side_hooks, config, manifest)
+
+        # Init submodules
+        if not args.skip_submodule:
+            init_submodules(workspace_dir, manifest, combo_name, 
+ args.verbose)
 
         # Perform a sparse checkout if requested.
         use_sparse = args.sparse
diff --git a/edkrepo/commands/sync_command.py b/edkrepo/commands/sync_command.py
index da99397..d3c724f 100644
--- a/edkrepo/commands/sync_command.py
+++ b/edkrepo/commands/sync_command.py
@@ -38,7 +38,7 @@ from edkrepo.common.common_repo_functions import checkout_repos, check_dirty_rep  from edkrepo.common.common_repo_functions import update_editor_config  from edkrepo.common.common_repo_functions import update_repo_commit_template, get_latest_sha  from edkrepo.common.common_repo_functions import has_primary_repo_remote, fetch_from_primary_repo, in_sync_with_primary -from edkrepo.common.common_repo_functions import update_hooks, maintain_submodules, combinations_in_manifest
+from edkrepo.common.common_repo_functions import update_hooks, 
+combinations_in_manifest
 from edkrepo.common.common_repo_functions import write_included_config, remove_included_config  from edkrepo.common.workspace_maintenance.workspace_maintenance import generate_name_for_obsolete_backup  from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import pull_workspace_manifest_repo @@ -48,6 +48,7 @@ from edkrepo.common.ui_functions import init_color_console  from edkrepo.config.config_factory import get_workspace_path, get_workspace_manifest, get_edkrepo_global_data_directory  from edkrepo.config.config_factory import get_workspace_manifest_file  from edkrepo_manifest_parser.edk_manifest import CiIndexXml, ManifestXml
+from project_utils.submodule import deinit_submodules, init_submodules
 
 
 class SyncCommand(EdkrepoCommand):
@@ -84,6 +85,7 @@ class SyncCommand(EdkrepoCommand):
         current_combo = initial_manifest.general_config.current_combo
         initial_sources = initial_manifest.get_repo_sources(current_combo)
         initial_hooks = initial_manifest.repo_hooks
+        initial_combo = current_combo
 
         source_global_manifest_repo = find_source_manifest_repo(initial_manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo)
         pull_workspace_manifest_repo(initial_manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo, False) @@ -100,6 +102,7 @@ class SyncCommand(EdkrepoCommand):
         if not args.update_local_manifest:
             self.__check_for_new_manifest(args, config, initial_manifest, workspace_path, global_manifest_directory)
         check_dirty_repos(initial_manifest, workspace_path)
+
         # Determine if sparse checkout needs to be disabled for this operation
         sparse_settings = initial_manifest.sparse_settings
         sparse_enabled = sparse_checkout_enabled(workspace_path, initial_sources) @@ -113,7 +116,7 @@ class SyncCommand(EdkrepoCommand):
             reset_sparse_checkout(workspace_path, initial_sources)
 
         # Get the latest manifest if requested
-        if args.update_local_manifest: #NOTE: hyphens in arg name replaced with underscores due to argparse
+        if args.update_local_manifest:  # NOTE: hyphens in arg name 
+ replaced with underscores due to argparse
             self.__update_local_manifest(args, config, initial_manifest, workspace_path, global_manifest_directory)
         manifest = get_workspace_manifest()
         if args.update_local_manifest:
@@ -127,6 +130,12 @@ class SyncCommand(EdkrepoCommand):
             repo_sources_to_sync = manifest.get_repo_sources(current_combo)
         manifest.write_current_combo(current_combo)
 
+        # At this point both new and old manifest files are ready so we can deinit any
+        # submodules that are removed due to a manifest update.
+        if not args.skip_submodule:
+            deinit_submodules(workspace_path, initial_manifest, initial_combo,
+                              manifest, current_combo, args.verbose)
+
         sync_error = False
         # Calculate the hooks which need to be updated, added or removed for the sync
         if args.update_local_manifest:
@@ -195,16 +204,16 @@ class SyncCommand(EdkrepoCommand):
             elif args.verbose:
                 print(NO_SYNC_DETACHED_HEAD.format(repo_to_sync.root))
 
-            if not args.skip_submodule:
-                if repo_to_sync.enable_submodule:
-                    # Perform submodule updates and url redirection
-                    maintain_submodules(repo_to_sync, repo)
             # Update commit message templates
             update_repo_commit_template(workspace_path, repo, repo_to_sync, config, source_global_manifest_repo)
 
         if sync_error:
             print(SYNC_ERROR)
 
+        # Initialize submodules
+        if not args.skip_submodule:
+            init_submodules(workspace_path, manifest, current_combo, 
+ args.verbose)
+
         # Restore sparse checkout state
         if sparse_enabled:
             print(SPARSE_CHECKOUT)
@@ -326,7 +335,7 @@ class SyncCommand(EdkrepoCommand):
                 print(path_to_source)
             if len(sources_to_remove) > 0:
                 print(SYNC_REMOVE_LIST_END_FORMATTING)
-            clone_repos(args, workspace_path, sources_to_clone, new_manifest_to_check.repo_hooks, config, args.skip_submodule, new_manifest_to_check)
+            clone_repos(args, workspace_path, sources_to_clone, 
+ new_manifest_to_check.repo_hooks, config, new_manifest_to_check)
             # Make a list of and only checkout repos that were newly cloned. Sync keeps repos on their initial active branches
             # cloning the entire combo can prevent existing repos from correctly being returned to their proper branch
             repos_to_checkout = []
diff --git a/edkrepo/common/common_repo_functions.py b/edkrepo/common/common_repo_functions.py
index 61133f1..18e545b 100644
--- a/edkrepo/common/common_repo_functions.py
+++ b/edkrepo/common/common_repo_functions.py
@@ -68,12 +68,13 @@ from edkrepo_manifest_parser.edk_manifest_validation import validate_manifestrep  from edkrepo_manifest_parser.edk_manifest_validation import get_manifest_validation_status  from edkrepo_manifest_parser.edk_manifest_validation import print_manifest_errors  from edkrepo_manifest_parser.edk_manifest_validation import validate_manifestfiles
+from project_utils.submodule import deinit_submodules, init_submodules
 
 CLEAR_LINE = '\x1b[K'
 DEFAULT_REMOTE_NAME = 'origin'
 PRIMARY_REMOTE_NAME = 'primary'
 
-def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, skip_submodule, manifest):
+def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, manifest):
     for repo_to_clone in repos_to_clone:
         local_repo_path = os.path.join(workspace_dir, repo_to_clone.root)
         local_repo_url = repo_to_clone.remote_url @@ -108,10 +109,6 @@ def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks,
         else:
             raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-        if not skip_submodule:
-            if repo_to_clone.enable_submodule:
-                maintain_submodules(repo_to_clone, repo, args.verbose)
-
         try:
             if 'source_manifest_repo' in vars(args).keys():
                 src_manifest_repo = find_source_manifest_repo(manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo) @@ -184,32 +181,6 @@ def write_conditional_include(workspace_path, repo_sources, included_configs):
                     gitglobalconfig.add_section(section)
                     gitglobalconfig.set(section, 'path', path)
 
-def maintain_submodules(repo_sources, repo, verbose = False):
-    try:
-        output_data = repo.git.execute(['git', 'submodule', 'init'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-        output_data = repo.git.execute(['git', 'submodule', 'sync', '--recursive'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-        output_data = repo.git.execute(['git', 'submodule', 'update', '--recursive'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-    except:
-        raise EdkrepoGitException(SUBMODULE_FAILURE.format(repo_sources.remote_name))
-
 def install_hooks(hooks, local_repo_path, repo_for_install, config, global_manifest_directory):
     # Determine the which hooks are for the repo in question and which are from a URL based source or are in a global
     # manifest repo relative path
@@ -399,9 +370,6 @@ def checkout_repos(verbose, override, repos_to_checkout, workspace_path, manifes
         else:
             raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-        if repo_to_checkout.enable_submodule:
-            maintain_submodules(repo_to_checkout, repo, verbose)
-
 def validate_manifest_repo(manifest_repo, verbose=False, archived=False):
     print(VERIFY_GLOBAL)
     if archived:
@@ -514,9 +482,11 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
     # Create combo_or_sha so we have original input and do not introduce any
     # unintended behavior by messing with parameters.
     combo_or_sha = combination_or_sha
+    submodule_combo = manifest.general_config.current_combo
     try:
         # Try to handle normalize combo name to match the manifest file.
         combo_or_sha = case_insensitive_single_match(combo_or_sha, combinations_in_manifest(manifest))
+        submodule_combo = combo_or_sha
     except:
         # No match so leave it alone.  It must be a SHA1 or a bad combo name.
         pass
@@ -528,6 +498,10 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
         log=log)
     initial_repo_sources = manifest.get_repo_sources(manifest.general_config.current_combo)
 
+    # Deinit any submodules that have been removed.
+    deinit_submodules(workspace_path, manifest, manifest.general_config.current_combo,
+                      manifest, submodule_combo, verbose)
+
     # Disable sparse checkout
     current_repos = initial_repo_sources
     sparse_enabled = sparse_checkout_enabled(workspace_path, initial_repo_sources) @@ -566,6 +540,7 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
         # Return to the initial combo, since there was an issue with cheking out the selected combo
         checkout_repos(verbose, override, initial_repo_sources, workspace_path, manifest)
     finally:
+        init_submodules(workspace_path, manifest, submodule_combo, 
+ verbose)
         if sparse_enabled or sparse_diff:
             print(SPARSE_CHECKOUT)
             sparse_checkout(workspace_path, current_repos, manifest)
--
2.21.0.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code
  2020-05-19  3:01   ` Ashley E Desimone
@ 2020-05-19  3:13     ` Bjorge, Erik C
  2020-05-19 16:45       ` Ashley E Desimone
  0 siblings, 1 reply; 8+ messages in thread
From: Bjorge, Erik C @ 2020-05-19  3:13 UTC (permalink / raw)
  To: Desimone, Ashley E, devel@edk2.groups.io
  Cc: Desimone, Nathaniel L, Pandya, Puja, Bret Barkelew,
	Agyeman, Prince

Ashley,

I am confused.  Here are the lines from the patch that remove the skip_submodule parameter.

-def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, skip_submodule, manifest):
+def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, manifest):

Here is the code from below where the call is removed in clone_repos.

         else:
             raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-        if not skip_submodule:
-            if repo_to_clone.enable_submodule:
-                maintain_submodules(repo_to_clone, repo, args.verbose)
-

Can you please double check these comments?

Thanks,
-Erik

-----Original Message-----
From: Desimone, Ashley E <ashley.e.desimone@intel.com> 
Sent: Monday, May 18, 2020 8:01 PM
To: Bjorge, Erik C <erik.c.bjorge@intel.com>; devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, Puja <puja.pandya@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
Subject: RE: [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code

Hi Erik,

You have not updated the definition of clone_repos() in common_repo_functions.py to reflect that you are no longer passing the skip_submodule parameter which is not set to a default value. Additionally you have not removed the calls to the previous submodule initialization code from clone_repos(). 

Thanks,
Ashley

-----Original Message-----
From: Bjorge, Erik C <erik.c.bjorge@intel.com>
Sent: Saturday, May 16, 2020 6:29 PM
To: devel@edk2.groups.io
Cc: Desimone, Ashley E <ashley.e.desimone@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, Puja <puja.pandya@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
Subject: [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code

Replaced use of maintain_submodules to use new common code.  This allows for selective submodule initialization via the manifest file.

Cc: Ashley E Desimone <ashley.e.desimone@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Puja Pandya <puja.pandya@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Prince Agyeman <prince.agyeman@intel.com>
Cc: Erik Bjorge <erik.c.bjorge@intel.com>
Signed-off-by: Erik Bjorge <erik.c.bjorge@intel.com>
---
 edkrepo/commands/checkout_pin_command.py |  6 ++++
 edkrepo/commands/clone_command.py        | 20 +++++++----
 edkrepo/commands/sync_command.py         | 23 +++++++++----
 edkrepo/common/common_repo_functions.py  | 43 +++++-------------------
 4 files changed, 44 insertions(+), 48 deletions(-)

diff --git a/edkrepo/commands/checkout_pin_command.py b/edkrepo/commands/checkout_pin_command.py
index 4aaf2b5..c250c49 100644
--- a/edkrepo/commands/checkout_pin_command.py
+++ b/edkrepo/commands/checkout_pin_command.py
@@ -22,6 +22,8 @@ from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import list  from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import find_source_manifest_repo  from edkrepo.config.config_factory import get_workspace_path, get_workspace_manifest  from edkrepo_manifest_parser.edk_manifest import ManifestXml
+from project_utils.submodule import deinit_submodules, init_submodules
+
 
 class CheckoutPinCommand(EdkrepoCommand):
     def __init__(self):
@@ -68,11 +70,15 @@ class CheckoutPinCommand(EdkrepoCommand):
         if sparse_enabled:
             print(SPARSE_RESET)
             reset_sparse_checkout(workspace_path, manifest_sources)
+        submodule_combo = pin.general_config.current_combo
+        deinit_submodules(workspace_path, manifest, manifest.general_config.current_combo,
+                          pin, submodule_combo, args.verbose)
         pin_repo_sources = pin.get_repo_sources(pin.general_config.current_combo)
         try:
             checkout_repos(args.verbose, args.override, pin_repo_sources, workspace_path, manifest)
             manifest.write_current_combo(humble.PIN_COMBO.format(args.pinfile))
         finally:
+            init_submodules(workspace_path, pin, submodule_combo,
+ args.verbose)
             if sparse_enabled:
                 print(SPARSE_CHECKOUT)
                 sparse_checkout(workspace_path, pin_repo_sources, manifest) diff --git a/edkrepo/commands/clone_command.py b/edkrepo/commands/clone_command.py
index 179aaf7..653e10b 100644
--- a/edkrepo/commands/clone_command.py
+++ b/edkrepo/commands/clone_command.py
@@ -24,6 +24,7 @@ from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import pull  from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import list_available_manifest_repos  from edkrepo.common.workspace_maintenance.humble.manifest_repos_maintenance_humble import PROJ_NOT_IN_REPO, SOURCE_MANIFEST_REPO_NOT_FOUND  from edkrepo_manifest_parser.edk_manifest import CiIndexXml, ManifestXml
+from project_utils.submodule import init_submodules
 
 
 class CloneCommand(EdkrepoCommand):
@@ -104,7 +105,6 @@ class CloneCommand(EdkrepoCommand):
         manifest = ManifestXml(local_manifest_path)
 
         # Process the combination name and make sure it can be found in the manifest
-        combo_name = None
         if args.Combination is not None:
             try:
                 combo_name = case_insensitive_single_match(args.Combination, combinations_in_manifest(manifest))
@@ -118,13 +118,15 @@ class CloneCommand(EdkrepoCommand):
             # Since pin files are subset of manifest files they do not have a "default combo" it is set to None. In this
             # case use the current_combo instead.
             combo_name = manifest.general_config.current_combo
+        else:
+            # If a combo was not specified or a pin file used the default combo should be cloned.  Also ensure that the
+            # current combo is updated to match.
+            combo_name = manifest.general_config.default_combo
+            manifest.write_current_combo(combo_name)
 
         # Get the list of repos to clone and clone them
-        manifest_config = manifest.general_config
-        if combo_name:
-            repo_sources_to_clone = manifest.get_repo_sources(combo_name)
-        else:
-            repo_sources_to_clone = manifest.get_repo_sources(manifest_config.default_combo)
+        repo_sources_to_clone = manifest.get_repo_sources(combo_name)
+
         #check that the repo sources do not contain duplicated local roots
         local_roots = [r.root for r in repo_sources_to_clone]
         for root in local_roots:
@@ -137,7 +139,11 @@ class CloneCommand(EdkrepoCommand):
         # Set up submodule alt url config settings prior to cloning any repos
         submodule_included_configs = write_included_config(manifest.remotes, manifest.submodule_alternate_remotes, local_manifest_dir)
         write_conditional_include(workspace_dir, repo_sources_to_clone, submodule_included_configs)
-        clone_repos(args, workspace_dir, repo_sources_to_clone, project_client_side_hooks, config, args.skip_submodule, manifest)
+        clone_repos(args, workspace_dir, repo_sources_to_clone, 
+ project_client_side_hooks, config, manifest)
+
+        # Init submodules
+        if not args.skip_submodule:
+            init_submodules(workspace_dir, manifest, combo_name,
+ args.verbose)
 
         # Perform a sparse checkout if requested.
         use_sparse = args.sparse
diff --git a/edkrepo/commands/sync_command.py b/edkrepo/commands/sync_command.py
index da99397..d3c724f 100644
--- a/edkrepo/commands/sync_command.py
+++ b/edkrepo/commands/sync_command.py
@@ -38,7 +38,7 @@ from edkrepo.common.common_repo_functions import checkout_repos, check_dirty_rep  from edkrepo.common.common_repo_functions import update_editor_config  from edkrepo.common.common_repo_functions import update_repo_commit_template, get_latest_sha  from edkrepo.common.common_repo_functions import has_primary_repo_remote, fetch_from_primary_repo, in_sync_with_primary -from edkrepo.common.common_repo_functions import update_hooks, maintain_submodules, combinations_in_manifest
+from edkrepo.common.common_repo_functions import update_hooks, 
+combinations_in_manifest
 from edkrepo.common.common_repo_functions import write_included_config, remove_included_config  from edkrepo.common.workspace_maintenance.workspace_maintenance import generate_name_for_obsolete_backup  from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import pull_workspace_manifest_repo @@ -48,6 +48,7 @@ from edkrepo.common.ui_functions import init_color_console  from edkrepo.config.config_factory import get_workspace_path, get_workspace_manifest, get_edkrepo_global_data_directory  from edkrepo.config.config_factory import get_workspace_manifest_file  from edkrepo_manifest_parser.edk_manifest import CiIndexXml, ManifestXml
+from project_utils.submodule import deinit_submodules, init_submodules
 
 
 class SyncCommand(EdkrepoCommand):
@@ -84,6 +85,7 @@ class SyncCommand(EdkrepoCommand):
         current_combo = initial_manifest.general_config.current_combo
         initial_sources = initial_manifest.get_repo_sources(current_combo)
         initial_hooks = initial_manifest.repo_hooks
+        initial_combo = current_combo
 
         source_global_manifest_repo = find_source_manifest_repo(initial_manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo)
         pull_workspace_manifest_repo(initial_manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo, False) @@ -100,6 +102,7 @@ class SyncCommand(EdkrepoCommand):
         if not args.update_local_manifest:
             self.__check_for_new_manifest(args, config, initial_manifest, workspace_path, global_manifest_directory)
         check_dirty_repos(initial_manifest, workspace_path)
+
         # Determine if sparse checkout needs to be disabled for this operation
         sparse_settings = initial_manifest.sparse_settings
         sparse_enabled = sparse_checkout_enabled(workspace_path, initial_sources) @@ -113,7 +116,7 @@ class SyncCommand(EdkrepoCommand):
             reset_sparse_checkout(workspace_path, initial_sources)
 
         # Get the latest manifest if requested
-        if args.update_local_manifest: #NOTE: hyphens in arg name replaced with underscores due to argparse
+        if args.update_local_manifest:  # NOTE: hyphens in arg name 
+ replaced with underscores due to argparse
             self.__update_local_manifest(args, config, initial_manifest, workspace_path, global_manifest_directory)
         manifest = get_workspace_manifest()
         if args.update_local_manifest:
@@ -127,6 +130,12 @@ class SyncCommand(EdkrepoCommand):
             repo_sources_to_sync = manifest.get_repo_sources(current_combo)
         manifest.write_current_combo(current_combo)
 
+        # At this point both new and old manifest files are ready so we can deinit any
+        # submodules that are removed due to a manifest update.
+        if not args.skip_submodule:
+            deinit_submodules(workspace_path, initial_manifest, initial_combo,
+                              manifest, current_combo, args.verbose)
+
         sync_error = False
         # Calculate the hooks which need to be updated, added or removed for the sync
         if args.update_local_manifest:
@@ -195,16 +204,16 @@ class SyncCommand(EdkrepoCommand):
             elif args.verbose:
                 print(NO_SYNC_DETACHED_HEAD.format(repo_to_sync.root))
 
-            if not args.skip_submodule:
-                if repo_to_sync.enable_submodule:
-                    # Perform submodule updates and url redirection
-                    maintain_submodules(repo_to_sync, repo)
             # Update commit message templates
             update_repo_commit_template(workspace_path, repo, repo_to_sync, config, source_global_manifest_repo)
 
         if sync_error:
             print(SYNC_ERROR)
 
+        # Initialize submodules
+        if not args.skip_submodule:
+            init_submodules(workspace_path, manifest, current_combo,
+ args.verbose)
+
         # Restore sparse checkout state
         if sparse_enabled:
             print(SPARSE_CHECKOUT)
@@ -326,7 +335,7 @@ class SyncCommand(EdkrepoCommand):
                 print(path_to_source)
             if len(sources_to_remove) > 0:
                 print(SYNC_REMOVE_LIST_END_FORMATTING)
-            clone_repos(args, workspace_path, sources_to_clone, new_manifest_to_check.repo_hooks, config, args.skip_submodule, new_manifest_to_check)
+            clone_repos(args, workspace_path, sources_to_clone, 
+ new_manifest_to_check.repo_hooks, config, new_manifest_to_check)
             # Make a list of and only checkout repos that were newly cloned. Sync keeps repos on their initial active branches
             # cloning the entire combo can prevent existing repos from correctly being returned to their proper branch
             repos_to_checkout = []
diff --git a/edkrepo/common/common_repo_functions.py b/edkrepo/common/common_repo_functions.py
index 61133f1..18e545b 100644
--- a/edkrepo/common/common_repo_functions.py
+++ b/edkrepo/common/common_repo_functions.py
@@ -68,12 +68,13 @@ from edkrepo_manifest_parser.edk_manifest_validation import validate_manifestrep  from edkrepo_manifest_parser.edk_manifest_validation import get_manifest_validation_status  from edkrepo_manifest_parser.edk_manifest_validation import print_manifest_errors  from edkrepo_manifest_parser.edk_manifest_validation import validate_manifestfiles
+from project_utils.submodule import deinit_submodules, init_submodules
 
 CLEAR_LINE = '\x1b[K'
 DEFAULT_REMOTE_NAME = 'origin'
 PRIMARY_REMOTE_NAME = 'primary'
 
-def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, skip_submodule, manifest):
+def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, manifest):
     for repo_to_clone in repos_to_clone:
         local_repo_path = os.path.join(workspace_dir, repo_to_clone.root)
         local_repo_url = repo_to_clone.remote_url @@ -108,10 +109,6 @@ def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks,
         else:
             raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-        if not skip_submodule:
-            if repo_to_clone.enable_submodule:
-                maintain_submodules(repo_to_clone, repo, args.verbose)
-
         try:
             if 'source_manifest_repo' in vars(args).keys():
                 src_manifest_repo = find_source_manifest_repo(manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo) @@ -184,32 +181,6 @@ def write_conditional_include(workspace_path, repo_sources, included_configs):
                     gitglobalconfig.add_section(section)
                     gitglobalconfig.set(section, 'path', path)
 
-def maintain_submodules(repo_sources, repo, verbose = False):
-    try:
-        output_data = repo.git.execute(['git', 'submodule', 'init'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-        output_data = repo.git.execute(['git', 'submodule', 'sync', '--recursive'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-        output_data = repo.git.execute(['git', 'submodule', 'update', '--recursive'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-    except:
-        raise EdkrepoGitException(SUBMODULE_FAILURE.format(repo_sources.remote_name))
-
 def install_hooks(hooks, local_repo_path, repo_for_install, config, global_manifest_directory):
     # Determine the which hooks are for the repo in question and which are from a URL based source or are in a global
     # manifest repo relative path
@@ -399,9 +370,6 @@ def checkout_repos(verbose, override, repos_to_checkout, workspace_path, manifes
         else:
             raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-        if repo_to_checkout.enable_submodule:
-            maintain_submodules(repo_to_checkout, repo, verbose)
-
 def validate_manifest_repo(manifest_repo, verbose=False, archived=False):
     print(VERIFY_GLOBAL)
     if archived:
@@ -514,9 +482,11 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
     # Create combo_or_sha so we have original input and do not introduce any
     # unintended behavior by messing with parameters.
     combo_or_sha = combination_or_sha
+    submodule_combo = manifest.general_config.current_combo
     try:
         # Try to handle normalize combo name to match the manifest file.
         combo_or_sha = case_insensitive_single_match(combo_or_sha, combinations_in_manifest(manifest))
+        submodule_combo = combo_or_sha
     except:
         # No match so leave it alone.  It must be a SHA1 or a bad combo name.
         pass
@@ -528,6 +498,10 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
         log=log)
     initial_repo_sources = manifest.get_repo_sources(manifest.general_config.current_combo)
 
+    # Deinit any submodules that have been removed.
+    deinit_submodules(workspace_path, manifest, manifest.general_config.current_combo,
+                      manifest, submodule_combo, verbose)
+
     # Disable sparse checkout
     current_repos = initial_repo_sources
     sparse_enabled = sparse_checkout_enabled(workspace_path, initial_repo_sources) @@ -566,6 +540,7 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
         # Return to the initial combo, since there was an issue with cheking out the selected combo
         checkout_repos(verbose, override, initial_repo_sources, workspace_path, manifest)
     finally:
+        init_submodules(workspace_path, manifest, submodule_combo,
+ verbose)
         if sparse_enabled or sparse_diff:
             print(SPARSE_CHECKOUT)
             sparse_checkout(workspace_path, current_repos, manifest)
--
2.21.0.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code
  2020-05-19  3:13     ` Bjorge, Erik C
@ 2020-05-19 16:45       ` Ashley E Desimone
  0 siblings, 0 replies; 8+ messages in thread
From: Ashley E Desimone @ 2020-05-19 16:45 UTC (permalink / raw)
  To: Bjorge, Erik C, devel@edk2.groups.io
  Cc: Desimone, Nathaniel L, Pandya, Puja, Bret Barkelew,
	Agyeman, Prince

Hi Erik,

Thanks for clarifying. I reapplied the patches this morning to look at and you are correct. Either I missed the content or made a mistake in applying the patches. Please ignore these comments.

Thanks,
Ashley

Reviewed-by: Ashley DeSimone <ashley.e.desimoen@intel.com>

-----Original Message-----
From: Bjorge, Erik C <erik.c.bjorge@intel.com> 
Sent: Monday, May 18, 2020 8:14 PM
To: Desimone, Ashley E <ashley.e.desimone@intel.com>; devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, Puja <puja.pandya@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
Subject: RE: [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code

Ashley,

I am confused.  Here are the lines from the patch that remove the skip_submodule parameter.

-def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, skip_submodule, manifest):
+def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, manifest):

Here is the code from below where the call is removed in clone_repos.

         else:
             raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-        if not skip_submodule:
-            if repo_to_clone.enable_submodule:
-                maintain_submodules(repo_to_clone, repo, args.verbose)
-

Can you please double check these comments?

Thanks,
-Erik

-----Original Message-----
From: Desimone, Ashley E <ashley.e.desimone@intel.com>
Sent: Monday, May 18, 2020 8:01 PM
To: Bjorge, Erik C <erik.c.bjorge@intel.com>; devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, Puja <puja.pandya@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
Subject: RE: [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code

Hi Erik,

You have not updated the definition of clone_repos() in common_repo_functions.py to reflect that you are no longer passing the skip_submodule parameter which is not set to a default value. Additionally you have not removed the calls to the previous submodule initialization code from clone_repos(). 

Thanks,
Ashley

-----Original Message-----
From: Bjorge, Erik C <erik.c.bjorge@intel.com>
Sent: Saturday, May 16, 2020 6:29 PM
To: devel@edk2.groups.io
Cc: Desimone, Ashley E <ashley.e.desimone@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, Puja <puja.pandya@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
Subject: [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code

Replaced use of maintain_submodules to use new common code.  This allows for selective submodule initialization via the manifest file.

Cc: Ashley E Desimone <ashley.e.desimone@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Puja Pandya <puja.pandya@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Prince Agyeman <prince.agyeman@intel.com>
Cc: Erik Bjorge <erik.c.bjorge@intel.com>
Signed-off-by: Erik Bjorge <erik.c.bjorge@intel.com>
---
 edkrepo/commands/checkout_pin_command.py |  6 ++++
 edkrepo/commands/clone_command.py        | 20 +++++++----
 edkrepo/commands/sync_command.py         | 23 +++++++++----
 edkrepo/common/common_repo_functions.py  | 43 +++++-------------------
 4 files changed, 44 insertions(+), 48 deletions(-)

diff --git a/edkrepo/commands/checkout_pin_command.py b/edkrepo/commands/checkout_pin_command.py
index 4aaf2b5..c250c49 100644
--- a/edkrepo/commands/checkout_pin_command.py
+++ b/edkrepo/commands/checkout_pin_command.py
@@ -22,6 +22,8 @@ from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import list  from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import find_source_manifest_repo  from edkrepo.config.config_factory import get_workspace_path, get_workspace_manifest  from edkrepo_manifest_parser.edk_manifest import ManifestXml
+from project_utils.submodule import deinit_submodules, init_submodules
+
 
 class CheckoutPinCommand(EdkrepoCommand):
     def __init__(self):
@@ -68,11 +70,15 @@ class CheckoutPinCommand(EdkrepoCommand):
         if sparse_enabled:
             print(SPARSE_RESET)
             reset_sparse_checkout(workspace_path, manifest_sources)
+        submodule_combo = pin.general_config.current_combo
+        deinit_submodules(workspace_path, manifest, manifest.general_config.current_combo,
+                          pin, submodule_combo, args.verbose)
         pin_repo_sources = pin.get_repo_sources(pin.general_config.current_combo)
         try:
             checkout_repos(args.verbose, args.override, pin_repo_sources, workspace_path, manifest)
             manifest.write_current_combo(humble.PIN_COMBO.format(args.pinfile))
         finally:
+            init_submodules(workspace_path, pin, submodule_combo,
+ args.verbose)
             if sparse_enabled:
                 print(SPARSE_CHECKOUT)
                 sparse_checkout(workspace_path, pin_repo_sources, manifest) diff --git a/edkrepo/commands/clone_command.py b/edkrepo/commands/clone_command.py
index 179aaf7..653e10b 100644
--- a/edkrepo/commands/clone_command.py
+++ b/edkrepo/commands/clone_command.py
@@ -24,6 +24,7 @@ from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import pull  from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import list_available_manifest_repos  from edkrepo.common.workspace_maintenance.humble.manifest_repos_maintenance_humble import PROJ_NOT_IN_REPO, SOURCE_MANIFEST_REPO_NOT_FOUND  from edkrepo_manifest_parser.edk_manifest import CiIndexXml, ManifestXml
+from project_utils.submodule import init_submodules
 
 
 class CloneCommand(EdkrepoCommand):
@@ -104,7 +105,6 @@ class CloneCommand(EdkrepoCommand):
         manifest = ManifestXml(local_manifest_path)
 
         # Process the combination name and make sure it can be found in the manifest
-        combo_name = None
         if args.Combination is not None:
             try:
                 combo_name = case_insensitive_single_match(args.Combination, combinations_in_manifest(manifest))
@@ -118,13 +118,15 @@ class CloneCommand(EdkrepoCommand):
             # Since pin files are subset of manifest files they do not have a "default combo" it is set to None. In this
             # case use the current_combo instead.
             combo_name = manifest.general_config.current_combo
+        else:
+            # If a combo was not specified or a pin file used the default combo should be cloned.  Also ensure that the
+            # current combo is updated to match.
+            combo_name = manifest.general_config.default_combo
+            manifest.write_current_combo(combo_name)
 
         # Get the list of repos to clone and clone them
-        manifest_config = manifest.general_config
-        if combo_name:
-            repo_sources_to_clone = manifest.get_repo_sources(combo_name)
-        else:
-            repo_sources_to_clone = manifest.get_repo_sources(manifest_config.default_combo)
+        repo_sources_to_clone = manifest.get_repo_sources(combo_name)
+
         #check that the repo sources do not contain duplicated local roots
         local_roots = [r.root for r in repo_sources_to_clone]
         for root in local_roots:
@@ -137,7 +139,11 @@ class CloneCommand(EdkrepoCommand):
         # Set up submodule alt url config settings prior to cloning any repos
         submodule_included_configs = write_included_config(manifest.remotes, manifest.submodule_alternate_remotes, local_manifest_dir)
         write_conditional_include(workspace_dir, repo_sources_to_clone, submodule_included_configs)
-        clone_repos(args, workspace_dir, repo_sources_to_clone, project_client_side_hooks, config, args.skip_submodule, manifest)
+        clone_repos(args, workspace_dir, repo_sources_to_clone, 
+ project_client_side_hooks, config, manifest)
+
+        # Init submodules
+        if not args.skip_submodule:
+            init_submodules(workspace_dir, manifest, combo_name,
+ args.verbose)
 
         # Perform a sparse checkout if requested.
         use_sparse = args.sparse
diff --git a/edkrepo/commands/sync_command.py b/edkrepo/commands/sync_command.py
index da99397..d3c724f 100644
--- a/edkrepo/commands/sync_command.py
+++ b/edkrepo/commands/sync_command.py
@@ -38,7 +38,7 @@ from edkrepo.common.common_repo_functions import checkout_repos, check_dirty_rep  from edkrepo.common.common_repo_functions import update_editor_config  from edkrepo.common.common_repo_functions import update_repo_commit_template, get_latest_sha  from edkrepo.common.common_repo_functions import has_primary_repo_remote, fetch_from_primary_repo, in_sync_with_primary -from edkrepo.common.common_repo_functions import update_hooks, maintain_submodules, combinations_in_manifest
+from edkrepo.common.common_repo_functions import update_hooks, 
+combinations_in_manifest
 from edkrepo.common.common_repo_functions import write_included_config, remove_included_config  from edkrepo.common.workspace_maintenance.workspace_maintenance import generate_name_for_obsolete_backup  from edkrepo.common.workspace_maintenance.manifest_repos_maintenance import pull_workspace_manifest_repo @@ -48,6 +48,7 @@ from edkrepo.common.ui_functions import init_color_console  from edkrepo.config.config_factory import get_workspace_path, get_workspace_manifest, get_edkrepo_global_data_directory  from edkrepo.config.config_factory import get_workspace_manifest_file  from edkrepo_manifest_parser.edk_manifest import CiIndexXml, ManifestXml
+from project_utils.submodule import deinit_submodules, init_submodules
 
 
 class SyncCommand(EdkrepoCommand):
@@ -84,6 +85,7 @@ class SyncCommand(EdkrepoCommand):
         current_combo = initial_manifest.general_config.current_combo
         initial_sources = initial_manifest.get_repo_sources(current_combo)
         initial_hooks = initial_manifest.repo_hooks
+        initial_combo = current_combo
 
         source_global_manifest_repo = find_source_manifest_repo(initial_manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo)
         pull_workspace_manifest_repo(initial_manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo, False) @@ -100,6 +102,7 @@ class SyncCommand(EdkrepoCommand):
         if not args.update_local_manifest:
             self.__check_for_new_manifest(args, config, initial_manifest, workspace_path, global_manifest_directory)
         check_dirty_repos(initial_manifest, workspace_path)
+
         # Determine if sparse checkout needs to be disabled for this operation
         sparse_settings = initial_manifest.sparse_settings
         sparse_enabled = sparse_checkout_enabled(workspace_path, initial_sources) @@ -113,7 +116,7 @@ class SyncCommand(EdkrepoCommand):
             reset_sparse_checkout(workspace_path, initial_sources)
 
         # Get the latest manifest if requested
-        if args.update_local_manifest: #NOTE: hyphens in arg name replaced with underscores due to argparse
+        if args.update_local_manifest:  # NOTE: hyphens in arg name 
+ replaced with underscores due to argparse
             self.__update_local_manifest(args, config, initial_manifest, workspace_path, global_manifest_directory)
         manifest = get_workspace_manifest()
         if args.update_local_manifest:
@@ -127,6 +130,12 @@ class SyncCommand(EdkrepoCommand):
             repo_sources_to_sync = manifest.get_repo_sources(current_combo)
         manifest.write_current_combo(current_combo)
 
+        # At this point both new and old manifest files are ready so we can deinit any
+        # submodules that are removed due to a manifest update.
+        if not args.skip_submodule:
+            deinit_submodules(workspace_path, initial_manifest, initial_combo,
+                              manifest, current_combo, args.verbose)
+
         sync_error = False
         # Calculate the hooks which need to be updated, added or removed for the sync
         if args.update_local_manifest:
@@ -195,16 +204,16 @@ class SyncCommand(EdkrepoCommand):
             elif args.verbose:
                 print(NO_SYNC_DETACHED_HEAD.format(repo_to_sync.root))
 
-            if not args.skip_submodule:
-                if repo_to_sync.enable_submodule:
-                    # Perform submodule updates and url redirection
-                    maintain_submodules(repo_to_sync, repo)
             # Update commit message templates
             update_repo_commit_template(workspace_path, repo, repo_to_sync, config, source_global_manifest_repo)
 
         if sync_error:
             print(SYNC_ERROR)
 
+        # Initialize submodules
+        if not args.skip_submodule:
+            init_submodules(workspace_path, manifest, current_combo,
+ args.verbose)
+
         # Restore sparse checkout state
         if sparse_enabled:
             print(SPARSE_CHECKOUT)
@@ -326,7 +335,7 @@ class SyncCommand(EdkrepoCommand):
                 print(path_to_source)
             if len(sources_to_remove) > 0:
                 print(SYNC_REMOVE_LIST_END_FORMATTING)
-            clone_repos(args, workspace_path, sources_to_clone, new_manifest_to_check.repo_hooks, config, args.skip_submodule, new_manifest_to_check)
+            clone_repos(args, workspace_path, sources_to_clone, 
+ new_manifest_to_check.repo_hooks, config, new_manifest_to_check)
             # Make a list of and only checkout repos that were newly cloned. Sync keeps repos on their initial active branches
             # cloning the entire combo can prevent existing repos from correctly being returned to their proper branch
             repos_to_checkout = []
diff --git a/edkrepo/common/common_repo_functions.py b/edkrepo/common/common_repo_functions.py
index 61133f1..18e545b 100644
--- a/edkrepo/common/common_repo_functions.py
+++ b/edkrepo/common/common_repo_functions.py
@@ -68,12 +68,13 @@ from edkrepo_manifest_parser.edk_manifest_validation import validate_manifestrep  from edkrepo_manifest_parser.edk_manifest_validation import get_manifest_validation_status  from edkrepo_manifest_parser.edk_manifest_validation import print_manifest_errors  from edkrepo_manifest_parser.edk_manifest_validation import validate_manifestfiles
+from project_utils.submodule import deinit_submodules, init_submodules
 
 CLEAR_LINE = '\x1b[K'
 DEFAULT_REMOTE_NAME = 'origin'
 PRIMARY_REMOTE_NAME = 'primary'
 
-def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, skip_submodule, manifest):
+def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks, config, manifest):
     for repo_to_clone in repos_to_clone:
         local_repo_path = os.path.join(workspace_dir, repo_to_clone.root)
         local_repo_url = repo_to_clone.remote_url @@ -108,10 +109,6 @@ def clone_repos(args, workspace_dir, repos_to_clone, project_client_side_hooks,
         else:
             raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-        if not skip_submodule:
-            if repo_to_clone.enable_submodule:
-                maintain_submodules(repo_to_clone, repo, args.verbose)
-
         try:
             if 'source_manifest_repo' in vars(args).keys():
                 src_manifest_repo = find_source_manifest_repo(manifest, config['cfg_file'], config['user_cfg_file'], args.source_manifest_repo) @@ -184,32 +181,6 @@ def write_conditional_include(workspace_path, repo_sources, included_configs):
                     gitglobalconfig.add_section(section)
                     gitglobalconfig.set(section, 'path', path)
 
-def maintain_submodules(repo_sources, repo, verbose = False):
-    try:
-        output_data = repo.git.execute(['git', 'submodule', 'init'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-        output_data = repo.git.execute(['git', 'submodule', 'sync', '--recursive'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-        output_data = repo.git.execute(['git', 'submodule', 'update', '--recursive'], with_extended_output=True, with_stdout=True)
-        if verbose and output_data[0]:
-            print(output_data[0])
-        if output_data[1]:
-            print(output_data[1])
-        if verbose and output_data[2]:
-            print(output_data[2])
-    except:
-        raise EdkrepoGitException(SUBMODULE_FAILURE.format(repo_sources.remote_name))
-
 def install_hooks(hooks, local_repo_path, repo_for_install, config, global_manifest_directory):
     # Determine the which hooks are for the repo in question and which are from a URL based source or are in a global
     # manifest repo relative path
@@ -399,9 +370,6 @@ def checkout_repos(verbose, override, repos_to_checkout, workspace_path, manifes
         else:
             raise EdkrepoManifestInvalidException(MISSING_BRANCH_COMMIT)
 
-        if repo_to_checkout.enable_submodule:
-            maintain_submodules(repo_to_checkout, repo, verbose)
-
 def validate_manifest_repo(manifest_repo, verbose=False, archived=False):
     print(VERIFY_GLOBAL)
     if archived:
@@ -514,9 +482,11 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
     # Create combo_or_sha so we have original input and do not introduce any
     # unintended behavior by messing with parameters.
     combo_or_sha = combination_or_sha
+    submodule_combo = manifest.general_config.current_combo
     try:
         # Try to handle normalize combo name to match the manifest file.
         combo_or_sha = case_insensitive_single_match(combo_or_sha, combinations_in_manifest(manifest))
+        submodule_combo = combo_or_sha
     except:
         # No match so leave it alone.  It must be a SHA1 or a bad combo name.
         pass
@@ -528,6 +498,10 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
         log=log)
     initial_repo_sources = manifest.get_repo_sources(manifest.general_config.current_combo)
 
+    # Deinit any submodules that have been removed.
+    deinit_submodules(workspace_path, manifest, manifest.general_config.current_combo,
+                      manifest, submodule_combo, verbose)
+
     # Disable sparse checkout
     current_repos = initial_repo_sources
     sparse_enabled = sparse_checkout_enabled(workspace_path, initial_repo_sources) @@ -566,6 +540,7 @@ def checkout(combination_or_sha, verbose=False, override=False, log=None):
         # Return to the initial combo, since there was an issue with cheking out the selected combo
         checkout_repos(verbose, override, initial_repo_sources, workspace_path, manifest)
     finally:
+        init_submodules(workspace_path, manifest, submodule_combo,
+ verbose)
         if sparse_enabled or sparse_diff:
             print(SPARSE_CHECKOUT)
             sparse_checkout(workspace_path, current_repos, manifest)
--
2.21.0.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [edk2-staging/EdkRepo] [PATCH v1 1/2] EdkRepo: Adding selective submodule init script
  2020-05-19  2:51   ` Ashley E Desimone
@ 2020-05-19 17:46     ` Bjorge, Erik C
  0 siblings, 0 replies; 8+ messages in thread
From: Bjorge, Erik C @ 2020-05-19 17:46 UTC (permalink / raw)
  To: Desimone, Ashley E, devel@edk2.groups.io
  Cc: Desimone, Nathaniel L, Pandya, Puja, Bret Barkelew,
	Agyeman, Prince

Good feedback.

- I will move _display_git_output to edkrepo.common.common_repo_functions and remove leading '_' from the name
- I will clean up the comments in init_submodules
- I will rename init_submodules to maintain_submodules to remove the confusion

I will submit a new patch set today.

Thanks,
-Erik

-----Original Message-----
From: Desimone, Ashley E <ashley.e.desimone@intel.com> 
Sent: Monday, May 18, 2020 7:51 PM
To: Bjorge, Erik C <erik.c.bjorge@intel.com>; devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, Puja <puja.pandya@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
Subject: RE: [edk2-staging/EdkRepo] [PATCH v1 1/2] EdkRepo: Adding selective submodule init script

Hi Erik,

Please see feedback inline below.

Thanks,
Ashley

-----Original Message-----
From: Bjorge, Erik C <erik.c.bjorge@intel.com>
Sent: Saturday, May 16, 2020 6:29 PM
To: devel@edk2.groups.io
Cc: Desimone, Ashley E <ashley.e.desimone@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, Puja <puja.pandya@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
Subject: [edk2-staging/EdkRepo] [PATCH v1 1/2] EdkRepo: Adding selective submodule init script

Adds selective submodule support functions and command line scripting support.  The existing support will be ported over to use this functionality.

Signed-off-by: Erik Bjorge <erik.c.bjorge@intel.com>
Cc: Ashley E Desimone <ashley.e.desimone@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Puja Pandya <puja.pandya@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Prince Agyeman <prince.agyeman@intel.com>
Cc: Erik Bjorge <erik.c.bjorge@intel.com>
---
 project_utils/arguments/__init__.py       |   8 +
 project_utils/arguments/submodule_args.py |  24 ++
 project_utils/project_utils_strings.py    |  24 ++
 project_utils/submodule.py                | 389 ++++++++++++++++++++++
 setup.py                                  |  50 +--
 5 files changed, 470 insertions(+), 25 deletions(-)  create mode 100644 project_utils/arguments/__init__.py
 create mode 100644 project_utils/arguments/submodule_args.py
 create mode 100644 project_utils/project_utils_strings.py
 create mode 100644 project_utils/submodule.py

diff --git a/project_utils/arguments/__init__.py b/project_utils/arguments/__init__.py
new file mode 100644
index 0000000..0486261
--- /dev/null
+++ b/project_utils/arguments/__init__.py
@@ -0,0 +1,8 @@
+#!/usr/bin/env python3
+#
+## @file
+# __init__.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> #
+SPDX-License-Identifier: BSD-2-Clause-Patent #
diff --git a/project_utils/arguments/submodule_args.py b/project_utils/arguments/submodule_args.py
new file mode 100644
index 0000000..804ce98
--- /dev/null
+++ b/project_utils/arguments/submodule_args.py
@@ -0,0 +1,24 @@
+#!/usr/bin/env python3
+#
+## @file
+# submodule.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> #
+SPDX-License-Identifier: BSD-2-Clause-Patent #
+
+"""
+Strings for command line arguments.
+"""
+
+SUBMOD_MANIFEST_HELP = 'The current manifest file.'
+SUBMOD_COMBO_HELP = 'The current branch combination in use. If a combo is not specified, the current ' \
+                    'combo will be used.'
+SUBMOD_NEW_MANIFEST_HELP = 'The new manifest file.  Used to test manifest upgrade paths.'
+SUBMOD_NEW_COMBO_HELP = 'The new branch combination to use.  Used to test switching combos.'
+SUBMOD_DEINIT_HELP = 'Performs submodule deinitialization before initializing submodules.'
+SUBMOD_INIT_FULL_HELP = 'Initialize all submodules (recursive).'
+SUBMOD_DEINIT_FULL_HELP = 'Deinitialize all submodules.'
+SUBMOD_WORKSPACE_HELP = 'The project workspace root.  If not specified the current working directory will ' \
+                        'be used.'
+SUBMOD_VERBOSE_HELP = 'Enables verbose messaging.'
diff --git a/project_utils/project_utils_strings.py b/project_utils/project_utils_strings.py
new file mode 100644
index 0000000..33c22d2
--- /dev/null
+++ b/project_utils/project_utils_strings.py
@@ -0,0 +1,24 @@
+#!/usr/bin/env python3
+#
+## @file
+# humble.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> #
+SPDX-License-Identifier: BSD-2-Clause-Patent #
+
+"""
+Contains strings used by the project_utils modules.
+"""
+# Status update messages
+SUBMOD_DEINIT = 'Deinitializing submodules'
+SUBMOD_DEINIT_FULL = 'Deinitializing all submodules'
+SUBMOD_INIT_UPDATE = 'Initializing/Updating submodules'
+SUBMOD_INIT_FULL = 'Initializing/Updating all submodules (recursive)'
+
+# Verbose messages
+SUBMOD_INIT_PATH = 'Submodule init: {}'
+SUBMOD_DEINIT_PATH = 'Submodule deinit: {}'
+SUBMOD_SYNC_PATH = 'Submodule sync: {}'
+SUBMOD_UPDATE_PATH = 'Submodule update: {}'
+SUBMOD_EXCEPTION = '- Exception: {}'
diff --git a/project_utils/submodule.py b/project_utils/submodule.py new file mode 100644 index 0000000..6534f2c
--- /dev/null
+++ b/project_utils/submodule.py
@@ -0,0 +1,389 @@
+#!/usr/bin/env python3
+#
+## @file
+# submodule.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> #
+SPDX-License-Identifier: BSD-2-Clause-Patent # import argparse import 
+git import os import sys import traceback
+
+from edkrepo_manifest_parser.edk_manifest import ManifestXml import 
+project_utils.project_utils_strings as strings import 
+project_utils.arguments.submodule_args as arguments
+
+
+def _init(repo, submodules=None, verbose=False):
+    """
+    Performs submodule initialization.  Note that if no selective submodule initialization
+    is enabled the init will be handled in _update.
+
+    repo       - GitPython Repo object
+    submodules - The list of selective submodule init objects
+    verbose    - Enable verbose messages
+    """
+    # Only handle selective initialization with this method.  For fully recursive init
+    # it is cleaner to do it as part of the update process.
+    if submodules is not None:
+        for sub in submodules:
+            if verbose:
+                print(strings.SUBMOD_INIT_PATH.format(sub.path))
+            output_data = repo.git.execute(['git', 'submodule', 'init', '--', sub.path],
+                                           with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+    return
+
+
+def _deinit(repo, submodules=None, verbose=False):
+    """
+    Performs deinitialization of submodules.
+
+    repo       - GitPython Repo object
+    submodules - The list of selective submodule init objects
+    verbose    - Enable verbose messages
+    """
+    if submodules is None:
+        output_data = repo.git.execute(['git', 'submodule', 'deinit', '--all'],
+                                       with_extended_output=True, with_stdout=True)
+        _display_git_outupt(output_data, verbose)
+    else:
+        for sub in submodules:
+            if verbose:
+                print(strings.SUBMOD_DEINIT_PATH.format(sub.path))
+            output_data = repo.git.execute(['git', 'submodule', 'deinit', '--', sub.path],
+                                           with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+    return
+
+
+def _update(repo, submodules=None, verbose=False, recursive=False):
+    """
+    Performs the update of submodules.  This includes the sync and update operations.
+
+    repo       - GitPython Repo object
+    submodules - The list of selective submodule init objects
+    verbose    - Enable verbose messages
+    recursive  - If submodules is None then use this parameter to determine if initialization
+                 should be recursive
+    """
+    # Now perform the update of the submodules.  For a fully recursive submodule init
+    # this code will update and initialize at the same time.
+    if submodules is None:
+        cmd = ['git', 'submodule', 'sync']
+        if recursive:
+            cmd.append('--recursive')
+        output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+        _display_git_outupt(output_data, verbose)
+        cmd = ['git', 'submodule', 'update', '--init']
+        if recursive:
+            cmd.append('--recursive')
+        output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+        _display_git_outupt(output_data, verbose)
+    else:
+        for sub in submodules:
+            if verbose:
+                print(strings.SUBMOD_SYNC_PATH.format(sub.path))
+            cmd = ['git', 'submodule', 'sync']
+            if sub.recursive:
+                cmd.append('--recursive')
+            cmd.extend(['--', sub.path])
+            output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+            if verbose:
+                print(strings.SUBMOD_UPDATE_PATH.format(sub.path))
+            cmd = ['git', 'submodule', 'update', '--init']
+            if sub.recursive:
+                cmd.append('--recursive')
+            cmd.extend(['--', sub.path])
+            output_data = repo.git.execute(cmd, with_extended_output=True, with_stdout=True)
+            _display_git_outupt(output_data, verbose)
+    return
+

This is a common pattern used for parsing git output throughout EdkRepo I would like to see it placed somewhere where it can be consumed without importing all of the submodule content as well. 
+def _display_git_outupt(output_data, verbose=False):
+    """
+    Displays output from GitPython git commands
+
+    output_data - Output from the git.execute method
+    verbose     - Enable verbose messages
+    """
+    if verbose and output_data[0]:
+        print(output_data[0])
+    if output_data[1]:
+        print(output_data[1])
+    if verbose and output_data[2]:
+        print(output_data[2])
+
+
+def _compute_change(current_subs, new_subs):
+    """
+    Determines the list of submodules that have been removed.  Also needs to
+    determine if any submodules need to have recursive init disabled
+
+    current_subs - List of selective submodule init entries for the current combo
+    new_subs     - List of selective submodule init entries for the new combo
+    """
+    # Create data objects for determining what submodules need to be deinitialized
+    tmp_current = {x.path: x for x in current_subs}
+    tmp_new = {x.path: x for x in new_subs}
+
+    # Initial deinitialization list
+    deinit_paths = list(set(tmp_current).difference(set(tmp_new)))
+
+    # Check for change in recursive initialization for the specific submodules
+    for path in tmp_new:
+        if path in tmp_new and path in tmp_current:
+            if tmp_current[path].recursive and not tmp_new[path].recursive:
+                deinit_paths.append(path)
+
+    # Create the final list of submodules that need to be deinitialized
+    return [x for x in current_subs if x.path in deinit_paths]
+
+
+def _get_submodule_enable(manifest, remote_name, combo):
+    """
+    Determines if submodules are enabled for the current repo and combo
+
+    manifest    - Manifest object
+    remote_name - The name of the current remote being processed
+    combo       - The current combo name being processed
+    """
+    repo_sources = manifest.get_repo_sources(combo)
+    for source in repo_sources:
+        if source.remote_name == remote_name:
+            return source.enable_submodule
+    return False
+
+
+def _get_submodule_state(remote_name, start_manifest, start_combo, end_manifest=None, end_combo=None):
+    """
+    Determines the state of submodules across manifest and combo changes.
+
+        remote_name    - The name of the current remote being processed.
+        start_manifest - Initial manifest parser object.
+        start_combo    - Initial combo.
+        end_manifest   - The manifest parser object for the project if the manifest is being updated.
+        end_combo      - The combination name at the end of the operation if being modified.
+    """
+    start_subs = start_manifest.get_submodule_init_paths(remote_name, start_combo)
+    start_subs_enabled = _get_submodule_enable(start_manifest, remote_name, start_combo)
+    if end_combo is not None:
+        if end_manifest is not None:
+            end_subs = end_manifest.get_submodule_init_paths(remote_name, end_combo)
+            end_subs_enabled = _get_submodule_enable(end_manifest, remote_name, end_combo)
+        else:
+            end_subs = start_manifest.get_submodule_init_paths(remote_name, end_combo)
+            end_subs_enabled = _get_submodule_enable(start_manifest, remote_name, end_combo)
+    else:
+        if end_manifest is not None:
+            end_subs = end_manifest.get_submodule_init_paths(remote_name, start_combo)
+            end_subs_enabled = _get_submodule_enable(end_manifest, remote_name, start_combo)
+        else:
+            end_subs = start_subs
+            end_subs_enabled = start_subs_enabled
+    return start_subs, start_subs_enabled, end_subs, end_subs_enabled
+
+
+def deinit_full(workspace, manifest, verbose=False):
+    """
+    Does full submodule deinit based on the current combo.
+
+        workspace - Path to the current workspace.
+        manifest  - The current manifest parser object.
+    """
+    print(strings.SUBMOD_DEINIT_FULL)
+    current_combo = manifest.general_config.current_combo
+    repo_sources = manifest.get_repo_sources(current_combo)
+    for source in repo_sources:
+        if _get_submodule_enable(manifest, source.remote_name, current_combo):
+            # Open the repo and process submodules
+            try:
+                repo = git.Repo(os.path.join(workspace, source.root))
+            except Exception as repo_error:
+                if args.verbose:
+                    print(strings.SUBMOD_EXCEPTION.format(repo_error))
+                continue
+            _deinit(repo, None, verbose)
+
+
+def init_full(workspace, manifest, verbose=False):
+    """
+    Does full submodule init based on the current combo.
+
+        workspace - Path to the current workspace.
+        manifest  - The current manifest parser object.
+    """
+    print(strings.SUBMOD_INIT_FULL)
+    current_combo = manifest.general_config.current_combo
+    repo_sources = manifest.get_repo_sources(current_combo)
+    for source in repo_sources:
+        if _get_submodule_enable(manifest, source.remote_name, current_combo):
+            # Open the repo and process submodules
+            try:
+                repo = git.Repo(os.path.join(workspace, source.root))
+            except Exception as repo_error:
+                if args.verbose:
+                    print(strings.SUBMOD_EXCEPTION.format(repo_error))
+                continue
+            _update(repo, None, verbose, True)
+
+
+def deinit_submodules(workspace, start_manifest, start_combo,
+                      end_manifest=None, end_combo=None,
+                      verbose=False):
+    """
+    Deinitializes the submodules for a project.
+
+        workspace      - Path to the current workspace.
+        start_manifest - The manifest parser object for the project.
+        start_combo    - The combination name at the start of the operation.
+        end_manifest   - The manifest parser object for the project if the manifest is being updated.  If the
+                         manifest file is not being updated us the value None.
+        end_combo      - The combination name at the end of the operation.  If the combo will not change
+                         use the value None.
+        verbose        - Enable verbose messages.
+    """
+    # Process each repo that may have submodules enabled
+    print(strings.SUBMOD_DEINIT)
+    repo_sources = start_manifest.get_repo_sources(start_combo)
+    for source in repo_sources:
+        # Open the repo and process submodules
+        try:
+            repo = git.Repo(os.path.join(workspace, source.root))
+        except Exception as repo_error:
+            if args.verbose:
+                print(strings.SUBMOD_EXCEPTION.format(repo_error))
+            continue
+
+        # Collect the submodule initialization data from manifest as well as if submodules
+        # should be processed for the repo.
+        start_subs, start_subs_enabled, end_subs, end_subs_enabled = _get_submodule_state(source.remote_name,
+                                                                                          start_manifest,
+                                                                                          start_combo,
+                                                                                          end_manifest,
+                                                                                          end_combo)
+        if not start_subs_enabled and not end_subs_enabled:
+            # At this point submodules are not enabled on this repo
+            continue
+
+        # Compute the list of submodules that need to be removed and added
+        deinit_list = _compute_change(start_subs, end_subs)
+
+        # Deinitialize submodules
+        if (start_subs_enabled and not end_subs_enabled) or (len(start_subs) == 0 and len(end_subs) != 0):
+            # Submodules are being disabled for the entire repo so do a
+            # full deinit on the repo.
+            _deinit(repo, None, verbose)
+        else:
+            # Do the deinit based on the list
+            _deinit(repo, deinit_list, verbose)
+
+

The comments mention needing to use the start combo/end combo parameters but I only see one combo_name parameter in the definition of this function. Additionally  init_submodules implies that this will initialize them not update them as described in the comment.
+def init_submodules(workspace, manifest, combo_name, verbose=False):
+    """
+    Updates the submodules for a specific repo.  Note that operations like checkout will need to use
+    the start_combo and end_combo parameter.  Operations like clone will just need to use the
+    start_combo parameter.
+
+        workspace  - Path to the current workspace.
+        manifest   - The manifest parser object for the project.
+        combo_name - The combination name at the start of the operation.
+        verbose    - Enable verbose messages.
+    """
+    # Process each repo that may have submodules enabled
+    print(strings.SUBMOD_INIT_UPDATE)
+    repo_sources = manifest.get_repo_sources(combo_name)
+    for source in repo_sources:
+        # Open the repo and process submodules
+        try:
+            repo = git.Repo(os.path.join(workspace, source.root))
+        except Exception as repo_error:
+            if args.verbose:
+                print(strings.SUBMOD_EXCEPTION.format(repo_error))
+            continue
+
+        # Collect the submodule initialization data from manifest as well as if submodules
+        # should be processed for the repo.
+        repo_subs = manifest.get_submodule_init_paths(source.remote_name, combo_name)
+        repo_subs_enabled = _get_submodule_enable(manifest, source.remote_name, combo_name)
+        if not repo_subs_enabled:
+            continue
+
+        # Initialize submodules
+        if len(repo_subs) > 0:
+            _init(repo, repo_subs, verbose)
+
+        # Perform sync/update
+        if len(repo_subs) == 0:
+            _update(repo, None, verbose)
+        else:
+            _update(repo, repo_subs, verbose)
+
+
+if __name__ == '__main__':
+    def parse_args():
+        parser = argparse.ArgumentParser()
+        parser.add_argument('manifest_file', metavar='MANIFEST', help=arguments.SUBMOD_MANIFEST_HELP)
+        parser.add_argument('--combo', default=None, help=arguments.SUBMOD_COMBO_HELP)
+        parser.add_argument('--new-manifest', default=None, help=arguments.SUBMOD_NEW_MANIFEST_HELP)
+        parser.add_argument('--new-combo', default=None, help=arguments.SUBMOD_NEW_COMBO_HELP)
+        parser.add_argument('--deinit', action='store_true', help=arguments.SUBMOD_DEINIT_HELP)
+        parser.add_argument('--init-full', action='store_true', help=arguments.SUBMOD_INIT_FULL_HELP)
+        parser.add_argument('--deinit-full', action='store_true', help=arguments.SUBMOD_DEINIT_FULL_HELP)
+        parser.add_argument('--workspace', default='.', help=arguments.SUBMOD_WORKSPACE_HELP)
+        parser.add_argument('--verbose', action='store_true', help=arguments.SUBMOD_VERBOSE_HELP)
+        return parser.parse_args()
+
+    def main(args):
+        # Optional data init
+        new_manifest = None
+        all_new_combos = None
+
+        # Extract basic manifest data
+        manifest = ManifestXml(args.manifest_file)
+        init_manifest = manifest
+        all_combos = manifest.combinations
+        all_combos.extend(manifest.archived_combinations)
+        if args.new_manifest is not None:
+            new_manifest = ManifestXml(args.new_manifest)
+            all_new_combos = new_manifest.combinations
+            all_new_combos.extend(new_manifest.archived_combinations)
+            init_manifest = new_manifest
+
+        # Determine current and new combo information
+        current_combo = manifest.general_config.current_combo
+        new_combo = args.new_combo
+        for combo in all_combos:
+            if args.combo is not None:
+                if args.combo.lower() == combo.name.lower():
+                    current_combo = combo.name
+            if args.new_combo is not None:
+                if args.new_combo.lower() == combo.name.lower():
+                    new_combo = combo.name
+        init_combo = current_combo
+        if new_combo is not None:
+            init_combo = new_combo
+
+        if args.deinit_full:
+            deinit_full(args.workspace, manifest, args.verbose)
+        elif args.init_full:
+            init_full(args.workspace, manifest, args.verbose)
+        else:
+            if args.deinit:
+                deinit_submodules(args.workspace, manifest, current_combo,
+                                  new_manifest, new_combo, args.verbose)
+            init_submodules(args.workspace, init_manifest, init_combo,
+ args.verbose)
+
+        return 0
+
+    try:
+        args = parse_args()
+        sys.exit(main(args))
+    except Exception:
+        if args.verbose:
+            traceback.print_exc()
+        sys.exit(1)
diff --git a/setup.py b/setup.py
index 5d2f55a..a855000 100755
--- a/setup.py
+++ b/setup.py
@@ -1,27 +1,27 @@
-#!/usr/bin/env python3
-#
-## @file setup.py
-#
-# Copyright (c) 2017 - 2020, Intel Corporation. All rights reserved.<BR> -# SPDX-License-Identifier: BSD-2-Clause-Patent -#
-
-from setuptools import setup
-
-setup(name='edkrepo',
-      version='2.0.0',
-      description='The edkrepo tools',
-      packages=['edkrepo', 'edkrepo.commands', 'edkrepo.commands.arguments', 'edkrepo.commands.humble',
+#!/usr/bin/env python3
+#
+## @file setup.py
+#
+# Copyright (c) 2017 - 2020, Intel Corporation. All rights 
+reserved.<BR> # SPDX-License-Identifier: BSD-2-Clause-Patent #
+
+from setuptools import setup
+
+setup(name='edkrepo',
+      version='2.0.0',
+      description='The edkrepo tools',
+      packages=['edkrepo', 'edkrepo.commands', 
+'edkrepo.commands.arguments', 'edkrepo.commands.humble',
                 'edkrepo.git_automation', 'edkrepo.common', 'edkrepo.common.workspace_maintenance',
                 'edkrepo.common.workspace_maintenance.humble', 'edkrepo.config', 'edkrepo.config.humble',
-                'edkrepo_manifest_parser', 'project_utils'],
-      package_data={
-         },
-      include_package_data=True,
-      entry_points={
-          'console_scripts': [
-              'edkrepo = edkrepo.edkrepo_entry_point:main',
-              'command_completion_edkrepo = edkrepo.command_completion_edkrepo:main'
-              ]
-          }
-      )
+                'edkrepo_manifest_parser', 'project_utils', 'project_utils.arguments'],
+      package_data={
+         },
+      include_package_data=True,
+      entry_points={
+          'console_scripts': [
+              'edkrepo = edkrepo.edkrepo_entry_point:main',
+              'command_completion_edkrepo = edkrepo.command_completion_edkrepo:main'
+              ]
+          }
+      )
--
2.21.0.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-05-19 17:46 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-17  1:29 [edk2-staging/EdkRepo] [PATCH v1 0/2] Enabling selective submodule initialization Bjorge, Erik C
2020-05-17  1:29 ` [edk2-staging/EdkRepo] [PATCH v1 1/2] EdkRepo: Adding selective submodule init script Bjorge, Erik C
2020-05-19  2:51   ` Ashley E Desimone
2020-05-19 17:46     ` Bjorge, Erik C
2020-05-17  1:29 ` [edk2-staging/EdkRepo] [PATCH v1 2/2] EdkRepo: Update commands to use new submodule code Bjorge, Erik C
2020-05-19  3:01   ` Ashley E Desimone
2020-05-19  3:13     ` Bjorge, Erik C
2020-05-19 16:45       ` Ashley E Desimone

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox