Repository: zephyrproject-rtos/west

Issue:
Create a command line interface for dealing with manifests
We need a way to deal with the west manifest from the command line. At the very least, this needs to cover creating a "frozen" or "pinned" manifest file which nails down the revisions of west and every project.

The interface would look like this:

```
west manifest <operation> [operation-arguments]
```

The `<operation>` argument would be a mutually exclusive group of options, to at least include `--freeze`.

So you could write:

```
west manifest --freeze [more --freeze-specific options]
```

(Using `--` style options in a mutually exclusive group instead of yet another subparser makes the argparse configuration easier to deal with.)

If we had another hypothetical operation `--foo`, you could write:

```
west manifest --foo [more --foo-specific options]
```

But this would be an error, since `--freeze` and `--foo` are mutually exclusive:

```
west manifest --freeze --foo
```


Discussion:
mbolivar: @carlescufi upon investigation, ruamel.yaml is undergoing a major API breakage. I chose to keep using PyYAML for now as a result. When they are done with their big refactor, we can revisit switching to it. In the meantime, PyYAML is stable. Hope that works for you too.
mbolivar: > I think we should try to move the yaml part into Manifest class, so that reading and freezing / writing of yaml files are located together.

If I understand what you're saying, then a problem with that approach is that that would require the manifest class to know how to run Git to freeze the revisions. That felt like an abstraction violation to me, which is why I chose to do it this way. Do you think we should move knowledge of git in to the manifest library? If so, I am open to it, but that seems like a bigger change that we might want to defer until after LTS.

> or will that require too much modding of yaml ?

I will take another look at the PyYAML dumping APIs and see what I can do.


tejlmand: > Do you think we should move knowledge of git in to the manifest library?

No, the `git` knowledge belongs in the `project.py` imo.
What we can do, is to update a `sha` value in each project, and when calling `freeze` on the manifest, then manifest class can loop through projects, and write that sha.

Today, the project class has the field: `revision`, which is what we use for `west update`, what we could do is to extend the class, to also hold a `sha` field, that can be set in `project.py` and then use that field when doing freeze.

But let's do that for 1.15.
mbolivar: > Today, the project class has the field: `revision`, which is what we use for `west update`, what we could do is to extend the class, to also hold a `sha` field, that can be set in `project.py` and then use that field when doing freeze.

My main concern about this approach is that it prevents programs which want to import west classes as library modules from using the `west.manifest` interface to access SHAs.  I think this is a valid use case, and I have always thought that project.py put too much "knowledge" into a commands module which could be better factored out into a reusable library, the same way the runners and `west build` do.

Let's keep discussing.
tejlmand: > Let's keep discussing.

Yep, created this: #191 to follow up.
tejlmand: I pushed an additional commit earlier today, however your forced push destroyed it.
And I see you did the change that was discussed on slack.

I made rebased my additional commit onto this PR, but to avoid problems, I created PR #192 .
Feel free to select the PR your prefer for now.
mbolivar: > I see you did the change that was discussed on slack, unfortunately I had also created an additional commit with a fix, unfortunately your forced push destroyed it. (that happens ;) )

Ah, whoops -- I'm not really used to that workflow and hadn't seen that you had updated this PR. Sorry for the misunderstanding. I have a few comments on the new patch in 192, so since you are OK with this one (189), let's get that merged and work on that patch ("west: West selfupdate to use same update mechanism as west update") on its own.

Patch:
diff --git a/src/west/_bootstrap/main.py b/src/west/_bootstrap/main.py
index 00d9dc1..e9dc9a1 100644
--- a/src/west/_bootstrap/main.py
+++ b/src/west/_bootstrap/main.py
@@ -336,6 +336,11 @@ def clone_west(manifest_file, directory):
     clone('west repository', west_url, west_rev,
           os.path.join(directory, WEST_DIR, WEST))
 
+    # Make sure west has a manifest-rev branch, like any project.
+    subprocess.check_call(['git', 'update-ref', 'refs/heads/manifest-rev',
+                           'HEAD'],
+                          cwd=os.path.join(directory, WEST_DIR, WEST))
+
 
 def hide_file(path):
     '''Ensure path is a hidden file.
diff --git a/src/west/commands/project.py b/src/west/commands/project.py
index dd57ec6..143ec8c 100644
--- a/src/west/commands/project.py
+++ b/src/west/commands/project.py
@@ -10,7 +10,9 @@ import collections
 import os
 import shutil
 import subprocess
+import sys
 import textwrap
+import yaml
 
 from west.config import config
 from west import log
@@ -157,6 +159,87 @@ class List(WestCommand):
             log.inf(result)
 
 
+class ManifestCommand(WestCommand):
+    # The slightly weird naming is to avoid a conflict with
+    # west.manifest.Manifest.
+
+    def __init__(self):
+        super(ManifestCommand, self).__init__(
+            'manifest',
+            'manage the west manifest',
+            _wrap('''
+            Manages the west manifest.
+
+            Currently, only one operation on the manifest is
+            implemented, namely --freeze. This outputs the manifest
+            with all project-related values fully specified: defaults
+            are applied, and all revisions are converted to SHAs based
+            on the current manifest-rev branches.'''),
+            accepts_unknown_args=False)
+
+    def do_add_parser(self, parser_adder):
+        parser = parser_adder.add_parser(
+            self.name,
+            help=self.help,
+            description=self.description)
+
+        group = parser.add_mutually_exclusive_group(required=True)
+        group.add_argument('--freeze', action='store_true',
+                           help='emit a manifest with SHAs for each revision')
+
+        group = parser.add_argument_group('options for --freeze')
+        group.add_argument('-o', '--out',
+                           help='output file, default is standard output')
+
+        return parser
+
+    def do_run(self, args, user_args):
+        # We assume --freeze here. Future extensions to the group
+        # --freeze is part of can move this code around.
+
+        m = Manifest.from_file()
+
+        # Build a 'frozen' representation of all projects, except the
+        # manifest project.
+        projects = list(m.projects)
+        del projects[MANIFEST_PROJECT_INDEX]
+        frozen_projects = []
+        for project in projects:
+            sha = _sha(project, '{qual_manifest_rev_branch}')
+            d = project.as_dict()
+            d['revision'] = sha
+            frozen_projects.append(d)
+
+        # We include the defaults value here even though all projects
+        # are fully specified in order to make the resulting manifest
+        # easy to extend by users who want to reuse the defaults.
+        o = collections.OrderedDict()
+        o['west'] = m.west_project.as_dict()
+        o['west']['revision'] = _sha(m.west_project,
+                                     '{qual_manifest_rev_branch}')
+        o['manifest'] = collections.OrderedDict()
+        o['manifest']['defaults'] = m.defaults.as_dict()
+        o['manifest']['remotes'] = [r.as_dict() for r in m.remotes]
+        o['manifest']['projects'] = frozen_projects
+        o['manifest']['self'] = m.projects[MANIFEST_PROJECT_INDEX].as_dict()
+
+        # This is a destructive operation, so it's done here to avoid
+        # impacting code which doesn't expect this representer to be
+        # in place.
+        yaml.SafeDumper.add_representer(collections.OrderedDict, self._rep)
+
+        if args.out:
+            with open(args.out, 'w') as f:
+                yaml.safe_dump(o, default_flow_style=False, stream=f)
+        else:
+            yaml.safe_dump(o, default_flow_style=False, stream=sys.stdout)
+
+    def _rep(self, dumper, value):
+        # See https://yaml.org/type/map.html for details on the tag.
+        return util._represent_ordered_dict(dumper, 'tag:yaml.org,2002:map',
+                                            value)
+
+
 class Diff(WestCommand):
     def __init__(self):
         super().__init__(
@@ -852,6 +935,8 @@ def _update_west():
                  "Can't be fast-forwarded to {revision} (from {url}).")
 
         elif old_sha != _sha(project, 'HEAD'):
+            _git(project, 'update-ref {qual_manifest_rev_branch} {revision}')
+
             _inf(project,
                  'Updated {name_and_path} to {revision} (from {url}).')
 
diff --git a/src/west/main.py b/src/west/main.py
index df7684e..90da5df 100755
--- a/src/west/main.py
+++ b/src/west/main.py
@@ -25,14 +25,15 @@ from west import log
 from west import config
 from west.commands import external_commands, \
     CommandError, CommandContextError, ExtensionCommandError
-from west.commands.project import List, Diff, Status, SelfUpdate, ForAll, \
-                             WestUpdated, PostInit, Update
+from west.commands.project import List, ManifestCommand, Diff, Status, \
+    SelfUpdate, ForAll, WestUpdated, PostInit, Update
 from west.manifest import Manifest, MalformedConfig
 from west.util import quote_sh_list
 
 PROJECT_COMMANDS = {
     'commands for managing multiple git repositories': [
         List(),
+        ManifestCommand(),
         Diff(),
         Status(),
         Update(),
diff --git a/src/west/manifest.py b/src/west/manifest.py
index 4187437..8384550 100644
--- a/src/west/manifest.py
+++ b/src/west/manifest.py
@@ -16,6 +16,7 @@ manifest. (I.e. "Remote" represents one of the elements in the
 such as the default project revision, may be supplied by this module
 if they are not present in the manifest data.'''
 
+import collections
 import os
 
 import configparser
@@ -322,6 +323,16 @@ class Defaults:
         return 'Defaults(remote={}, revision={})'.format(repr(self.remote),
                                                          repr(self.revision))
 
+    def as_dict(self):
+        '''Return a representation of this object as a dict, as it would be
+        parsed from an equivalent YAML manifest.'''
+        ret = collections.OrderedDict()
+        if self.remote and isinstance(self.remote, Remote):
+            ret['remote'] = self.remote.name
+        if self.revision:
+            ret['revision'] = self.revision
+        return ret
+
 
 class Remote:
     '''Represents a remote defined in a west manifest.
@@ -350,6 +361,12 @@ class Remote:
         return 'Remote(name={}, url_base={})'.format(repr(self.name),
                                                      repr(self.url_base))
 
+    def as_dict(self):
+        '''Return a representation of this object as a dict, as it would be
+        parsed from an equivalent YAML manifest.'''
+        return collections.OrderedDict(
+            ((s, getattr(self, s)) for s in self.__slots__))
+
 
 class Project:
     '''Represents a project defined in a west manifest.
@@ -402,6 +419,21 @@ class Project:
         return ('Project(name={}, remote={}, url={}, path={}, abspath={}, '
                 'clone_depth={}, revision={})').format(*reprs)
 
+    def as_dict(self):
+        '''Return a representation of this object as a dict, as it would be
+        parsed from an equivalent YAML manifest.'''
+        ret = collections.OrderedDict(
+            (('name', self.name),
+             ('remote', self.remote.name),
+             ('revision', self.revision)))
+        if self.path != self.name:
+            ret['path'] = self.path
+        if self.clone_depth:
+            ret['clone-depth'] = self.clone_depth
+        if self.west_commands:
+            ret['west-commands'] = self.west_commands
+        return ret
+
 
 class SpecialProject(Project):
     '''Represents a special project, e.g. the west or manifest project.
@@ -436,6 +468,17 @@ class SpecialProject(Project):
         self.clone_depth = None
         self.west_commands = west_commands
 
+    def as_dict(self):
+        if self.name == 'west':
+            return collections.OrderedDict((('url', self.url),
+                                            ('revision', self.revision)))
+        else:
+            # Manifest project is assumed.
+            ret = collections.OrderedDict({'path': self.path})
+            if self.west_commands:
+                ret['west-commands'] = self.west_commands
+            return ret
+
 def _wrn_if_not_remote(remote):
     if not isinstance(remote, Remote):
         log.wrn('Remote', remote, 'is not a Remote instance')
diff --git a/src/west/util.py b/src/west/util.py
index df06bf9..34b758c 100644
--- a/src/west/util.py
+++ b/src/west/util.py
@@ -2,6 +2,36 @@
 #
 # SPDX-License-Identifier: Apache-2.0
 
+# Some code (the PyYAML representer for an OrderedDict) is adapted
+# from the PyYAML source code, which is MIT Licensed:
+#
+# SPDX-License-Identifier: MIT
+#
+# Full license text from the PyYAML repository is reproduced below:
+#
+# Copyright (c) 2017-2018 Ingy döt Net
+# Copyright (c) 2006-2016 Kirill Simonov
+#
+# Permission is hereby granted, free of charge, to any person
+# obtaining a copy of this software and associated documentation files
+# (the "Software"), to deal in the Software without restriction,
+# including without limitation the rights to use, copy, modify, merge,
+# publish, distribute, sublicense, and/or sell copies of the Software,
+# and to permit persons to whom the Software is furnished to do so,
+# subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be
+# included in all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+# EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+# MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+# NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+# BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+# ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+# CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+# SOFTWARE.
+
 '''Miscellaneous utilities used by west
 '''
 
@@ -10,6 +40,8 @@ import pathlib
 import shlex
 import textwrap
 
+import yaml
+
 
 def escapes_directory(path, directory):
     '''Returns True if `path` escapes parent directory `directory`.
@@ -84,3 +116,34 @@ def west_topdir(start=None, fall_back=True):
                 raise WestNotFound('Could not find a West installation '
                                    'in this or any parent directory')
         cur_dir = parent_dir
+
+
+def _represent_ordered_dict(dumper, tag, mapping, flow_style=None):
+    # PyYAML representer for ordered dicts. Used internally.
+
+    value = []
+    node = yaml.MappingNode(tag, value, flow_style=flow_style)
+    if dumper.alias_key is not None:
+        dumper.represented_objects[dumper.alias_key] = node
+    best_style = True
+    if hasattr(mapping, 'items'):
+        mapping = list(mapping.items())
+        # The only real difference between
+        # BaseRepresenter.represent_mapping and this function is that
+        # we omit the sort here. Ugh!
+    for item_key, item_value in mapping:
+        node_key = dumper.represent_data(item_key)
+        node_value = dumper.represent_data(item_value)
+        if not (isinstance(node_key, yaml.ScalarNode) and
+                not node_key.style):
+            best_style = False
+        if not (isinstance(node_value, yaml.ScalarNode) and
+                not node_value.style):
+            best_style = False
+        value.append((node_key, node_value))
+    if flow_style is None:
+        if dumper.default_flow_style is not None:
+            node.flow_style = dumper.default_flow_style
+        else:
+            node.flow_style = best_style
+    return node


Test patch:
diff --git a/tests/west/project/test_project.py b/tests/west/project/test_project.py
index 7138f14..ec50238 100644
--- a/tests/west/project/test_project.py
+++ b/tests/west/project/test_project.py
@@ -1,5 +1,6 @@
 import os
 from os.path import dirname
+import re
 import shlex
 import shutil
 import subprocess
@@ -245,6 +246,43 @@ def test_list(west_update_tmpdir):
         cmd('list NOT_A_PROJECT')
 
 
+def test_manifest_freeze(west_update_tmpdir):
+    # We should be able to freeze manifests.
+    actual = cmd('manifest --freeze').splitlines()
+    # Match the actual output against the expected line by line,
+    # so failing lines can be printed in the test output.
+    #
+    # Since the actual remote URLs and SHAs are not predictable, we
+    # don't match those precisely. However, we do expect the output to
+    # match project order as specified in our manifest, that all
+    # revisions are full 40-character SHAs, and there isn't any random
+    # YAML tag crap.
+    expected_res = ['^west:$',
+                    '^  url: .*$',
+                    '^  revision: [a-f0-9]{40}$',
+                    '^manifest:$',
+                    '^  defaults:$',
+                    '^    remote: test-local$',
+                    '^    revision: master$',
+                    '^  remotes:$',
+                    '^  - name: test-local$',
+                    '^    url_base: .*$',
+                    '^  projects:$',
+                    '^  - name: Kconfiglib$',
+                    '^    remote: test-local$',
+                    '^    revision: [a-f0-9]{40}$',
+                    '^    path: subdir/Kconfiglib$',
+                    '^  - name: net-tools$',
+                    '^    remote: test-local$',
+                    '^    revision: [a-f0-9]{40}$',
+                    '^    west-commands: scripts/west-commands.yml$',
+                    '^  self:$',
+                    '^    path: zephyr$']
+
+    for eline_re, aline in zip(expected_res, actual):
+        assert re.match(eline_re, aline) is not None, (aline, eline_re)
+
+
 def test_diff(west_init_tmpdir):
     # FIXME: Check output