Error running after fresh install (config not found)

andypiper commented 1 year ago

Fedora 37 on 12th Gen Framework laptop.

Running fw-fanctrl with no arguments after a fresh install:

$ fw-fanctrl
Traceback (most recent call last):
  File "/usr/local/bin/fw-fanctrl", line 226, in <module>
    main()
  File "/usr/local/bin/fw-fanctrl", line 221, in main
    fan = FanController(configPath=args.config, strategy=args.strategy)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/bin/fw-fanctrl", line 37, in __init__
    with open(configPath, "r") as fp:
         ^^^^^^^^^^^^^^^^^^^^^
IsADirectoryError: [Errno 21] Is a directory: '.'

So far as I can determine, this is because it is assuming that the config path is the current directory (.), but the configuration file has been installed to /home/[user]/.config/fw-fanctrl. Issuing fw-fanctrl --config ~/.config/fw-fanctrl/config.json causes the program to run.

It is not clear from the README or from the help output that a path to a config file is required, and it seems sensible to assume that the config file might be looked for in the location where it was installed to by the installation script. It might even make sense for this to operate at a system level (since the installation script expects to be run as root, and installs a system service), so having a default configuration under the standard Unix /etc path may be a useful step.

andypiper commented 1 year ago

I think I understand that the user is not intended / expected to run the command without a config file (since the config file is specified in the service script); and that it runs as root in that context (answers #12).

In this case I would suggest that the config file should be installed to /etc rather than to the user config directory; and that the help output should explain that it is intended to be run as a service, or directly only to change the strategy. It might also be a good idea to have a command that lists out the strategies from the config file.

TamtamHero commented 1 year ago

I think I understand that the user is not intended / expected to run the command without a config file (since the config file is specified in the service script); and that it runs as root in that context (answers #12).

Yup, you got it !

In this case I would suggest that the config file should be installed to /etc rather than to the user config directory;

Can you detail why do you think /etc is a better place for the config file ?

and that the help output should explain that it is intended to be run as a service, or directly only to change the strategy.

I can improve the Readme, that's for sure, even though it's getting quite big already :')

It might also be a good idea to have a command that lists out the strategies from the config file.

PRs welcomed o/

andypiper commented 1 year ago

So - this may be old-school thinking in UNIX terms, but generally speaking I'd expect system-level services to have a system-level default configuration file. You're already going to need to have superuser access to install and run the tool successfully. Having said that, I've also recently seen other (single user) systems (notably the Steam Deck) with services installed and configured on a per user basis via .config, and maybe that's better from the perspective of having the main system more read-only / less volatile / contained.

I'll look into helping with a PR with some of the suggestions I've made. Thanks for listening!

TamtamHero commented 1 year ago

Indeed,I now see how it would make sense to use /etc, but then it means we need to update the install/update script to clean the old config path, edit the readme, and we potentially perturbate existing users who have played with their config at the current path. I'm not sure it's worth the efforts, at least I won't do it (also if it ain't broke, don't fix it !). But if you feel like making a PR to change it, be my guest, I'm not opposed to it :)

zquestz commented 11 months ago

Definitely prefer /etc/ for the config file location. This is how the package is setup for Arch.

Since most of the existing users are technical, I am pretty sure they would be okay moving a configuration file, or the installer can see if they have an existing config at the old location, and copy it to /etc for them.