search

apache / buildstream

BuildStream, the software integration tool
https://buildstream.build/
Apache License 2.0
80 stars 27 forks source link

Hang when duplicate keys defined in element #1917

Open nanonyme opened 3 months ago

nanonyme commented 3 months ago

Noticed with BuildStream 2.2.0

[12:40:35][--:--:--][        ][    main:core activity                 ] START   Build
[12:40:35][--:--:--][        ][    main:core activity                 ] START   Loading elements
[12:40:36][--:--:--][        ][    main:core activity                 ] BUG     Duplicate key (@) at line 26 column 0
    Traceback (most recent call last):
      File "/usr/lib/python3.11/site-packages/buildstream/_loader/loader.py", line 409, in _load_one_file
        element = self._elements[filename]
                  ~~~~~~~~~~~~~~^^^^^^^^^^
    KeyError: 'extensions/vulkaninfo/vulkan-tools.bst'

    During handling of the above exception, another exception occurred:

    buildstream._yaml.YAMLLoadError: Duplicate key (@) at line 26 column 0

BuildStream doesn't correctly handle the raised exception but instead hits BUG and promptly hangs after this.

abderrahim commented 3 months ago

Please post the freedesktop-sdk commit to reproduce this. It would help fix it.

nanonyme commented 3 months ago

https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/commit/e71cbc8432df1f5058cc11d1109d32a3ad7c4f73

nanonyme commented 3 months ago

You don't have to use the custom config template to reproduce. It's enough to use BuildStream 2.2.0 and that commit.

abderrahim commented 3 months ago

This is indeed weird. I can't reproduce it using my host install of buildstream (installed using pipx). However, using the CI container used in that commit I can reproduce it.

The error itself looks like we're leaking a YAMLLoadError which should be caught within the _yaml module. Reading the code, I suspect it's a typo here https://github.com/apache/buildstream/blob/c7274d41d0d0af87cc2081d67589841e8e16b592/src/buildstream/_yaml.pyx#L287, where should be YAMLLoadError instead of LoadError.

nanonyme commented 3 months ago

Is there a difference in ruamel.yaml or ruamel.yaml.clib versions?

abderrahim commented 3 months ago

Yes, indeed. On my system I have

ruamel.yaml        0.17.21
ruamel.yaml.clib   0.2.7

In the container

ruamel.yaml                 0.18.6
ruamel.yaml.clib            0.2.8

However, upgrading to the same version on my system I keep getting the same behaviour (a proper FAILURE, not a BUG with a hang).

nanonyme commented 3 months ago

This is so odd. While there may be a difference in Cython used to buils this extension, I don't expect it to affect quite that.