search

mainsail-crew / crowsnest

Webcam Service for multiple Cams
GNU General Public License v3.0
302 stars 71 forks source link

make install assumes output goes to a user home-folder. Should not assume anything #209

Closed etamminga closed 7 months ago

etamminga commented 7 months ago

What happened

make install with the following .config file:

BASE_USER="admin"
CROWSNEST_USTREAMER_REPO_SHIP="https://github.com/pikvm/ustreamer.git"
CROWSNEST_USTREAMER_REPO_BRANCH="master"
CROWSNEST_CAMERA_STREAMER_REPO_SHIP="https://github.com/ayufan/camera-streamer.git"
CROWSNEST_CAMERA_STREAMER_REPO_BRANCH="master"
CROWSNEST_CONFIG_PATH="/opt/klipper/printer_data/config"
CROWSNEST_LOG_PATH="/opt/klipper/printer_data/logs"
CROWSNEST_ENV_PATH="/opt/klipper/printer_data/systemd"
CROWSNEST_ADD_CROWSNEST_MOONRAKER="0"

tools/libs/core.sh in function install_env_file() assumes the username $(BASE_USER) must exist in the env file. If it does not exists, it returns an error. My CROWSNET_ENV_PATH does not contain the name of the user.

Better would be to test for the %CONFPATH% keyword to exist. If it exists, return an error.

What did you expect to happen

Install in my specified folder

How to reproduce

Create mentioned .config file and do a make install

Additional information

No response

mryel00 commented 7 months ago

You are right but this whole tools/.config option was designed around multi-instance Klipper+Moonraker setups. There was never an intention of setting anything outside the home directory of your user. Currently the implementation only limits the config file to be inside the home directory of BASE_USER. As a linux file is normally readable by default by anyone, there shouldn't be any problems. So I will change that with the next update.

You and everyone else have to understand that we don't support any setup that are outside the "normal" setup for Moonraker and Klipper. If you get any problems editing your config or that the log file doesn't get written to or anything similar, we won't support you on that, as this is out of the scope of the intended purpose of Crowsnest.

So I will "fix" this bug as it doesn't make sense for the overall implementation but with every problem following that you will be on your own.

The fix will be that we check on CROWSNEST_CONFIG_PATH as in every other part of that installation. That parameter will be inserted for %CONFPATH%. So checking for %CONFPATH% doesn't make any sense and would lead to the same error you already got.

etamminga commented 7 months ago

Thank you!

It's not my intention to force any support. Just mentioning an item that could be better implemented. I (We) don't know any of the design principles. So just reporting what I find. Found this during development of an ansible role to install klipper for my personal setup. (for when the Pi SD fails again on me)

mryel00 commented 7 months ago

It's not my intention to force any support. Just mentioning an item that could be better implemented. I (We) don't know any of the design principles. So just reporting what I find.

I'm glad for that, don't get me wrong😄 I just wanted to clearly state this at some point, not that someone else comes across this and thinks we would now do support on setups like that.

Thank you again for the heads up on that implementation detail.