You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by "gtristan (via GitHub)" <gi...@apache.org> on 2024/01/18 05:21:48 UTC
[PR] WIP: Implement plugin mirrors [buildstream]
gtristan opened a new pull request, #1890:
URL: https://github.com/apache/buildstream/pull/1890
This merge request, when complete, implements the proposal outlined on the mailing list:
https://lists.apache.org/thread/oxp2tmvd66wdwo3hzbgpc8x4hvrfyl5w
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1508812480
##########
src/buildstream/source.py:
##########
@@ -696,14 +699,22 @@ def get_mirror_directory(self) -> str:
return self.__mirror_directory
- def translate_url(self, url: str, *, alias_override: Optional[str] = None, primary: bool = True) -> str:
+ def translate_url(
+ self,
+ url: str,
+ *,
+ alias_override: Optional[str] = None,
+ primary: bool = True,
+ extra_data: Optional[Dict[str, Any]] = None,
+ ) -> str:
"""Translates the given url which may be specified with an alias
into a fully qualified url.
Args:
url: A URL, which may be using an alias
alias_override: Optionally, an URI to override the alias with.
primary: Whether this is the primary URL for the source.
+ extra_data: Additional data provided by :class:`SourceMirror <buildstream.sourcemirror.SourceMirror>` (*Since: 2.2*)
Review Comment:
Shall we provide guidance how a source plugin should deal with this. I would say source plugins should fail if the mirror provides a key it doesn't understand. Or is this too restrictive? `DownloadableFileSource` in #1895 currently doesn't fail in that case, though.
##########
src/buildstream/sourcemirror.py:
##########
@@ -0,0 +1,203 @@
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Authors:
+# Tristan Van Berkom <tr...@codethink.co.uk>
+"""
+SourceMirror - Base source mirror class
+=======================================
+The SourceMirror plugin allows one to customize how
+:func:`Source.translate_url() <buildstream.source.Source.translate_url>` will
+behave when looking up mirrors, allowing some additional flexibility in the
+implementation of source mirrors.
+
+
+.. _core_source_mirror_abstract_methods:
+
+Abstract Methods
+----------------
+For loading and configuration purposes, SourceMirrors may optionally implement
+the :func:`Plugin base class Plugin.configure() method <buildstream.plugin.Plugin.configure>`
+in order to load any custom configuration in the `config` dictionary.
+
+The remaining :ref:`Plugin base class abstract methods <core_plugin_abstract_methods>` are
+not relevant to the SourceMirror plugin object and need not be implemented.
+
+Sources expose the following abstract methods. Unless explicitly mentioned,
Review Comment:
```suggestion
SourceMirrors expose the following abstract methods. Unless explicitly mentioned,
```
##########
src/buildstream/sourcemirror.py:
##########
@@ -0,0 +1,203 @@
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Authors:
+# Tristan Van Berkom <tr...@codethink.co.uk>
+"""
+SourceMirror - Base source mirror class
+=======================================
+The SourceMirror plugin allows one to customize how
+:func:`Source.translate_url() <buildstream.source.Source.translate_url>` will
+behave when looking up mirrors, allowing some additional flexibility in the
+implementation of source mirrors.
+
+
+.. _core_source_mirror_abstract_methods:
+
+Abstract Methods
+----------------
+For loading and configuration purposes, SourceMirrors may optionally implement
+the :func:`Plugin base class Plugin.configure() method <buildstream.plugin.Plugin.configure>`
+in order to load any custom configuration in the `config` dictionary.
+
+The remaining :ref:`Plugin base class abstract methods <core_plugin_abstract_methods>` are
+not relevant to the SourceMirror plugin object and need not be implemented.
+
+Sources expose the following abstract methods. Unless explicitly mentioned,
+these methods are mandatory to implement.
+
+* :func:`SourceMirror.translate_url() <buildstream.source.SourceMirror.translate_url>`
+
+ Produce an appropriate URL for the given URL and alias.
+
+
+Class Reference
+---------------
+"""
+
+from typing import Optional, Dict, List, Any, TYPE_CHECKING
+
+from .node import MappingNode, SequenceNode
+from .plugin import Plugin
+from ._exceptions import BstError, LoadError
+from .exceptions import ErrorDomain, LoadErrorReason
+
+if TYPE_CHECKING:
+
+ # pylint: disable=cyclic-import
+ from ._context import Context
+ from ._project import Project
+
+ # pylint: enable=cyclic-import
+
+
+class SourceMirrorError(BstError):
+ """This exception should be raised by :class:`.SourceMirror` implementations
+ to report errors to the user.
+
+ Args:
+ message: The breif error description to report to the user
+ detail: A possibly multiline, more detailed error message
+ reason: An optional machine readable reason string, used for test cases
+
+ *Since: 2.2*
+ """
+
+ def __init__(
+ self, message: str, *, detail: Optional[str] = None, reason: Optional[str] = None, temporary: bool = False
+ ):
+ super().__init__(message, detail=detail, domain=ErrorDomain.SOURCE, reason=reason)
+
+
+class SourceMirror(Plugin):
+ """SourceMirror()
+
+ Base SourceMirror class.
+
+ All SourceMirror plugins derive from this class, this interface defines how
+ the core will be interacting with SourceMirror plugins.
+
+ *Since: 2.2*
+ """
+
+ # The SourceMirror plugin type is only supported since BuildStream 2.2
+ BST_MIN_VERSION = "2.2"
+
+ def __init__(
+ self,
+ context: "Context",
+ project: "Project",
+ node: MappingNode,
+ ):
+ # Note: the MappingNode passed here is already expanded with
+ # the project level base variables, so there is no need
+ # to expand them redundantly here.
+ #
+ node.validate_keys(["name", "kind", "config", "aliases"])
+
+ # Do local base class parsing first
+ name: str = node.get_str("name")
+ self.__aliases: Dict[str, List[str]] = self.__load_aliases(node)
+
+ # Chain up to Plugin
+ super().__init__(name, context, project, node, "source-mirror")
+
+ # Plugin specific parsing
+ config = node.get_mapping("config", default={})
+ self._configure(config)
+
+ ##########################################################
+ # Public API #
+ ##########################################################
+ def translate_url(
+ self,
+ *,
+ project_name: str,
+ alias: str,
+ alias_url: str,
Review Comment:
Could it be sensible to make these two alias parameters optional? They are currently always set but we could consider supporting URL translation without an alias in the future. Making them optional in the signature now may make it easier to support this in the future without breaking backward compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1526427770
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
Just to be clear, this discussion is supposed to be on https://github.com/apache/buildstream/pull/1890#discussion_r1509989058 (I hope you are replying to that, and not to this which is simply pointing out the "weird source mirror plugins"). I've tried to reply earlier, but my comment was detached from this discussion by a github bug (https://github.com/apache/buildstream/pull/1890#discussion_r1523626484).
Anyway, my point is that by forcing plugins to have aliases defined in a specific way, they end up doing weird things to try to work around it. For instance, take my `gitlab_lfs_mirror` plugin from https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/merge_requests/18099. I am doing
```
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
config:
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
cpan: [invalid]
crates: [invalid]
ftp_gnu_org: [invalid]
github_files: [invalid]
oasis_open_xml: [invalid]
pagure_releases: [invalid]
pypi: [invalid]
sourceforge: [invalid]
sourceforge_download: [invalid]
sourceware_pub: [invalid]
tar_https: [invalid]
```
I'd like to be able to do
```
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
config:
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
- cpan
- crates
- ftp_gnu_org
- github_files
- oasis_open_xml
- pagure_releases
- pypi
- sourceforge
- sourceforge_download
- sourceware_pub
- tar_https
```
Or better yet, dropping the `config:`
```
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
- cpan
- crates
- ftp_gnu_org
- github_files
- oasis_open_xml
- pagure_releases
- pypi
- sourceforge
- sourceforge_download
- sourceware_pub
- tar_https
```
Ultimately, the plugin only needs to tell the core the aliases that it can handle and optionally how many URLs it can provide for a given alias. Otherwise, it should be free to define its configuration in the way that makes most sense to what it is doing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1512571456
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
Additional comment about _"needing to set mirror values for aliases"_ in general.
As I already mentioned, yes I am in favor of expanding this later in order to support more generic application of `SourceMirror`, perhaps allowing a project to just declare a single `SourceMirror` object once and allow that to handle all of the url translations.
That said, there is a lot to work out for that, e.g. we have priorities and lists etc in the API to consider.
Finally to answer _that_ question about needing to specify the aliases with at least some _values_ for the aliases in a given mirror, no it will not possibly work without specifying aliases in the patch's current state.
The reason for this is that the current, existing workflow, taking all mirrors and priorities etc into account, needs to run through `Project.get_alias_uris()`, this is where the URI fetch/track policies are considered and this is the code section which controls the priority order of _"each URI which shall be consulted for a given alias"_.
In order to extend `SourceMirror` to apply more generically, this whole concept of _iterating over URIs for a given alias_ needs to be rethought.
Note also, that the _same `SourceMirror`_ may be consulted _multiple times_ should it be given more than a single URL value for a given alias that it declares (which was of course, already the case before introducing this plugin).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1509989058
##########
src/buildstream/sourcemirror.py:
##########
@@ -0,0 +1,203 @@
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Authors:
+# Tristan Van Berkom <tr...@codethink.co.uk>
+"""
+SourceMirror - Base source mirror class
+=======================================
+The SourceMirror plugin allows one to customize how
+:func:`Source.translate_url() <buildstream.source.Source.translate_url>` will
+behave when looking up mirrors, allowing some additional flexibility in the
+implementation of source mirrors.
+
+
+.. _core_source_mirror_abstract_methods:
+
+Abstract Methods
+----------------
+For loading and configuration purposes, SourceMirrors may optionally implement
+the :func:`Plugin base class Plugin.configure() method <buildstream.plugin.Plugin.configure>`
+in order to load any custom configuration in the `config` dictionary.
+
+The remaining :ref:`Plugin base class abstract methods <core_plugin_abstract_methods>` are
+not relevant to the SourceMirror plugin object and need not be implemented.
+
+Sources expose the following abstract methods. Unless explicitly mentioned,
+these methods are mandatory to implement.
+
+* :func:`SourceMirror.translate_url() <buildstream.source.SourceMirror.translate_url>`
+
+ Produce an appropriate URL for the given URL and alias.
+
+
+Class Reference
+---------------
+"""
+
+from typing import Optional, Dict, List, Any, TYPE_CHECKING
+
+from .node import MappingNode, SequenceNode
+from .plugin import Plugin
+from ._exceptions import BstError, LoadError
+from .exceptions import ErrorDomain, LoadErrorReason
+
+if TYPE_CHECKING:
+
+ # pylint: disable=cyclic-import
+ from ._context import Context
+ from ._project import Project
+
+ # pylint: enable=cyclic-import
+
+
+class SourceMirrorError(BstError):
+ """This exception should be raised by :class:`.SourceMirror` implementations
+ to report errors to the user.
+
+ Args:
+ message: The breif error description to report to the user
+ detail: A possibly multiline, more detailed error message
+ reason: An optional machine readable reason string, used for test cases
+
+ *Since: 2.2*
+ """
+
+ def __init__(
+ self, message: str, *, detail: Optional[str] = None, reason: Optional[str] = None, temporary: bool = False
+ ):
+ super().__init__(message, detail=detail, domain=ErrorDomain.SOURCE, reason=reason)
+
+
+class SourceMirror(Plugin):
+ """SourceMirror()
+
+ Base SourceMirror class.
+
+ All SourceMirror plugins derive from this class, this interface defines how
+ the core will be interacting with SourceMirror plugins.
+
+ *Since: 2.2*
+ """
+
+ # The SourceMirror plugin type is only supported since BuildStream 2.2
+ BST_MIN_VERSION = "2.2"
+
+ def __init__(
+ self,
+ context: "Context",
+ project: "Project",
+ node: MappingNode,
+ ):
+ # Note: the MappingNode passed here is already expanded with
+ # the project level base variables, so there is no need
+ # to expand them redundantly here.
+ #
+ node.validate_keys(["name", "kind", "config", "aliases"])
+
+ # Do local base class parsing first
+ name: str = node.get_str("name")
+ self.__aliases: Dict[str, List[str]] = self.__load_aliases(node)
Review Comment:
Can we maybe not make this mandatory? Having this be mandatory gives weird source mirror plugins (like the one in tests).
Can I suggest the following:
* Move this alias logic to a core plugin
* Have the plugin factory check and require that either `kind` or `aliases` is provided, but not both
* if `aliases` is provided, create the default bst implementation (it could be special-cased to get "aliases" instead of "config")
* if `kind` is provided, create the specified plugin.
* Have some API to allow the plugins to request the aliases they can handle (This could probably be extended in the future to handle unaliased urls)
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
^ This is what I am referring to. We shouldn't have to do this when implementing source mirrors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1523626484
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
Just to make sure, did you see my other comment? https://github.com/apache/buildstream/pull/1890#discussion_r1509989058. I feel that you're answering in the wrong thread.
I agree with what you say here, but I want to change it before landing this.
> yes I am in favor of expanding this later in order to support more generic application of SourceMirror
> no it will not possibly work without specifying aliases in the patch's current state.
I'm afraid we can't reconcile these two statements. If we want to expand this later, we shouldn't make such a hard requirement. We need to leave ourselves some space to work with.
##########
src/buildstream/_project.py:
##########
@@ -211,14 +235,10 @@ def source_overrides(self):
# fully qualified urls based on the shorthand which is allowed
# to be specified in the YAML
def translate_url(self, url, *, first_pass=False):
- if first_pass:
- config = self.first_pass_config
- else:
- config = self.config
if url and utils._ALIAS_SEPARATOR in url:
url_alias, url_body = url.split(utils._ALIAS_SEPARATOR, 1)
- alias_url = config._aliases.get_str(url_alias, default=None)
+ alias_url = self.get_alias_url(url_alias)
Review Comment:
```suggestion
alias_url = self.get_alias_url(url_alias, first_pass=first_pass)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1526427770
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
Just to be clear, this discussion is supposed to be on https://github.com/apache/buildstream/pull/1890#discussion_r1509989058 (I hope you are replying to that, and not to this which is simply pointing out the "weird source mirror plugins"). I've tried to reply earlier, but my comment was detached from this discussion by a github bug (https://github.com/apache/buildstream/pull/1890#discussion_r1523626484).
Anyway, my point is that by forcing plugins to have aliases defined in a specific way, they end up doing weird things to try to work around it. For instance, take my gitlab-lfs-mirror plugin from https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/merge_requests/18099. I am doing
```
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
config:
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
cpan: [invalid]
crates: [invalid]
ftp_gnu_org: [invalid]
github_files: [invalid]
oasis_open_xml: [invalid]
pagure_releases: [invalid]
pypi: [invalid]
sourceforge: [invalid]
sourceforge_download: [invalid]
sourceware_pub: [invalid]
tar_https: [invalid]
```
I'd like to be able to do
```
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
config:
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
- cpan
- crates
- ftp_gnu_org
- github_files
- oasis_open_xml
- pagure_releases
- pypi
- sourceforge
- sourceforge_download
- sourceware_pub
- tar_https
```
Or better yet, dropping the `config:`
```
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
- cpan
- crates
- ftp_gnu_org
- github_files
- oasis_open_xml
- pagure_releases
- pypi
- sourceforge
- sourceforge_download
- sourceware_pub
- tar_https
```
Ultimately, the plugin only needs to tell the core the aliases that it can handle and optionally how many URLs it can provide for a given alias. Otherwise, it should be free to define its configuration in the way that makes most sense to what it is doing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1512346658
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
Can it be written any other way? BuildStream won't accept setting aliases to a list, it needs to be a dict. I haven't tested, but I assume it won't let me set the values to an empty list either (or would it?).
The end result is that, as a user, I need to set the `aliases` key to a bogus dictionary where only keys are taken from it, and those keys are repeated in the `config` below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1510855796
##########
src/buildstream/sourcemirror.py:
##########
@@ -0,0 +1,203 @@
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Authors:
+# Tristan Van Berkom <tr...@codethink.co.uk>
+"""
+SourceMirror - Base source mirror class
+=======================================
+The SourceMirror plugin allows one to customize how
+:func:`Source.translate_url() <buildstream.source.Source.translate_url>` will
+behave when looking up mirrors, allowing some additional flexibility in the
+implementation of source mirrors.
+
+
+.. _core_source_mirror_abstract_methods:
+
+Abstract Methods
+----------------
+For loading and configuration purposes, SourceMirrors may optionally implement
+the :func:`Plugin base class Plugin.configure() method <buildstream.plugin.Plugin.configure>`
+in order to load any custom configuration in the `config` dictionary.
+
+The remaining :ref:`Plugin base class abstract methods <core_plugin_abstract_methods>` are
+not relevant to the SourceMirror plugin object and need not be implemented.
+
+Sources expose the following abstract methods. Unless explicitly mentioned,
+these methods are mandatory to implement.
+
+* :func:`SourceMirror.translate_url() <buildstream.source.SourceMirror.translate_url>`
+
+ Produce an appropriate URL for the given URL and alias.
+
+
+Class Reference
+---------------
+"""
+
+from typing import Optional, Dict, List, Any, TYPE_CHECKING
+
+from .node import MappingNode, SequenceNode
+from .plugin import Plugin
+from ._exceptions import BstError, LoadError
+from .exceptions import ErrorDomain, LoadErrorReason
+
+if TYPE_CHECKING:
+
+ # pylint: disable=cyclic-import
+ from ._context import Context
+ from ._project import Project
+
+ # pylint: enable=cyclic-import
+
+
+class SourceMirrorError(BstError):
+ """This exception should be raised by :class:`.SourceMirror` implementations
+ to report errors to the user.
+
+ Args:
+ message: The breif error description to report to the user
+ detail: A possibly multiline, more detailed error message
+ reason: An optional machine readable reason string, used for test cases
+
+ *Since: 2.2*
+ """
+
+ def __init__(
+ self, message: str, *, detail: Optional[str] = None, reason: Optional[str] = None, temporary: bool = False
+ ):
+ super().__init__(message, detail=detail, domain=ErrorDomain.SOURCE, reason=reason)
+
+
+class SourceMirror(Plugin):
+ """SourceMirror()
+
+ Base SourceMirror class.
+
+ All SourceMirror plugins derive from this class, this interface defines how
+ the core will be interacting with SourceMirror plugins.
+
+ *Since: 2.2*
+ """
+
+ # The SourceMirror plugin type is only supported since BuildStream 2.2
+ BST_MIN_VERSION = "2.2"
+
+ def __init__(
+ self,
+ context: "Context",
+ project: "Project",
+ node: MappingNode,
+ ):
+ # Note: the MappingNode passed here is already expanded with
+ # the project level base variables, so there is no need
+ # to expand them redundantly here.
+ #
+ node.validate_keys(["name", "kind", "config", "aliases"])
+
+ # Do local base class parsing first
+ name: str = node.get_str("name")
+ self.__aliases: Dict[str, List[str]] = self.__load_aliases(node)
+
+ # Chain up to Plugin
+ super().__init__(name, context, project, node, "source-mirror")
+
+ # Plugin specific parsing
+ config = node.get_mapping("config", default={})
+ self._configure(config)
+
+ ##########################################################
+ # Public API #
+ ##########################################################
+ def translate_url(
+ self,
+ *,
+ project_name: str,
+ alias: str,
+ alias_url: str,
Review Comment:
I actually spent some time inquiring on this.
Having optional keyword parameters does not work with normal python, unless the plugin author uses `**kwargs` syntax to declare the function, this won't be very pretty and have nice type annotations.
If we instruct the plugin author to _only_ ever declare the function with `**kwargs` rather than explicitly typing out the parameters, then we can have functions with extensible keyword arguments in the API without breaking API.
Alternatively, we can have some internal monstrosity which observes the python function in memory only only feeds the function with parameters it is prepared to accept, which might look more pretty in documentation but wont have any sensible type checking with mypy either.
Keyword arguments are nice and extensible for us when we _provide_ a function that plugins _may call_, but not the other way around, we could go ahead with instructing plugin authors to use a `def function(self, **kwargs)` only, if we want extensibility, I'm not opposed to that but it does deviate from our current API style.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1527095613
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
I understand what you're saying now.
I would have rather expected:
```yaml
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
aliases:
cpan: https://gitlab.com/freedesktop-sdk/mirrors/{alias}/raw-files
crates: https://gitlab.com/freedesktop-sdk/mirrors/{alias}/raw-files
ftp_gnu_org: https://gitlab.com/freedesktop-sdk/mirrors/{alias}/raw-files
...
```
I.e. we're still expecting the existing mirror configuration to be valuable, and used in the default `SourceMirror` implementation (base class).
There are two paths I can see towards supporting this.
## Allow list-or-dict declaration of the aliases
In your example:
```yaml
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
- cpan
- crates
- ftp_gnu_org
- github_files
```
This could be supported on the condition that the mirror is a user plugin, and would result in passing `None` values for the configured URI passed through `SourceMirror.translate_url()`.
## Support non aliased URIs
In your example:
```yaml
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
config:
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
- cpan
- crates
- ftp_gnu_org
- github_files
```
This will require restructuring the `Source.__do_fetch()` and `Source.__do_track()` loops and is a bit more involved.
I.e. we would want a mirror to be allowed to apply to _"any alias"_ and/or _"no alias"_.
For the _"no alias"_ case it is a bit more clear cut, afaics there are specific code blocks where we simply do not as the `Project` about the urls for aliases and directly invoke the plugin implementation.
### For plugins who want to support _"any alias"_
For the _"any alias"_ case, maybe we would want a `None` URL with a `SourceMirror` tuple to be returned in the list from `Project.get_alias_uris()` (allowing the project to continue to respect the user configuration of priorities).
Both cases will require that `Source.__clone_for_uri()` work with a `None` uri and/or a `None` alias.
The last part, for blanket mirrors to not need to specify the alias/[urls], it would make sense for `Source.translate_url()` to be allowed to _"not work"_ when given unrecognized aliases.
This could allow specification of multiple SourceMirror plugins which do things in different ways, and can bail out on aliases/urls which that plugin does not support, also it allows falling back to other mirrors or original project alias if the user has configured `SourceUriPolicy.ALL` and the mirror failed to produce a URI.
This is tricky to accomplish with the current code structure though. At fetch/track time it could work by raising an exception, allowing the loop to continue when the SourceMirror does:
```python
def translate_url(self, ... alias ...):
if i_dont_know_this(alias):
raise SourceMirrorUnhandled()
```
This would abort the plugin code calling into `Source.translate_url()` without breaking existing plugin code, as long as mirrors are never used in `Plugin.configure()` time and only at `Source.fetch()` and `Source.track()` time (we would only end up throwing away cloned Sources in the case that a URI could not be produced, and move on to the next mirror/uri).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1512142924
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
Just to clarify, this is not _necessary_ to do per se, while declaring aliases is still a requirement which may be lifted later in order to provide mirrors more generically without specific alias targeting.
To be clear, it is written this way in the _test case_ only to demonstrate that it is the `SourceMirror` in action, retrieving the information required to complete the URL in it's own plugin configuration rather than in the base mirror configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim closed pull request #1890: Implement plugin mirrors
URL: https://github.com/apache/buildstream/pull/1890
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1527095613
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
I understand what you're saying now.
I would have rather expected:
```yaml
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
aliases:
cpan: [https://gitlab.com/freedesktop-sdk/mirrors/{alias}/raw-files]
crates: [https://gitlab.com/freedesktop-sdk/mirrors/{alias}/raw-files]
ftp_gnu_org: [https://gitlab.com/freedesktop-sdk/mirrors/{alias}/raw-files]
...
```
I.e. we're still expecting the existing mirror configuration to be valuable, and used in the default `SourceMirror` implementation (base class).
There are two paths I can see towards supporting this.
## Allow list-or-dict declaration of the aliases
In your example:
```yaml
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
- cpan
- crates
- ftp_gnu_org
- github_files
```
This could be supported on the condition that the mirror is a user plugin, and would result in passing `None` values for the configured URI passed through `SourceMirror.translate_url()`.
## Support non aliased URIs
In your example:
```yaml
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
config:
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
- cpan
- crates
- ftp_gnu_org
- github_files
```
This will require restructuring the `Source.__do_fetch()` and `Source.__do_track()` loops and is a bit more involved.
I.e. we would want a mirror to be allowed to apply to _"any alias"_ and/or _"no alias"_.
For the _"no alias"_ case it is a bit more clear cut, afaics there are specific code blocks where we simply do not as the `Project` about the urls for aliases and directly invoke the plugin implementation.
### For plugins who want to support _"any alias"_
For the _"any alias"_ case, maybe we would want a `None` URL with a `SourceMirror` tuple to be returned in the list from `Project.get_alias_uris()` (allowing the project to continue to respect the user configuration of priorities).
Both cases will require that `Source.__clone_for_uri()` work with a `None` uri and/or a `None` alias.
The last part, for blanket mirrors to not need to specify the alias/[urls], it would make sense for `Source.translate_url()` to be allowed to _"not work"_ when given unrecognized aliases.
This could allow specification of multiple SourceMirror plugins which do things in different ways, and can bail out on aliases/urls which that plugin does not support, also it allows falling back to other mirrors or original project alias if the user has configured `SourceUriPolicy.ALL` and the mirror failed to produce a URI.
This is tricky to accomplish with the current code structure though. At fetch/track time it could work by raising an exception, allowing the loop to continue when the SourceMirror does:
```python
def translate_url(self, ... alias ...):
if i_dont_know_this(alias):
raise SourceMirrorUnhandled()
```
This would abort the plugin code calling into `Source.translate_url()` without breaking existing plugin code, as long as mirrors are never used in `Plugin.configure()` time and only at `Source.fetch()` and `Source.track()` time (we would only end up throwing away cloned Sources in the case that a URI could not be produced, and move on to the next mirror/uri).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1512556940
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
This is just a test case, and the `"<invalid>"` URLs set to the aliases are only there to indicate that it would be impossible for the test case to _work_ using the traditional mirror configuration keys, but that it works _only_ by virtue of the `SourceMirror` plugin parsing the additional data set in the plugin configuration (`config:` section)
The API changes in the YAML, compared to before, are only the following:
* When defining `mirrors:`
* The `mirror` definition now has an optional `kind:`, which can be used to refer to a plugin (defaulting to the default `SourceMirror` base plugin in the core)
* The `mirror` definition now has an optional `config:` section, which can be parsed and validated by the plugin, in `Plugin.configure()` in the normal way.
Does this answer your questions ? I think you are misunderstanding this test case and I hope I've clarified it, otherwise I am perhaps misunderstanding the issue you are raising.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1526119859
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
For mirror plugins where a single string is sufficient for per-alias configuration, the plugin wouldn't need to define its own `aliases` dict in its `config` section and could use meaningful values in the main `aliases` dict instead of "\<invalid>". E.g., the test plugin also works with the following changes: https://gist.github.com/juergbi/1c57fc0a5c0d512ef7f51654f0f0b26d (I understand that the current test is better for validation purposes, this is just for illustration).
I assume that potential future support for mirror plugins without specifying aliases will anyway require opt-in from the plugin. Do you think it won't be possible to extend the proposed plugin mirror API to allow opt-in of such a feature? Or was your concern only the need for "\<invalid>" values, which I've addressed in the first paragraph?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1526427770
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
Just to be clear, this discussion is supposed to be on https://github.com/apache/buildstream/pull/1890#discussion_r1509989058 (I hope you are replying to that, and not to this which is simply pointing out the weird source plugins). I've tried to reply earlier, but my comment was detached from this discussion by a github bug (https://github.com/apache/buildstream/pull/1890#discussion_r1523626484).
Anyway, my point is that by forcing plugins to have aliases defined in a specific way, they end up doing weird things to try to work around it. For instance, take my gitlab-lfs-mirror plugin from https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/merge_requests/18099. I am doing
```
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
config:
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
cpan: [invalid]
crates: [invalid]
ftp_gnu_org: [invalid]
github_files: [invalid]
oasis_open_xml: [invalid]
pagure_releases: [invalid]
pypi: [invalid]
sourceforge: [invalid]
sourceforge_download: [invalid]
sourceware_pub: [invalid]
tar_https: [invalid]
```
I'd like to be able to do
```
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
config:
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
- cpan
- crates
- ftp_gnu_org
- github_files
- oasis_open_xml
- pagure_releases
- pypi
- sourceforge
- sourceforge_download
- sourceware_pub
- tar_https
```
Or better yet, dropping the `config:`
```
- name: freedesktop-sdk-raw-file-mirrors
kind: gitlab_lfs_mirror
url: https://gitlab.com/
project: freedesktop-sdk/mirrors/{alias}/raw-files
aliases:
- cpan
- crates
- ftp_gnu_org
- github_files
- oasis_open_xml
- pagure_releases
- pypi
- sourceforge
- sourceforge_download
- sourceware_pub
- tar_https
```
Ultimately, the plugin only needs to tell the core the aliases that it can handle and optionally how many URLs it can provide for a given alias. Otherwise, it should be free to define its configuration in the way that makes most sense to what it is doing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1510895684
##########
src/buildstream/sourcemirror.py:
##########
@@ -0,0 +1,203 @@
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Authors:
+# Tristan Van Berkom <tr...@codethink.co.uk>
+"""
+SourceMirror - Base source mirror class
+=======================================
+The SourceMirror plugin allows one to customize how
+:func:`Source.translate_url() <buildstream.source.Source.translate_url>` will
+behave when looking up mirrors, allowing some additional flexibility in the
+implementation of source mirrors.
+
+
+.. _core_source_mirror_abstract_methods:
+
+Abstract Methods
+----------------
+For loading and configuration purposes, SourceMirrors may optionally implement
+the :func:`Plugin base class Plugin.configure() method <buildstream.plugin.Plugin.configure>`
+in order to load any custom configuration in the `config` dictionary.
+
+The remaining :ref:`Plugin base class abstract methods <core_plugin_abstract_methods>` are
+not relevant to the SourceMirror plugin object and need not be implemented.
+
+Sources expose the following abstract methods. Unless explicitly mentioned,
+these methods are mandatory to implement.
+
+* :func:`SourceMirror.translate_url() <buildstream.source.SourceMirror.translate_url>`
+
+ Produce an appropriate URL for the given URL and alias.
+
+
+Class Reference
+---------------
+"""
+
+from typing import Optional, Dict, List, Any, TYPE_CHECKING
+
+from .node import MappingNode, SequenceNode
+from .plugin import Plugin
+from ._exceptions import BstError, LoadError
+from .exceptions import ErrorDomain, LoadErrorReason
+
+if TYPE_CHECKING:
+
+ # pylint: disable=cyclic-import
+ from ._context import Context
+ from ._project import Project
+
+ # pylint: enable=cyclic-import
+
+
+class SourceMirrorError(BstError):
+ """This exception should be raised by :class:`.SourceMirror` implementations
+ to report errors to the user.
+
+ Args:
+ message: The breif error description to report to the user
+ detail: A possibly multiline, more detailed error message
+ reason: An optional machine readable reason string, used for test cases
+
+ *Since: 2.2*
+ """
+
+ def __init__(
+ self, message: str, *, detail: Optional[str] = None, reason: Optional[str] = None, temporary: bool = False
+ ):
+ super().__init__(message, detail=detail, domain=ErrorDomain.SOURCE, reason=reason)
+
+
+class SourceMirror(Plugin):
+ """SourceMirror()
+
+ Base SourceMirror class.
+
+ All SourceMirror plugins derive from this class, this interface defines how
+ the core will be interacting with SourceMirror plugins.
+
+ *Since: 2.2*
+ """
+
+ # The SourceMirror plugin type is only supported since BuildStream 2.2
+ BST_MIN_VERSION = "2.2"
+
+ def __init__(
+ self,
+ context: "Context",
+ project: "Project",
+ node: MappingNode,
+ ):
+ # Note: the MappingNode passed here is already expanded with
+ # the project level base variables, so there is no need
+ # to expand them redundantly here.
+ #
+ node.validate_keys(["name", "kind", "config", "aliases"])
+
+ # Do local base class parsing first
+ name: str = node.get_str("name")
+ self.__aliases: Dict[str, List[str]] = self.__load_aliases(node)
+
+ # Chain up to Plugin
+ super().__init__(name, context, project, node, "source-mirror")
+
+ # Plugin specific parsing
+ config = node.get_mapping("config", default={})
+ self._configure(config)
+
+ ##########################################################
+ # Public API #
+ ##########################################################
+ def translate_url(
+ self,
+ *,
+ project_name: str,
+ alias: str,
+ alias_url: str,
Review Comment:
With optional I meant marking the type as `Optional[str]` for `alias` and `alias_url` as we already do for `alias_substitute_url`. BuildStream core could in the future call it with `None` if we add support for non-alias translation. This would have to be opt-in for source mirror plugins.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#issuecomment-2036837179
Superseded by #1903
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement plugin mirrors [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1527187182
##########
tests/frontend/mirror.py:
##########
@@ -807,3 +807,93 @@ def test_mirror_expand_project_and_toplevel_root(cli, tmpdir):
# Success if the expanded %{project-root} is found
assert foo_str in contents
assert bar_str in contents
+
+
+# Test a simple SourceMirror implementation which reads
+# plugin configuration and behaves in the same way as default
+# mirrors but using data in the plugin configuration instead.
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.usefixtures("datafiles")
+def test_source_mirror_plugin(cli, tmpdir):
+ output_file = os.path.join(str(tmpdir), "output.txt")
+ project_dir = str(tmpdir)
+ element_dir = os.path.join(project_dir, "elements")
+ os.makedirs(element_dir, exist_ok=True)
+ element_name = "test.bst"
+ element_path = os.path.join(element_dir, element_name)
+ element = generate_element(output_file)
+ _yaml.roundtrip_dump(element, element_path)
+
+ project_file = os.path.join(project_dir, "project.conf")
+ project = {
+ "name": "test",
+ "min-version": "2.0",
+ "element-path": "elements",
+ "aliases": {
+ "foo": "FOO/",
+ "bar": "BAR/",
+ },
+ "mirrors": [
+ {
+ "name": "middle-earth",
+ "kind": "mirror",
+ "aliases": {
+ "foo": ["<invalid>"],
Review Comment:
Tbf, I think that we can have all the needed flexibility (at least in the case where there is an alias) by making `SourceMirror._get_alias_uris()` public for plugins to implement. This allows the plugins to express whatever they can support.
As I said before, I think we could have this method just return the number of URLs it will provide, and have `SourceMirror.translate_url()` take an index rather than a "URL" which isn't exactly going to be a URL depending on what the source mirror is doing internally.
In the absence of an alias, I think we can just pass `None` as an alias. `Project.get_alias_uris()` currently allows the alias to be `None`, so it is more a question of forwarding this to mirrors, and making sure sources and source fetchers don't explode when given a substitution URL for a `None` alias. We may not do this now, but explicitly state that alias can be `None` in `SourceMirror._get_alias_uris()` (or whatever it will be called once it's made public).
The only question that remains is about a mirror plugin applying to more than one project: In the current state, a mirror is a property of a project, and `SourceMirror._get_alias_uris()` doesn't know about which project it is answering for. But I think this is a separate question.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org