You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by GitBox <gi...@apache.org> on 2022/07/14 10:00:02 UTC
[GitHub] [buildstream] abderrahim opened a new issue, #1685: Source.set_ref sometimes gets passed a dictionary instead of a Node instance
abderrahim opened a new issue, #1685:
URL: https://github.com/apache/buildstream/issues/1685
The [documentation](https://docs.buildstream.build/master/buildstream.source.html#buildstream.source.Source.set_ref) it says that node is a buildstream.node.MappingNode, which isn't always true in my testing.
There is a TODO at https://github.com/apache/buildstream/blob/91130cc61e44bd627f515785df25ae3d814d00af/src/buildstream/source.py#L1018 about it being "nicer" to use the Node API. However, this critical because it is API.
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan closed issue #1685: Source.set_ref sometimes gets passed a dictionary instead of a Node instance
Posted by GitBox <gi...@apache.org>.
gtristan closed issue #1685: Source.set_ref sometimes gets passed a dictionary instead of a Node instance
URL: https://github.com/apache/buildstream/issues/1685
--
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
[GitHub] [buildstream] abderrahim commented on issue #1685: Source.set_ref sometimes gets passed a dictionary instead of a Node instance
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1685:
URL: https://github.com/apache/buildstream/issues/1685#issuecomment-1191937428
I just remembered that the `git` plugin with`track-tags` enabled needs something complex. It does something [pretty convoluted](https://github.com/apache/buildstream-plugins/blob/master/src/buildstream_plugins/sources/git.py#L830) to keep buildstream happy.
--
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
[GitHub] [buildstream] abderrahim commented on issue #1685: Source.set_ref sometimes gets passed a dictionary instead of a Node instance
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1685:
URL: https://github.com/apache/buildstream/issues/1685#issuecomment-1191930697
Related to this, we should also have a recommendation for how plugins should store their ref (and consequently what type should `Source.get_ref()` return).
I find this very confusing, and whoever wrote the `cargo` plugin too. And even though I tried to fix it by porting changes from bst-external. I still find that it's not correct.
--
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
[GitHub] [buildstream] gtristan commented on issue #1685: Source.set_ref sometimes gets passed a dictionary instead of a Node instance
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1685:
URL: https://github.com/apache/buildstream/issues/1685#issuecomment-1197814637
I've updated #1704 to also include some more elaboration about how Sources are expected to handle the ref in the documentation.
--
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
[GitHub] [buildstream] abderrahim commented on issue #1685: Source.set_ref sometimes gets passed a dictionary instead of a Node instance
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1685:
URL: https://github.com/apache/buildstream/issues/1685#issuecomment-1197341593
> Maybe it's worth documenting that the plugin restrict it's ref value to occupying only a single _key_ in the _node_ passed to `Source.set_ref(ref, node)`, and that _that key's value_ is what is expected to be returned by `Source.get_ref()`, the recommended key name for this is _"ref"_.
Sounds good. I think this is what we found out was best while investigating #1642. (@nanonyme correct me if I'm wrong).
> The actual type of the _value_ of the ref is only expected to be a simple python data structure, using only `dict`, `list` and normal scalar values such as `str` and `int`. Python _typing_ syntax does not allow us to strongly specify this constraint on our `SourceRef` definition (I believe the possibly recursive nature of the type definition is what prevents this), but we should ensure this is documented.
While this sounds good, it doesn't really answer my question. In particular, the problem I had is that "_that key's value_ is what is expected to be returned by `Source.get_ref()`" implies that it's type is a `Node` subclass, or more likely a list/dict whose values are `Node`s.
Either way, with #1704 the cargo plugin as currently written works fine. So I'd say what you wrote above is fine as a guideline.
--
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
[GitHub] [buildstream] gtristan commented on issue #1685: Source.set_ref sometimes gets passed a dictionary instead of a Node instance
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1685:
URL: https://github.com/apache/buildstream/issues/1685#issuecomment-1196346500
> Related to this, we should also have a recommendation for how plugins should store their ref (and consequently what type should `Source.get_ref()` return). For plugins where ref is a scalar it's pretty obvious, but when it's a sequence or even more complex it's way less obvious how to navigate between `load_ref()`, `get_ref()` and `set_ref()` and keep them all happy.
>
> I find this very confusing, and whoever wrote the `cargo` plugin too. And even though I tried to fix it by porting changes from bst-external. I still find that it's not correct.
Maybe it's worth documenting that the plugin restrict it's ref value to occupying only a single *key* in the node passed to `Source.set_ref()`, and that *that key's value* is what is expected to be returned by `Source.get_ref()` is the value for *that key* (which is typically named *"ref"* by the plugin).
The actual type of the *value* of the ref is only expected to be a simple python data structure, using only `dict`, `list` and normal scalar values such as `str` and `int`. Python *typing* syntax does not allow us to strongly specify this constraint on our `SourceRef` definition (I believe the possibly recursive nature of the type definition is what prevents this), but we should ensure this is documented.
While the actual value structure of the ref is irrelevant to buildstream, it should also be noted that the *order of elements* in a `list` and the *order of keys* in a `dict` is relevant and that the plugin should use stable algorithms to generate these, otherwise the plugin risks generating random cache keys.
--
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