From 37d367816349270e4842864cd8883efdadea682e Mon Sep 17 00:00:00 2001 From: Matthew Carlson Date: Fri, 23 Oct 2020 13:08:54 -0700 Subject: [PATCH] Refactor override system in SDE to align with usage and clarify edge cases (#220) --- docs/features/feature_sde.md | 9 +- .../self_describing_environment.py | 151 ++++++++++-------- .../tests/test_self_describing_environment.py | 38 ++++- 3 files changed, 122 insertions(+), 76 deletions(-) diff --git a/docs/features/feature_sde.md b/docs/features/feature_sde.md index dd51074e..ec42f204 100644 --- a/docs/features/feature_sde.md +++ b/docs/features/feature_sde.md @@ -94,12 +94,15 @@ The following path_env fields are optional or conditional: - id - Part of the Override System, this field defines the name that this file will be referred to as by any override_id fields. + It is an error for there to be multiple files with the same id in the same set of scopes. + The SDE will throw an exception in this case. + You are welcome to have the same id if they're on separate scopes that won't overlap. - override_id - - This file will override any descriptor files found in lower lexical order or scope order. Files are traversed in - directory order (depth first) and then scope order (highest to lowest). Overrides are only applied forward in the - traversal, not backwards. + - This file will override any descriptor files. + Override can apply to any type of descriptor (a path env can override an ext_dep for example). + If two descriptors override the same file, this is considered an error and the SDE will throw an exception. ## The Belly of the Beast diff --git a/edk2toolext/environment/self_describing_environment.py b/edk2toolext/environment/self_describing_environment.py index 71cc7eb7..2d7b8f40 100644 --- a/edk2toolext/environment/self_describing_environment.py +++ b/edk2toolext/environment/self_describing_environment.py @@ -69,81 +69,90 @@ def load_workspace(self): logging.debug("Loading workspace: %s" % self.workspace) logging.debug(" Including scopes: %s" % ', '.join(self.scopes)) - # # First, we need to get all of the files that describe our environment. - # - env_files = self._gather_env_files( - ('path_env', 'ext_dep', 'plug_in'), self.workspace) + env_files = self._gather_env_files(('path_env', 'ext_dep', 'plug_in'), self.workspace) + + # Next, get a list of all our scopes + all_scopes_lower = [x.lower() for x in self.scopes] - # # Now that the files have been found, load them, sort them, and filter them # so they can be applied to the environment. - # - def _sort_and_filter_descriptors(class_type, file_list, scopes): - all_descriptors = tuple(class_type( - desc_file).descriptor_contents for desc_file in file_list) - - known_ids = {} - active_overrides = {} - final_list = [] - - for scope in scopes: - for descriptor in all_descriptors: - # If this descriptor isn't in the current scope, we can ignore it for now. - if descriptor['scope'].lower() != scope.lower(): - continue - - cur_file = descriptor['descriptor_file'] - - # If this descriptor has an ID, we need to check for overrides and collisions. - if 'id' in descriptor: - cur_id = descriptor['id'].lower() - - # First, check for overrides. There's no reason to process this file if it's being overridden. - if cur_id in active_overrides: - logging.debug("Descriptor '%s' is being overridden by descriptor '%s' based on ID '%s'." % ( - cur_file, active_overrides[cur_id], cur_id)) - continue - - # Next, check for ID collisions. - if cur_id in known_ids: - raise RuntimeError( - "Descriptor '%s' shares the same ID '%s' with descriptor '%s'." % ( - cur_file, cur_id, known_ids[cur_id]) - ) - - # Finally, we can add this file to the known IDs list. - known_ids[cur_id] = cur_file - - # If we're still processing, we can add this descriptor to the output. - logging.debug("Adding descriptor '%s' to the environment with scope '%s'." % ( - cur_file, scope)) - final_list.append(descriptor) - - # Finally, check to see whether this descriptor overrides anything else. - if 'override_id' in descriptor: - cur_override_id = descriptor['override_id'].lower() - # If we're attempting to override someting that's already been processed, - # we should spit out a warning of sort. - if cur_override_id in known_ids: - logging.warning("Descriptor '%s' is trying to override iID '%s', " - "but it's already been processed." % - (cur_file, cur_override_id)) - active_overrides[cur_override_id] = descriptor['descriptor_file'] - - return tuple(final_list) - - if 'path_env' in env_files: - self.paths = _sort_and_filter_descriptors( - EDF.PathEnvDescriptor, env_files['path_env'], self.scopes) - - if 'ext_dep' in env_files: - self.extdeps = _sort_and_filter_descriptors( - EDF.ExternDepDescriptor, env_files['ext_dep'], self.scopes) - - if 'plug_in' in env_files: - self.plugins = _sort_and_filter_descriptors( - EDF.PluginDescriptor, env_files['plug_in'], self.scopes) + + # We need to convert them from files to descriptors + all_descriptors = list() + + # helper function to get all the descriptors of a type and cast them + def _get_all_descriptors_of_type(key, class_type): + if key not in env_files: + return tuple() + return tuple(class_type(desc_file) for desc_file in env_files[key]) + + # Collect all the descriptors of each type + all_descriptors.extend(_get_all_descriptors_of_type('path_env', EDF.PathEnvDescriptor)) + all_descriptors.extend(_get_all_descriptors_of_type('ext_dep', EDF.ExternDepDescriptor)) + all_descriptors.extend(_get_all_descriptors_of_type('plug_in', EDF.PluginDescriptor)) + + # Get the properly scoped descriptors by checking if the scope is in the list of all the scopes + scoped_desc_gen = [x for x in all_descriptors if x.descriptor_contents['scope'].lower() in all_scopes_lower] + scoped_descriptors = list(scoped_desc_gen) + + # Check that each found item has a unique ID, that's an error if it isn't + allids_gen = [x.descriptor_contents['id'].lower() for x in scoped_descriptors if 'id' in x.descriptor_contents] + all_ids = list(allids_gen) + all_unique_ids = set(all_ids) + if len(all_ids) != len(all_unique_ids): + logging.error("Multiple descriptor files share the same id") + all_unique_id_dict = {} + for desc_id in all_ids: + dict_id_seen = desc_id not in all_unique_id_dict + all_unique_id_dict[desc_id] = 1 if dict_id_seen else all_unique_id_dict[desc_id] + 1 + for desc_id in all_unique_id_dict: + if all_unique_id_dict[desc_id] == 1: + continue + # get the descriptors + desc_of_id = [x for x in scoped_descriptors if x.descriptor_contents['id'].lower() == desc_id] + paths_of_desc_of_id = [x.file_path for x in desc_of_id] + invalid_desc_paths = f"{os.pathsep} ".join(paths_of_desc_of_id) + logging.error(f"Descriptors that have this id {desc_id}: {invalid_desc_paths}") + raise RuntimeError("Multiple descriptor files share the same id") + + # Now check for overrides, first get a list of all the descriptors that have an override id tag + override_descriptors = [x for x in scoped_descriptors if "override_id" in x.descriptor_contents] + active_overrides = {} + for desc in override_descriptors: + override_id = desc.descriptor_contents["override_id"].lower() + # we found this was already overriden, make sure to let the user know + if override_id in active_overrides: + logging.warning("A descriptor file is trying to override a file that was previously overriden") + logging.warning(f"File ID being overriden: {override_id}") + logging.warning(f"Previous override: {active_overrides[override_id].file_path}") + logging.warning(f"New override: {desc.file_path}") + raise RuntimeError(f"Multiple descriptor files share the same override_id: {override_id}") + active_overrides[override_id] = desc + + # Now we filter the overriden id's out and debug to the user whether we are including them or not + overriden_ids = active_overrides.keys() + final_descriptors = [] + for desc in scoped_descriptors: + desc_file = desc.file_path + if 'id' in desc.descriptor_contents: + desc_id = desc.descriptor_contents['id'].lower() + if desc_id in overriden_ids: + override = active_overrides[desc_id] + desc_name = f"{desc_file}:{desc_id}" + override_name = f"{override.file_path}" + logging.debug(f"Skipping descriptor {desc_name} as it is being overridden by {override_name}.") + continue + # add them to the final list + desc_scope = desc.descriptor_contents['scope'] + logging.debug(f"Adding descriptor {desc_file} to the environment with scope {desc_scope}") + final_descriptors.append(desc) + + # Finally, sort them back in the right categories + self.paths = list([x.descriptor_contents for x in final_descriptors if isinstance(x, EDF.PathEnvDescriptor)]) + self.extdeps = list( + [x.descriptor_contents for x in final_descriptors if isinstance(x, EDF.ExternDepDescriptor)]) + self.plugins = list([x.descriptor_contents for x in final_descriptors if isinstance(x, EDF.PluginDescriptor)]) return self diff --git a/edk2toolext/tests/test_self_describing_environment.py b/edk2toolext/tests/test_self_describing_environment.py index 569cc63b..9646b3dc 100644 --- a/edk2toolext/tests/test_self_describing_environment.py +++ b/edk2toolext/tests/test_self_describing_environment.py @@ -43,6 +43,15 @@ def test_collect_path_env(self): build_env, shell_env = self_describing_environment.BootstrapEnvironment(self.workspace, scopes) self.assertEqual(len(build_env.paths), 4) + def test_collect_path_env_scoped(self): + ''' makes sure the SDE can collect path env with the right scopes ''' + scopes = ("global", "testing") + tree = uefi_tree(self.workspace, create_platform=False) + tree.create_path_env("testing_corebuild", scope="testing") + tree.create_path_env("testing_corebuild2", scope="not_valid") + build_env, shell_env = self_describing_environment.BootstrapEnvironment(self.workspace, scopes) + self.assertEqual(len(build_env.paths), 1) + def test_override_path_env(self): ''' checks the SDE descriptor override system ''' custom_scope = "global" @@ -52,8 +61,32 @@ def test_override_path_env(self): tree.create_path_env("testing_corebuild2", var_name="jokes", scope=custom_scope, extra_data={"override_id": "testing_corebuild"}) build_env, shell_env = self_describing_environment.BootstrapEnvironment(self.workspace, scopes) - print(build_env.paths) - print(tree.get_workspace()) + self.assertEqual(len(build_env.paths), 1) + + def test_multiple_override_path_env(self): + ''' checks the SDE descriptor override system will throw an error on multiple overrides''' + custom_scope = "global" + scopes = (custom_scope,) + tree = uefi_tree(self.workspace, create_platform=False) + tree.create_path_env("testing_corebuild", var_name="hey", dir_path="test1", scope=custom_scope) + tree.create_path_env("testing_corebuild2", var_name="jokes", scope=custom_scope, + extra_data={"override_id": "testing_corebuild"}) + tree.create_path_env("testing_corebuild3", var_name="laughs", scope=custom_scope, + extra_data={"override_id": "testing_corebuild"}) + # we should get an exception because we have two overrides + with self.assertRaises(RuntimeError): + build_env, shell_env = self_describing_environment.BootstrapEnvironment(self.workspace, scopes) + self.fail() + + def test_override_path_env_swapped_order(self): + ''' checks the SDE descriptor override system with reversed paths so they are discovered in opposite order''' + custom_scope = "global" + scopes = (custom_scope,) + tree = uefi_tree(self.workspace, create_platform=False) + tree.create_path_env("testing_corebuild", var_name="hey", scope=custom_scope) + tree.create_path_env(var_name="jokes", dir_path="test1", scope=custom_scope, + extra_data={"override_id": "testing_corebuild"}) + build_env, shell_env = self_describing_environment.BootstrapEnvironment(self.workspace, scopes) self.assertEqual(len(build_env.paths), 1) def test_duplicate_id_path_env(self): @@ -65,6 +98,7 @@ def test_duplicate_id_path_env(self): tree.create_path_env("testing_corebuild") with self.assertRaises(RuntimeError): self_describing_environment.BootstrapEnvironment(self.workspace, scopes) + self.fail() if __name__ == '__main__':