Overnigth jobs are failing: BUG: Attempt to do `checkout in Node ...

changed the description

changed title from Overnigth jobs are failing: to Overnigth jobs are failing: BUG: Attempt to do `checkout in Node ...

@jjardon This will be because the overnight tests from master are still using a bst-external which is only compatible with BuildStream 1.

@danielsilverstone-ct I think this is from your "BUG" assertion added in 3816dcf8, do you think we could improve this plugin author facing assertion message please ?

First, there is no need for the message to redundantly include the word "BUG", it is an unhandled exception so it will anyway be reported in the logs as a "BUG".

Secondly, I think that the message itself is confusing, we could do better to inform the plugin author that the in expression is not supported by the Node type (I think the plugin author would eventually figure it out, but we should make it easier and more obvious I think).

mentioned in merge request !1275 (merged)

@tristanvb @jjardon Yeah, I can see that the message is bad. I'll put it on my list to improve today, pointing people at node_has_member() or similar

I propose to update that error as follows:

        assert False, \
            """
            Code contains `{} in {}` test which is unsupported by YAML nodes.

            If this is in a plugin, then please ask the author to use the new
            `self.node_has_member()` method instead.
            """.format(what, self)

How does that sit for you?

It looks like this is still going to format the self as something wonky like:

`checkout in Node(value={'checkout': Node(value='True', file_index=6, line=35, column=16), 'url': Node(value='gnome:libglnx.git', file_index=6, line=36, column=11)}, file_index=6, line=35, column=6)`

I doubt that it is valuable to show the whole stringified namedtuple for this case; the stack trace above the message should be enough to trace down where the invalid in statement originated from.

I think a simpler message here might be:

"Unsupported check for member '{}' in YAML Node, use Plugin.node_has_member() instead".format(what)

I wouldn't go as far as "If this is in a plugin", in any case it is a bug, either in BuildStream core or in a plugin. Stack traces from YAML node accesses are not really any more special than any other stack trace: we hope that users never see them, and if they do, we hope they file a bug somewhere; the stack trace will help us to determine where the bug originated from.

Beyond this, if Plugin.node_has_member() is going to exist, does it not make sense to simply call it instead of raising an error (and actually support the in statement) ?

We made the decision to not support in because when we did it tended to mask other parts of code which needed fixing. However I suppose now it might be better to simply support it, you're right there. I'll give it a bit of thought. I take your point about the stringified node though.

mentioned in merge request !1280 (merged)

I've decided to agree with you and have submitted !1280 (merged) to Marge for merge (after James reviewed it).

@jjardon You'll find this message no longer exists, but any other issues it was masking may now come forward. Please keep an eye out and either transmute this report as needed, or close it and open fresh ones if necessary.

Thanks for your vigilance,

D.

Thanks a lot @danielsilverstone-ct

Let's open a new one

closed

mentioned in issue #989 (closed)

Overnigth jobs are failing: BUG: Attempt to do `checkout in Node ...

Child items 0

Activity