Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log message in west command __init__ func may cause infinite call stack #708

Closed
yvesll opened this issue Apr 29, 2024 · 5 comments
Closed

Comments

@yvesll
Copy link

yvesll commented Apr 29, 2024

If use any log api like self.err or self.die in WestCommand's __init__ function, it may throw following exception:

  ........................

  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 287, in _get_config
    self.die(f"can't run west {self.name}; it requires config "
  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 504, in die
    self.err(*args, fatal=True)
  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 487, in err
    if self.color_ui:
  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 510, in color_ui
    return self.config.getboolean('color.ui', default=True)
  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 287, in _get_config
    self.die(f"can't run west {self.name}; it requires config "
  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 504, in die
    self.err(*args, fatal=True)
  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 487, in err
    if self.color_ui:
  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 510, in color_ui
    return self.config.getboolean('color.ui', default=True)
  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 287, in _get_config
    self.die(f"can't run west {self.name}; it requires config "
  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 504, in die
    self.err(*args, fatal=True)
  File "/home/yves/.local/lib/python3.10/site-packages/west/commands.py", line 487, in err
    if self.color_ui:
RecursionError: maximum recursion depth exceeded

In __init__ phase, the variable config is None, but the property color_ui try infinitely get it from config

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 29, 2024

Thanks for the report! Can you please share:

  • The west commit used
  • The one-line diff
  • Just in case, the manifest used.

@yvesll
Copy link
Author

yvesll commented Apr 30, 2024

Hi, @marc-hb , sorry my description is not quite clear. The issue happens on a west extension command and my installed west version is v1.2.0

Hook following extension command to a west project can reproduce the issue.

# example.py
from west.commands import WestCommand
class Example(WestCommand):
    def __init__(self):
        super().__init__(
          'example',
          # Keep this in sync with the string in west-commands.yml.
          'example',
          "example")
        
        # This may raise an exception
        self.err("show a log")

    def do_add_parser(self, parser_adder):
        parser = self._parser(parser_adder, epilog='')
        return parser
    
    def do_run(self, args):
        pass

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 30, 2024

Initialization is often tricky: ordering dependencies can be hard. So it's not a major surprise that some things don't work (yet) in an __init__() method. I just had a look at initializations in zephyr/scripts/west_commands/ and none of them has any logic, just constants. This should probably be better documented but: "In Rome, do as the Romans do?" I recommend you defer any logic somewhere after __init__() time.

This being said, error-handling should be better: better message and nothing should ever trigger an infinite loop/recursion.

@yvesll yvesll closed this as completed Sep 20, 2024
@marc-hb
Copy link
Collaborator

marc-hb commented Sep 20, 2024

@yvesll can you please explain what prompted you to close this?

@yvesll
Copy link
Author

yvesll commented Sep 21, 2024

I think we should not put too complex logic in __init__ func, as you say, its tricky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants