From 5665d6140647484ea82a73325fd92b4a6b44e668 Mon Sep 17 00:00:00 2001 From: John Alex Date: Sun, 8 Jan 2023 20:39:19 +0000 Subject: [PATCH 1/2] Fix broken checkout behavior introduced by PR #172. Finding locally installed optional components was broken both because it did not correctly inspect the status of those components, and because it added components-to-checkout by path instead of by name. Printing status updates also a bit wonky because 'print_progress' was not correctly passed down. Slight update to testing docs: no need to run python 2 and 3 separately, and system tests now exist. --- manic/checkout.py | 4 +++- manic/sourcetree.py | 41 ++++++++++++++++++++++++----------------- test/README.md | 20 +++----------------- 3 files changed, 30 insertions(+), 35 deletions(-) diff --git a/manic/checkout.py b/manic/checkout.py index dff11b6617..a38867ff8a 100755 --- a/manic/checkout.py +++ b/manic/checkout.py @@ -402,8 +402,10 @@ def main(args): for comp in args.components: if comp not in ext_description.keys(): + # Note we can't print out the list of found externals because + # they were filtered in create_externals_description above. fatal_error( - "No component {} found in {}".format( + "No component {} found in {}.".format( comp, args.externals)) source_tree = SourceTree(root_dir, ext_description, svn_ignore_ancestry=args.svn_ignore_ancestry) diff --git a/manic/sourcetree.py b/manic/sourcetree.py index 3ab9d014c9..b041978f5a 100644 --- a/manic/sourcetree.py +++ b/manic/sourcetree.py @@ -1,5 +1,5 @@ """ - +0;10;1c FIXME(bja, 2017-11) External and SourceTree have a circular dependancy! """ @@ -98,12 +98,14 @@ def get_local_path(self): """ return self._local_path - def status(self, force=False): + def status(self, force=False, print_progress=False): """ - Returns status of this component and all subcomponents (if available). + Returns status of this component and all subcomponents. - Returns a dict mapping our local path to an ExternalStatus dict. Any - subcomponents will have their own top-level key. + Returns a dict mapping our local path (not component name!) to an + ExternalStatus dict. Any subcomponents will have their own top-level + path keys. Note the return value includes entries for this and all + subcomponents regardless of whether they are locally installed or not. Side-effect: If self._stat is empty or force is True, calculates _stat. """ @@ -151,7 +153,7 @@ def status(self, force=False): # SourceTree.status() expects to be called from the correct # root directory. os.chdir(self._repo_dir_path) - subcomponent_stats = self._externals_sourcetree.status(self._local_path, force=force) + subcomponent_stats = self._externals_sourcetree.status(self._local_path, force=force, print_progress=print_progress) os.chdir(cwd) # Merge our status + subcomponent statuses into one return dict keyed @@ -311,19 +313,22 @@ def status(self, relative_path_base=LOCAL_PATH_INDICATOR, force=False, print_progress=False): """Return a dictionary of local path->ExternalStatus. - Note that all traversed components, whether recursive or top-level, have - a top-level key in the returned dictionary. - - Note that all components that are checked out locally, whether required or - optional, ar included in the returned status. - """ + Notes about the returned dictionary: + * It is keyed by local path (e.g. 'components/mom'), not by + component name (e.g. 'mom'). + * It contains top-level keys for all traversed components, whether + discovered by recursion or top-level. + * It contains entries for all components regardless of whether they + are locally installed or not, or required or optional. +x """ load_comps = self._all_components.keys() summary = {} # Holds merged statuses from all components. for comp in load_comps: if print_progress: printlog('{0}, '.format(comp), end='') - stat = self._all_components[comp].status(force=force) + stat = self._all_components[comp].status(force=force, + print_progress=print_progress) # Returned status dictionary is keyed by local path; prepend # relative_path_base if not already there. @@ -342,14 +347,16 @@ def status(self, relative_path_base=LOCAL_PATH_INDICATOR, def _find_installed_optional_components(self): """Returns a list of installed optional component names, if any.""" - installed_comps = set() + installed_comps = [] for comp_name, ext in self._all_components.items(): if comp_name in self._required_compnames: continue # Note that in practice we expect this status to be cached. - stat = ext.status() - installed_comps.update(stat.keys()) - return list(installed_comps) + path_to_stat = ext.status() + if any(stat.sync_state != ExternalStatus.EMPTY + for stat in path_to_stat.values()): + installed_comps.append(comp_name) + return installed_comps def checkout(self, verbosity, load_all, load_comp=None): """ diff --git a/test/README.md b/test/README.md index 938a900eec..700bccf62c 100644 --- a/test/README.md +++ b/test/README.md @@ -18,28 +18,18 @@ Development environments should be setup for python2 and python3: ## Unit tests -Tests should be run for both python2 and python3. It is recommended -that you have seperate terminal windows open python2 and python3 -testing to avoid errors activating and deactivating environments. - ```SH cd checkout_externals/test - . env_python2/bin/activate make utest - deactivate ``` +## System tests + ```SH cd checkout_externals/test - . env_python2/bin/activate - make utest - deactivate + make stest ``` -## System tests - -Not yet implemented. - ## Static analysis checkout_externals is difficult to test thoroughly because it relies @@ -51,9 +41,7 @@ regularly for automatic code formatting and linting. ```SH cd checkout_externals/test - . env_python2/bin/activate make lint - deactivate ``` The canonical formatting for the code is whatever autopep8 @@ -68,10 +56,8 @@ coverage, run the code coverage tool: ```SH cd checkout_externals/test - . env_python2/bin/activate make coverage open -a Firefox.app htmlcov/index.html - deactivate ``` From cfe0f888a2586f29939ed5123677f6f48804fefc Mon Sep 17 00:00:00 2001 From: John Alex Date: Mon, 9 Jan 2023 01:36:34 +0000 Subject: [PATCH 2/2] fix typos --- manic/checkout.py | 2 +- manic/sourcetree.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/manic/checkout.py b/manic/checkout.py index a38867ff8a..ac30f3a5d2 100755 --- a/manic/checkout.py +++ b/manic/checkout.py @@ -405,7 +405,7 @@ def main(args): # Note we can't print out the list of found externals because # they were filtered in create_externals_description above. fatal_error( - "No component {} found in {}.".format( + "No component {} found in {}".format( comp, args.externals)) source_tree = SourceTree(root_dir, ext_description, svn_ignore_ancestry=args.svn_ignore_ancestry) diff --git a/manic/sourcetree.py b/manic/sourcetree.py index b041978f5a..218b5febb4 100644 --- a/manic/sourcetree.py +++ b/manic/sourcetree.py @@ -1,5 +1,5 @@ """ -0;10;1c + FIXME(bja, 2017-11) External and SourceTree have a circular dependancy! """