[aleixpol]

When composing onto a string allow prepending and appending onto them.

It uses the same operators we use for lists (<), (=) and (>).

[nathanwilliams-ct]

Need to update the documentation: https://github.com/apache/buildstream/blob/master/doc/source/format_intro.rst#-list-prepend

[aleixpol]

Need to update the documentation: https://github.com/apache/buildstream/blob/master/doc/source/format_intro.rst#-list-prepend

I gave it a go, please see what it looks like. Don't be afraid of saying it's a bit shit, it's always harder to document something more abstract. Ideas welcome.

[aleixpol]

I think all the concerns have been addressed now.

[nathanwilliams-ct]

LGTM

[juergbi]

As ScalarNode strips whitespace from the start and end of strings (even if they're quoted), it won't be possible to append e.g. " -foo" to a flags variable, as far as I can tell. Not sure what the best solution is for this, especially considering backwards compatibility, but this might block real use cases.

[aleixpol]

Good catch!

Is it a necessary feature to have to always strip?

We could check if we are in an operator and then not do it.

Or we can add a !preserve yaml tag and we can skip the stripping. So it would be (>): !preserve " -banana". I'll try implementing something like that unless someone is baffled by the concept.

[juergbi]

I'm wondering whether we could move the strip() call from ScalarNode.__cinit__ to ScalarNode.as_str(). We'd need to do some testing, but I think this should preserve the stripping for final values that BuildStream and plugins work with, while still allowing composition to work on the raw values.

I'd rather avoid YAML tags if we can.

[juergbi]

diff --git a/src/buildstream/node.pyx b/src/buildstream/node.pyx
index 761e7b775..8adffd212 100644
--- a/src/buildstream/node.pyx
+++ b/src/buildstream/node.pyx
@@ -278,7 +278,7 @@ cdef class ScalarNode(Node):
         cdef value_type = type(value)
 
         if value_type is str:
-            value = value.strip()
+            value = value
         elif value_type is bool:
             if value:
                 value = "True"
@@ -401,7 +401,7 @@ cdef class ScalarNode(Node):
         # We keep 'None' as 'None' to simplify the API's usage and allow chaining for users
         if self.value is None:
             return None
-        return str(self.value)
+        return str(self.value).strip()
 
     cpdef bint is_none(self):
         """Determine whether the current scalar is `None`.

[aleixpol]

Cool! That addresses it then. I had to add some more things around it but it seems to work.

[juergbi]

Unfortunately, I think there is a case where this may break cache keys. The issue is that variable expansion happens after __cinit__ but before as_str() is called. This means that for something like

cmake-args: -DFOO %{cmake-local}

if cmake-local is empty, bst 2.8 expands cmake-args to -DFOO with a trailing space. With the latest change in this branch, the trailing space will be stripped. While the difference may typically not be significant, I think there is a real risk that this changes existing cache keys.

A possible solution could be to revert the latest change, but let _handle_wait_value_ScalarEvent in _yaml.pyx pass extra information to the ScalarNode constructor. In particular, pass through ev.style and key. __cinit__ could then skip stripping if style is either " or ' (i.e., the value is a quoted string) and key is either (<) or (>). This should result in a narrow carveout that shouldn't affect any existing projects (as there was no string composition) while still fixing the stripping issue for composition.

I haven't tested this and it's certainly possible that I'm still missing something.

Edit: clone() currently results in the string being stripped as well, which seems like a bug. There is a possibility that the trailing space in the scenario mentioned above is eliminated in typical usage, but I haven't checked.