From 2a27b9c424a882b36b84e8f327dace6d0e734f39 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 1 Sep 2018 17:58:43 +0900 Subject: source.py: Document Source.get_source_fetchers() to return an iterable Also highlight the fact that the plugin can rely on the fetcher's fetch() method getting called before consuming the next item in the list, which is the magick behavior that the git plugin relies on. This is a part of #620 --- buildstream/source.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/buildstream/source.py b/buildstream/source.py index 9f8f4ffdb..2793fbc26 100644 --- a/buildstream/source.py +++ b/buildstream/source.py @@ -380,8 +380,15 @@ class Source(Plugin): is recommended. Returns: - list: A list of SourceFetchers. If SourceFetchers are not supported, - this will be an empty list. + iterable: The Source's SourceFetchers, if any. + + .. note:: + + Implementors can implement this as a generator. + + The :func:`SourceFetcher.fetch() ` + method will be called on the returned fetchers one by one, + before consuming the next fetcher in the list. *Since: 1.2* """ -- cgit v1.2.1 From a6dbf5fca7add4b12852ecf551bdb7137b43b006 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 1 Sep 2018 18:19:50 +0900 Subject: source.py: Fixing docs link formatting to be consistent. --- buildstream/source.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/buildstream/source.py b/buildstream/source.py index 2793fbc26..232325c85 100644 --- a/buildstream/source.py +++ b/buildstream/source.py @@ -290,10 +290,10 @@ class Source(Plugin): Args: ref (simple object): The internal source reference to set, or ``None`` node (dict): The same dictionary which was previously passed - to :func:`~buildstream.source.Source.configure` + to :func:`Plugin.configure() ` - See :func:`~buildstream.source.Source.get_ref` for a discussion on - the *ref* parameter. + See :func:`Source.get_ref() ` + for a discussion on the *ref* parameter. .. note:: @@ -317,8 +317,8 @@ class Source(Plugin): backend store allows one to query for a new ref from a symbolic tracking data without downloading then that is desirable. - See :func:`~buildstream.source.Source.get_ref` for a discussion on - the *ref* parameter. + See :func:`Source.get_ref() ` + for a discussion on the *ref* parameter. """ # Allow a non implementation return None @@ -362,7 +362,7 @@ class Source(Plugin): :class:`.SourceError` Default implementation is to call - :func:`~buildstream.source.Source.stage`. + :func:`Source.stage() `. Implementors overriding this method should assume that *directory* already exists. @@ -450,8 +450,10 @@ class Source(Plugin): def mark_download_url(self, url): """Identifies the URL that this Source uses to download - This must be called during :func:`~buildstream.plugin.Plugin.configure` if - :func:`~buildstream.source.Source.translate_url` is not called. + This must be called during + :func:`Plugin.configure() ` if + :func:`Source.translate_url() ` + is not called. Args: url (str): The url used to download -- cgit v1.2.1 From 62b7ec4595b2dfb67674026c89ed891632680da1 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sat, 1 Sep 2018 18:34:47 +0900 Subject: source.py: Documenting that marking download URLs is mandatory A download URL must be interpreted by the core at `Plugin.configure()` time, even if only employed later on. This is a part of #620 --- buildstream/source.py | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/buildstream/source.py b/buildstream/source.py index 232325c85..15acba732 100644 --- a/buildstream/source.py +++ b/buildstream/source.py @@ -28,6 +28,18 @@ Abstract Methods For loading and configuration purposes, Sources must implement the :ref:`Plugin base class abstract methods `. +.. attention:: + + In order to ensure that all configuration data is processed at + load time, it is important that all URLs have been processed during + :func:`Plugin.configure() `. + + Source implementations *must* either call + :func:`Source.translate_url() ` or + :func:`Source.mark_download_url() ` + for every URL that has been specified in the configuration during + :func:`Plugin.configure() ` + Sources expose the following abstract methods. Unless explicitly mentioned, these methods are mandatory to implement. @@ -149,6 +161,13 @@ class SourceFetcher(): fetching and substituting aliases. *Since: 1.2* + + .. attention:: + + When implementing a SourceFetcher, remember to call + :func:`Source.mark_download_url() ` + for every URL found in the configuration data at + :func:`Plugin.configure() ` time. """ def __init__(self): self.__alias = None @@ -171,7 +190,7 @@ class SourceFetcher(): Implementors should raise :class:`.SourceError` if the there is some network error or if the source reference could not be matched. """ - raise ImplError("Source fetcher '{}' does not implement fetch()".format(type(self))) + raise ImplError("SourceFetcher '{}' does not implement fetch()".format(type(self))) ############################################################# # Public Methods # @@ -421,6 +440,13 @@ class Source(Plugin): Returns: str: The fully qualified url, with aliases resolved + + .. note:: + + This must be called for every URL in the configuration during + :func:`Plugin.configure() ` if + :func:`Source.mark_download_url() ` + is not called. """ # Alias overriding can happen explicitly (by command-line) or # implicitly (the Source being constructed with an __alias_override). @@ -450,14 +476,16 @@ class Source(Plugin): def mark_download_url(self, url): """Identifies the URL that this Source uses to download - This must be called during - :func:`Plugin.configure() ` if - :func:`Source.translate_url() ` - is not called. - Args: url (str): The url used to download + .. note:: + + This must be called for every URL in the configuration during + :func:`Plugin.configure() ` if + :func:`Source.translate_url() ` + is not called. + *Since: 1.2* """ self.__expected_alias = _extract_alias(url) -- cgit v1.2.1 From 4df625c84c8d755631ab818587870e29e5f1e2e9 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sun, 2 Sep 2018 17:49:00 +0900 Subject: plugin.py: Added _configure() and _get_configuring() private APIs Keeps track of whether the plugin is currently being configured. Adjusted Element and Source classes to call _configure() in place of calling configure() directly. This is a part of #620 --- buildstream/element.py | 2 +- buildstream/plugin.py | 27 +++++++++++++++++++++++++++ buildstream/source.py | 2 +- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/buildstream/element.py b/buildstream/element.py index 70e12313c..6a6d1a9b8 100644 --- a/buildstream/element.py +++ b/buildstream/element.py @@ -262,7 +262,7 @@ class Element(Plugin): # Collect the composited element configuration and # ask the element to configure itself. self.__config = self.__extract_config(meta) - self.configure(self.__config) + self._configure(self.__config) # Extract Sandbox config self.__sandbox_config = self.__extract_sandbox_config(meta) diff --git a/buildstream/plugin.py b/buildstream/plugin.py index 836b60834..f57c0e15c 100644 --- a/buildstream/plugin.py +++ b/buildstream/plugin.py @@ -162,6 +162,7 @@ class Plugin(): self.__provenance = provenance # The Provenance information self.__type_tag = type_tag # The type of plugin (element or source) self.__unique_id = _plugin_register(self) # Unique ID + self.__configuring = False # Whether we are currently configuring # Infer the kind identifier modulename = type(self).__module__ @@ -651,7 +652,32 @@ class Plugin(): else: yield log + # _configure(): + # + # Calls configure() for the plugin, this must be called by + # the core instead of configure() directly, so that the + # _get_configuring() state is up to date. + # + # Args: + # node (dict): The loaded configuration dictionary + # + def _configure(self, node): + self.__configuring = True + self.configure(node) + self.__configuring = False + + # _get_configuring(): + # + # Checks whether the plugin is in the middle of having + # its Plugin.configure() method called + # + # Returns: + # (bool): Whether we are currently configuring + def _get_configuring(self): + return self.__configuring + # _preflight(): + # # Calls preflight() for the plugin, and allows generic preflight # checks to be added # @@ -659,6 +685,7 @@ class Plugin(): # SourceError: If it's a Source implementation # ElementError: If it's an Element implementation # ProgramNotFoundError: If a required host tool is not found + # def _preflight(self): self.preflight() diff --git a/buildstream/source.py b/buildstream/source.py index 15acba732..bec01553b 100644 --- a/buildstream/source.py +++ b/buildstream/source.py @@ -247,7 +247,7 @@ class Source(Plugin): self.__config = self.__extract_config(meta) self.__first_pass = meta.first_pass - self.configure(self.__config) + self._configure(self.__config) COMMON_CONFIG_KEYS = ['kind', 'directory'] """Common source config keys -- cgit v1.2.1 From 435dc56ceffcd7ae3824c182a4f4788e07e0b587 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sun, 2 Sep 2018 17:41:56 +0900 Subject: source.py: Added `primary` argument to URL marking APIs The Source must now mention whether the marked or translated URL is "primary" or not. Even when a Source may have multiple URLs, the auxilliary URLs are derived from the primary one, not only is this true for git, but it is mandated by our tracking API which assumes there is a primary URL. This adjusts the `git` source and the test `fetch_source.py` source to behave properly and advertize it's primary URL properly. This is a part of #620 --- buildstream/plugins/sources/git.py | 17 +++++++++--- buildstream/source.py | 36 +++++++++++++++++--------- tests/frontend/project/sources/fetch_source.py | 18 ++++++++++--- 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/buildstream/plugins/sources/git.py b/buildstream/plugins/sources/git.py index 354f80b5d..065870169 100644 --- a/buildstream/plugins/sources/git.py +++ b/buildstream/plugins/sources/git.py @@ -90,13 +90,14 @@ GIT_MODULES = '.gitmodules' # class GitMirror(SourceFetcher): - def __init__(self, source, path, url, ref): + def __init__(self, source, path, url, ref, *, primary=False): super().__init__() self.source = source self.path = path self.url = url self.ref = ref + self.primary = primary self.mirror = os.path.join(source.get_mirror_directory(), utils.url_directory_name(url)) self.mark_download_url(url) @@ -114,7 +115,8 @@ class GitMirror(SourceFetcher): # system configured tmpdir is not on the same partition. # with self.source.tempdir() as tmpdir: - url = self.source.translate_url(self.url, alias_override=alias_override) + url = self.source.translate_url(self.url, alias_override=alias_override, + primary=self.primary) self.source.call([self.source.host_git, 'clone', '--mirror', '-n', url, tmpdir], fail="Failed to clone git repository {}".format(url), fail_temporarily=True) @@ -136,7 +138,9 @@ class GitMirror(SourceFetcher): .format(self.source, url, tmpdir, self.mirror, e)) from e def _fetch(self, alias_override=None): - url = self.source.translate_url(self.url, alias_override=alias_override) + url = self.source.translate_url(self.url, + alias_override=alias_override, + primary=self.primary) if alias_override: remote_name = utils.url_directory_name(alias_override) @@ -294,7 +298,7 @@ class GitSource(Source): self.node_validate(node, config_keys + Source.COMMON_CONFIG_KEYS) self.original_url = self.node_get_member(node, str, 'url') - self.mirror = GitMirror(self, '', self.original_url, ref) + self.mirror = GitMirror(self, '', self.original_url, ref, primary=True) self.tracking = self.node_get_member(node, str, 'track', None) # At this point we now know if the source has a ref and/or a track. @@ -314,6 +318,11 @@ class GitSource(Source): for path, _ in self.node_items(modules): submodule = self.node_get_member(modules, Mapping, path) url = self.node_get_member(submodule, str, 'url', None) + + # Make sure to mark all URLs that are specified in the configuration + if url: + self.mark_download_url(url, primary=False) + self.submodule_overrides[path] = url if 'checkout' in submodule: checkout = self.node_get_member(submodule, bool, 'checkout') diff --git a/buildstream/source.py b/buildstream/source.py index bec01553b..1edabcbf1 100644 --- a/buildstream/source.py +++ b/buildstream/source.py @@ -235,8 +235,10 @@ class Source(Plugin): self.__element_kind = meta.element_kind # The kind of the element owning this source self.__directory = meta.directory # Staging relative directory self.__consistency = Consistency.INCONSISTENT # Cached consistency state + + # The alias_override is only set on a re-instantiated Source self.__alias_override = alias_override # Tuple of alias and its override to use instead - self.__expected_alias = None # A hacky way to store the first alias used + self.__expected_alias = None # The primary alias # FIXME: Reconstruct a MetaSource from a Source instead of storing it. self.__meta = meta # MetaSource stored so we can copy this source later. @@ -430,17 +432,17 @@ class Source(Plugin): os.makedirs(directory, exist_ok=True) return directory - def translate_url(self, url, *, alias_override=None): + def translate_url(self, url, *, alias_override=None, primary=True): """Translates the given url which may be specified with an alias into a fully qualified url. Args: - url (str): A url, which may be using an alias + url (str): A URL, which may be using an alias alias_override (str): Optionally, an URI to override the alias with. (*Since: 1.2*) + primary (bool): Whether this is the primary URL for the source. (*Since: 1.2*) Returns: - str: The fully qualified url, with aliases resolved - + str: The fully qualified URL, with aliases resolved .. note:: This must be called for every URL in the configuration during @@ -448,6 +450,9 @@ class Source(Plugin): :func:`Source.mark_download_url() ` is not called. """ + # Ensure that the download URL is also marked + self.mark_download_url(url, primary=primary) + # Alias overriding can happen explicitly (by command-line) or # implicitly (the Source being constructed with an __alias_override). if alias_override or self.__alias_override: @@ -466,18 +471,15 @@ class Source(Plugin): url = override_url + url_body return url else: - # Sneakily store the alias if it hasn't already been stored - if not self.__expected_alias and url and utils._ALIAS_SEPARATOR in url: - self.mark_download_url(url) - project = self._get_project() return project.translate_url(url, first_pass=self.__first_pass) - def mark_download_url(self, url): + def mark_download_url(self, url, *, primary=True): """Identifies the URL that this Source uses to download Args: - url (str): The url used to download + url (str): The URL used to download + primary (bool): Whether this is the primary URL for the source .. note:: @@ -488,7 +490,17 @@ class Source(Plugin): *Since: 1.2* """ - self.__expected_alias = _extract_alias(url) + # Only mark the Source level aliases on the main instance, not in + # a reinstantiated instance in mirroring. + if not self.__alias_override: + if primary: + expected_alias = _extract_alias(url) + + assert (self.__expected_alias is None or + self.__expected_alias == expected_alias), \ + "Primary URL marked twice with different URLs" + + self.__expected_alias = expected_alias def get_project_directory(self): """Fetch the project base directory diff --git a/tests/frontend/project/sources/fetch_source.py b/tests/frontend/project/sources/fetch_source.py index ebd3fe757..10e89960c 100644 --- a/tests/frontend/project/sources/fetch_source.py +++ b/tests/frontend/project/sources/fetch_source.py @@ -15,14 +15,17 @@ from buildstream import Source, Consistency, SourceError, SourceFetcher class FetchFetcher(SourceFetcher): - def __init__(self, source, url): + def __init__(self, source, url, primary=False): super().__init__() self.source = source self.original_url = url + self.primary = primary self.mark_download_url(url) def fetch(self, alias_override=None): - url = self.source.translate_url(self.original_url, alias_override=alias_override) + url = self.source.translate_url(self.original_url, + alias_override=alias_override, + primary=self.primary) with open(self.source.output_file, "a") as f: success = url in self.source.fetch_succeeds and self.source.fetch_succeeds[url] message = "Fetch {} {} from {}\n".format(self.original_url, @@ -37,12 +40,21 @@ class FetchSource(Source): # Read config to know which URLs to fetch def configure(self, node): self.original_urls = self.node_get_member(node, list, 'urls') - self.fetchers = [FetchFetcher(self, url) for url in self.original_urls] self.output_file = self.node_get_member(node, str, 'output-text') self.fetch_succeeds = {} if 'fetch-succeeds' in node: self.fetch_succeeds = {x[0]: x[1] for x in self.node_items(node['fetch-succeeds'])} + # First URL is the primary one for this test + # + primary = True + self.fetchers = [] + for url in self.original_urls: + self.mark_download_url(url, primary=primary) + fetcher = FetchFetcher(self, url, primary=primary) + self.fetchers.append(fetcher) + primary = False + def get_source_fetchers(self): return self.fetchers -- cgit v1.2.1 From 51eaa2c38fbeee6a34898bec3db048b0aa108696 Mon Sep 17 00:00:00 2001 From: Tristan Van Berkom Date: Sun, 2 Sep 2018 18:32:54 +0900 Subject: source.py: Track marked URLs and assert they are marked during Plugin.configure() This cannot test for unaliased URLs, as those can be discovered later on outside of user provided element configuration; at least we assert that if an alias was used, we have seen it at load time. This will cause a BUG to occur for a plugin which marks an aliased URL (or attempts to translate one) outside of `Plugin.configure()`, if that URL was not previously seen. This is a part of #620 --- buildstream/source.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/buildstream/source.py b/buildstream/source.py index 1edabcbf1..f546258e6 100644 --- a/buildstream/source.py +++ b/buildstream/source.py @@ -239,6 +239,7 @@ class Source(Plugin): # The alias_override is only set on a re-instantiated Source self.__alias_override = alias_override # Tuple of alias and its override to use instead self.__expected_alias = None # The primary alias + self.__marked_urls = set() # Set of marked download URLs # FIXME: Reconstruct a MetaSource from a Source instead of storing it. self.__meta = meta # MetaSource stored so we can copy this source later. @@ -502,6 +503,25 @@ class Source(Plugin): self.__expected_alias = expected_alias + # Enforce proper behaviour of plugins by ensuring that all + # aliased URLs have been marked at Plugin.configure() time. + # + if self._get_configuring(): + # Record marked urls while configuring + # + self.__marked_urls.add(url) + else: + # If an unknown aliased URL is seen after configuring, + # this is an error. + # + # It is still possible that a URL that was not mentioned + # in the element configuration can be marked, this is + # the case for git submodules which might be automatically + # discovered. + # + assert (url in self.__marked_urls or not _extract_alias(url)), \ + "URL was not seen at configure time: {}".format(url) + def get_project_directory(self): """Fetch the project base directory -- cgit v1.2.1