Buildstream crashes unhelpfully when ujson can't process a cache key #1341

Open BuildStream-Migration-Bot opened 3 years ago

See original issue on GitLab In GitLab by [Gitlab user @willsalmon] on Jun 16, 2020, 10:28

Summary

[Edit/update] Buildstream plugins must supply a function called "get_unique_key", which returns an object that can be used to identify an element based on the config defined in its YAML. If the object is in a format that cannot be processed correctly, this can cause buildstream to crash when calculating the key.

There ought to be a clean failure, with a helpful error message. Instead we get a crash, and a traceback that doesn't immediately explain the problem. [End edit/update]

(venvbuildstream) [will[[Gitlab user @wsx280]](https://gitlab.com/wsx280) freedesktop-sdk]$ bst build flatpak-release.bst
[--:--:--][        ][    main:core activity                 ] START   Build
[--:--:--][        ][    main:core activity                 ] START   Loading elements
[00:00:02][        ][    main:core activity                 ] SUCCESS Loading elements
[--:--:--][        ][    main:core activity                 ] START   Resolving elements
[00:00:00][        ][    main:core activity                 ] SUCCESS Resolving elements
[--:--:--][        ][    main:core activity                 ] START   Initializing remote caches
[00:00:00][        ][    main:core activity                 ] SUCCESS Initializing remote caches
[--:--:--][        ][    main:core activity                 ] START   Resolving cached state
[--:--:--][        ][    main:core activity                 ] START   Terminating buildbox-casd
[00:00:01][        ][    main:core activity                 ] SUCCESS Terminating buildbox-casd
[--:--:--][        ][    main:core activity                 ] BUG     <configparser.ConfigParser object at 0x7f5a1077e850> is not JSON serializable

    Traceback (most recent call last):
      File "/home/will/projects/buildstream/venvbuildstream/bin/bst", line 8, in <module>
        sys.exit(cli())
      File "/home/will/projects/buildstream/venvbuildstream/lib64/python3.7/site-packages/click/core.py", line 829, in __call__
        return self.main(*args, **kwargs)
      File "/home/will/projects/buildstream/buildstream/src/buildstream/_frontend/cli.py", line 280, in override_main
        original_main(self, args=args, prog_name=prog_name, complete_var=None, standalone_mode=standalone_mode, **extra)
      File "/home/will/projects/buildstream/venvbuildstream/lib64/python3.7/site-packages/click/core.py", line 782, in main
        rv = self.invoke(ctx)
      File "/home/will/projects/buildstream/venvbuildstream/lib64/python3.7/site-packages/click/core.py", line 1259, in invoke
        return _process_result(sub_ctx.command.invoke(sub_ctx))
      File "/home/will/projects/buildstream/venvbuildstream/lib64/python3.7/site-packages/click/core.py", line 1066, in invoke
        return ctx.invoke(self.callback, **ctx.params)
      File "/home/will/projects/buildstream/venvbuildstream/lib64/python3.7/site-packages/click/core.py", line 610, in invoke
        return callback(*args, **kwargs)
      File "/home/will/projects/buildstream/venvbuildstream/lib64/python3.7/site-packages/click/decorators.py", line 33, in new_func
        return f(get_current_context().obj, *args, **kwargs)
      File "/home/will/projects/buildstream/buildstream/src/buildstream/_frontend/cli.py", line 501, in build
        app.stream.build(elements, selection=deps, ignore_junction_targets=ignore_junction_targets, remote=remote)
      File "/home/will/projects/buildstream/buildstream/src/buildstream/_stream.py", line 282, in build
        dynamic_plan=True,
      File "/home/will/projects/buildstream/buildstream/src/buildstream/_stream.py", line 1266, in _load
        self._pipeline.resolve_elements(self.targets)
      File "/home/will/projects/buildstream/buildstream/src/buildstream/_pipeline.py", line 121, in resolve_elements
        element._initialize_state()
      File "/home/will/projects/buildstream/buildstream/src/buildstream/element.py", line 1184, in _initialize_state
        self.__update_resolved_state()
      File "/home/will/projects/buildstream/buildstream/src/buildstream/element.py", line 2330, in __update_resolved_state
        self.__update_cache_keys()
      File "/home/will/projects/buildstream/buildstream/src/buildstream/element.py", line 3006, in __update_cache_keys
        self.__weak_cache_key = self._calculate_cache_key(dependencies)
      File "/home/will/projects/buildstream/buildstream/src/buildstream/element.py", line 2104, in _calculate_cache_key
        return _cachekey.generate_key(cache_key_dict)
      File "/home/will/projects/buildstream/buildstream/src/buildstream/_cachekey.py", line 65, in generate_key
        ustring = ujson.dumps(value, sort_keys=True, escape_forward_slashes=False).encode("utf-8")
    TypeError: <configparser.ConfigParser object at 0x7f5a1077e850> is not JSON serializable

(venvbuildstream) [will[[Gitlab user @wsx280]](https://gitlab.com/wsx280) freedesktop-sdk]$

Steps to reproduce

Install bst at tag 1.93.4

install bst-plugins-experimental at commit aa3c71568b0cdc537028e6ec389276bb79441f46 (HEAD -> master, origin/master, origin/HEAD)

checkout Freedesktop-sdk at

commit 6eaed03f17ba70428c7beac6b8121f2da62aac7d (HEAD -> willsalmon/bst2-fv, origin/willsalmon/bst2-fv)
Author: William Salmon <will.salmon[[Gitlab user @codethink]](https://gitlab.com/codethink).co.uk>
Date:   Wed May 13 16:07:47 2020 +0100

    Use pip from bst-plugins-experimental

What is the current bug behaviour?

[Gitlab user @BenjaminSchubert] thinks this is a issue with the plugins repo, i dont understand why it works in CI https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/jobs/593127809 with what i belive are the same versions.

But if the bug is in the plugins then bst should handle this failer cleanly

In GitLab by [Gitlab user @willsalmon] on Jun 16, 2020, 10:28

changed the description

In GitLab by [Gitlab user @willsalmon] on Jun 17, 2020, 11:40

A example in CI https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/jobs/598778674

In GitLab by [Gitlab user @coldtom] on Jun 17, 2020, 12:19

[Gitlab user @willsalmon] This is caused by the flatpak_image (or maybe flatpak_repo) plugin passing a configparser to get_unique_key, which can't be dumped by ujson.

In GitLab by [Gitlab user @DouglasWinship] on Jun 18, 2020, 14:30

In the CI example above, the fault was with the snap_image plugin.

Managed to avoid the issue by editing the configure() function.

-        self.metadata = node.get_node('metadata')
+        self.metadata = node.get_node('metadata').strip_node_info()

But as Will says, the real bug here (for buildstream) is that it doesn't fail cleanly.

In GitLab by [Gitlab user @abderrahimk] on Jun 20, 2020, 07:35

The difference in version is in ujson, ujson 1.x accepts the ConfigParser, but 2.0 doesn't.

I agree that bst should have a more defensive attitude towards things coming from the plugins, and fail cleanly.

In GitLab by [Gitlab user @DouglasWinship] on Jun 22, 2020, 10:33

changed title from {-Crash with trace back-} to {+Buildstream crashes unhelpfully when ujson can't process a cache key+}

In GitLab by [Gitlab user @DouglasWinship] on Jun 22, 2020, 10:33

changed the description

In GitLab by [Gitlab user @tristanvb] on Nov 6, 2020, 09:15

The current policy is that if BuildStream core or a BuildStream plugin has a bug.

If a plugin returns a non-serializable object to get_unique_key(), this certainly qualifies as a bug, and we should do the right thing and treat it as a BUG and always provide a stack trace.

Stack traces are more helpful for plugin authors to fix their bugs than simple messages, as a general rule.

Reported user facing errors are supposed to assist the CLI user and project YAML author directly, i.e. they should clearly inform the user how to fix the issue, or report an error from a build.

That said, I do think that the return of get_unique_key() should be asserted sooner, and a non BstError exception should be raised earlier, to more clearly indicate what went wrong.

apache / buildstream