search

openhab / openhab-linuxpkg

Repo for Linux packages
Eclipse Public License 2.0
18 stars 33 forks source link

Removed oh2_dir_layout, replaced with env var usage #60

Closed BClark09 closed 7 years ago

BClark09 commented 7 years ago

Allowing for changes upstream to pass through. Also allows users to set their own.

closes #59

Signed-off-by: Ben Clark ben@benjyc.uk

BClark09 commented 7 years ago

Hi @ThomDietrich, fairly big change to allow openHAB distribution's version of oh2_dir_layout to be used. Would you mind going through it and maybe testing the changes? I'll be going through it a lot before merge.

I've done this by adding a second /etc/default/ file which contains the paths, and edited the init.d and systemd service starters to use them as environment vars.

ThomDietrich commented 7 years ago

Hey @BClark09, looking at the code all small changes look good. I can see that you took great care to make everything POSIX sh compliant. I've left a few comments.

I'm pretty much packed with events all week and will be traveling over the weekend. I'm afraid I can't test much...

BClark09 commented 7 years ago

Thanks @ThomDietrich, very helpful. Don't worry about testing, I can setup my usual procedure for each type of Unix on a virtual box and see if I get the same functionality. I don't expect to merge this soon.

ThomDietrich commented 7 years ago

I've done this by adding a second /etc/default/ file which contains the paths

May I ask why? Seems highly unusual...

BClark09 commented 7 years ago

May I ask why? Seems highly unusual...

Absolutely, it didn't seem like it fit anywhere else, and I'm worried that adding these variables to the existing /etc/default/openhab file will skip by some users, who would idly select the default "no" for overwriting it. Do you have any suggestions for this? I guess I could make sure these env vars are set in both init.d and systemd .service and use /etc/default/openhab to overwrite them if they exist.

ThomDietrich commented 7 years ago

Okay that sounds in line with what I've commented above. In my understanding everything below /etc/default should be optional and should be expected to be missing or wrong. Hence we should make sure to check all data provided via /etc/default/openhab2

Edit: found a source https://askubuntu.com/a/843068

BClark09 commented 7 years ago

Awesome, that's what I'll do then. Thanks Thomas!!

BClark09 commented 7 years ago

@ThomDietrich, Thought it'd be best to go through systemd here, from the beginning to clarify things:

User= and Group= are systemd specific and do not expand, these have to be written into the conf file and cannot use variables. The exception to this is that you can use %i in a template, the %i is replaced with [param] when you start the service with start openhab2@[param].service. Otherwise, you'd make the file a conffile and allow users to type their configuration in.

At this point in the PR, openhab2.service is NOT a conffile, and is replaced on each update. This was done to make sure that everyone updates to the new format. The path environment variables must be set for this service to run correctly, so if we kept this as a connfile then I can be sure some people would be confused as to why their systems are failing.

So the way I see it, we have the following options (I'm currently leaning towards 1):

  1. Keep openhab2.service as a conffile, and hope that most people who have edited the service file do not ignore the warning. I imagine this would be the most "unix" approach.
  2. Make openhab2.service a non-conffile and have users either write their own service file or overwrite each update.
  3. Make openhab2.service a non-confile, but add a service template, and start telling users who use root to define the user with start openhab2@root.service. People that use openhab as the user would still need to write openhab2@openhab.service.

Unless you can think of an alternative? :thinking:

BClark09 commented 7 years ago

Based on tests, and reading up on the use cases of setting root as the user and group for openHAB. I've decided that it's best to keep openhab2.service as static file, and that /etc/default/openhab2 can be used for path variations.

In my opinion, the user and group for openHAB should never be root. GPIO, dialout and tty groups exist for this reason. Those who really need to set a different user can do so on each update.

I can as always be persuaded the other way if you think differently?

ThomDietrich commented 7 years ago

I agree with this approach. The user should not execute openHAB as root! We shouldn't provide this option as a feature. The problems I know of can all be solved by adding openhab to gpio/dialout/tty, by setcap or by adding a sudo exception. If a user really knows what he is doing, he will know where to mess around to run openHAB as root.

Last commit looks good. Great work @BClark09

BClark09 commented 7 years ago

Awesome, thanks again Thom! I've updated the PR with hopefully the final changes. I've pushed packages to the test repo and will test them soon.

BClark09 commented 7 years ago

Changes, work across Ubuntu <14, Debian 8&9, and Fedora so will merge now and monitor any problems.