smarthomeNG / smarthome

Device integration platform for your smart home
https://www.smarthomeNG.de
GNU General Public License v3.0
122 stars 92 forks source link

Usage of logging.default.yaml #633

Closed AAPohl closed 7 months ago

AAPohl commented 7 months ago

Hi, I am trying to get smarthomeng running within a docker container (base on henfri).

One problem is, that smarthomeng does not startup when it does not find logging.yaml in /usr/smarthome/etc. However, there is a default config (logging.default.yaml). That default pattern is used for module e.g.

I suggest to use this fallback also for logging.

I can implement that and create a PR, but only if it fits the concept and i dont miss something

msinn commented 7 months ago

When usinig SmartHomeNG without Docker, SmartHomeNG starts without any problems even if there is no etc/logging.yaml. In that case, etc/logging.yaml.default is copied to etc/logging.yaml SmartHomeNG starts as usual

It must be a problem in your (Docker-)configuration that prevents SmartHomeNG to copy the file.

AAPohl commented 7 months ago

Thank you for the info. I will look in that direction

AAPohl commented 7 months ago

I did some debugging. I think I got the bug:

I use a custom conf_dir with parameter --config_dir /mnt/conf

lib/smarthome.py Line 218 - 251 ensures, as you mention, all config files beeing copied from ".default". In my case for /mnt/conf (with /usr/local/smarthome/etc as source) Important: self._etc_dir is modified in Line 237 to /mnt/conf.

However, Line 211 (earlier) a instance of Log is created, that reads etc_dir in Line 66 stil to /usr/local/smarthome/etc.

Later, when configure_logging is called, that is not updated.

I suggest to move Line 66 lazy into the function that need it, as it is mutable.

AAPohl commented 7 months ago

https://github.com/smarthomeNG/smarthome/compare/master...AAPohl:smarthomeLoggingFix:master

It works with these modifications

Morg42 commented 7 months ago

Fix looks basically fine, but absolutely not merging into master... ;). please create a new PR from a develop based branch to develop.

AAPohl commented 7 months ago

Sorry for that. Made a new PR into develop

Morg42 commented 7 months ago

Merged fix.