search

openenergymonitor / emonpi

Raspberry Pi Based Energy Monitor. Hardware, Firmware & related software for the PI.
https://guide.openenergymonitor.org/setup
270 stars 113 forks source link

update service file + create install script same manner as emonhub #121

Closed alexandrecuer closed 4 years ago

alexandrecuer commented 4 years ago

This PR will soon have a 'sister' one on the EmonScripts repo

goal : adopt a modus operandi similar to that of emonhub, plus preparation for the transition to python3 cf this PR : https://github.com/openenergymonitor/emonpi/pull/120

Nota : the use of PermissionsStartOnly=true is said to be deprecated https://superuser.com/questions/1504114/permissionsstartonly-alternative-in-systemd* Anyway, I kept it as it is as we can find it on most OEM repos

Best

Alex

borpin commented 4 years ago

Looks good.

This really needs to simply do the install so the end result is the same as it was before. Introduce other changes in subsequent releases.

Let's not change too many things at once.

It needs a complementary update script eventually.

alexandrecuer commented 4 years ago

There is no need for the check on python version. the version of the install should match the python files in the release.

True but with the proposed change, you dont have to change the service file when going to python3....it will follow by itself

This really needs to simply do the install so the end result is the same as it was before. Introduce other changes in subsequent releases.

the only change is the user which I change from user root to user pi...anyway, it was not working well with user root

The first part of the process triggers update/service-runner-update.sh from the EmonScripts repo :

In the second part of the process, the update/main.sh script will :

So actually the lcd/install.sh file will not be launched by the update process except if we include a specific command in the update/emonpi.sh, as it was recently done for emonhub....

TrystanLea commented 4 years ago

Thanks @alexandrecuer this looks great, are both of these pull requests ready to merge? Il try and test here in the next couple of days and feedback what I can

alexandrecuer commented 4 years ago

normally it is ready to merge but please test before...

The only thing I forgot is to update the emonpi.sh script from the EmonScripts, in the manner on emonhub.sh..

Indeed, the emonpi.sh does not include a command such as lcd/./install.sh, so the update will have to be launched manually by the user through ssh..

Le 29/06/2020 à 14:44, > Trystan Lea (par Internet, dépôt noreply@github.com) a écrit :

Thanks @alexandrecuer https://github.com/alexandrecuer this looks great, are both of these pull requests ready to merge? Il try and test here in the next couple of days and feedback what I can

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openenergymonitor/emonpi/pull/121#issuecomment-651094671, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF3KSCZYUI6GJUCGHRDQMTDRZCEDDANCNFSM4OIW7FLQ.

TrystanLea commented 4 years ago

Thanks @alexandrecuer great

borpin commented 4 years ago

I don't like the test for Python version - it isn't done on any other install so should not be needed here.

There needs to be complementary update script.

alexandrecuer commented 4 years ago

I don't like the test for Python version

OK just removed :-)

Anyway doing this I just followed one of your idea :

cf https://github.com/openenergymonitor/emonpi/pull/120#issuecomment-646085248

borpin commented 4 years ago

Yes but I realised that actually you don't need to know as the install should match the script within the repository at any point in time / release.

borpin commented 4 years ago

OK just removed :-)

I do think it is cleaner and when the python 3 version is subsequently released, the install & update scripts should be changed to match.

alexandrecuer commented 4 years ago

Just amended EmonScripts to make the update process work, and updated some documentation

My testing setup in update mode is the following :

Everything went well.....

So if testing (both install and update) is successfull from your side, I think this is OK for merge

@TrystanLea : next step could be to create a specific python3 branch with the @bwduncan python3 PR....

best

Alex

TrystanLea commented 4 years ago

Hello @alexandrecuer was there a reason not to include the emonpilcd log folder and file creation? https://github.com/openenergymonitor/EmonScripts/blob/master/install/emonpilcd.sh#L12

borpin commented 4 years ago

was there a reason not to include the emonpilcd log folder and file creation?

It is now in the service file just like the other services.

alexandrecuer commented 4 years ago

Yes log files are managed by systemd cf emonhub

https://github.com/openenergymonitor/emonhub/blob/master/service/emonhub.service

But using user/group pi instead of user emonhub

Best Alex

alexandrecuer commented 3 years ago

From what I know, the /opt/openenergymonitor/ path is a design decision of the OEM team, taken when the scripted installation was introduced. @TrystanLea : do yu confirm ?

in the install script, we could use sed to replace the hardcoded path in the service file by the real path of the emonPiLCD.py file...but it injects more complexity

borpin commented 3 years ago

This commit broke emonPiLCD

What SD Image are you running (emonPi release)?

image

@TrystanLea this could break quite a few installs. The solution is to create a dropin if the path /home/pi/emonpi exists on running the update with a new EXEC command (as we do for Ubuntu installs).

I did say at the outset....

  • Changing the service file might not be a good idea as part of this.

And

It needs a complementary update script eventually.

Neither of which were done... 😢

the only change is the user which I change

No you changed the path to the executable as well.

borpin commented 3 years ago

This commit broke emonPiLCD on my emonpi, because it does not live in /opt/openenergymonitor. Is there any documentation for this change?

The rest of the changes are great, though.

@bwduncan - have you just pulled an update? I think this is not the most recent update. My service file uses Python3.

# Systemd unit file for emonPiLCD script
# installation is done via the install.sh script in the same folder
# VIEW STATUS
# systemctl status emonPiLCD
# VIEW LOG
# journalctl -f -u emonPiLCD

[Unit]
Description=emonPi LCD driver
After=mosquitto.service

[Service]
User=pi
PIDFile=/var/run/emonpilcd.pid
Environment='LOG_PATH=/var/log/emonpilcd'
ExecStart=/usr/bin/python3 /opt/openenergymonitor/emonpi/lcd/emonPiLCD.py --logfile=${LOG_PATH}/emonpilcd.log
PermissionsStartOnly=true
ExecStartPre=/bin/mkdir -p ${LOG_PATH}/
ExecStartPre=/bin/chgrp -R pi ${LOG_PATH}/
ExecStartPre=/bin/chmod 775 ${LOG_PATH}/
Type=simple
Restart=always

[Install]
WantedBy=multi-user.target

Definitely - this has been superceded by this https://github.com/openenergymonitor/emonpi/commit/34e35b50953edac8b2e83d0d9be577768d4d1c44#diff-7f64367c0c9f859e14def7fc1ca940dec7275a63efcb98d51c81ca8ca12f37c7

Has the update run properly?

bwduncan commented 3 years ago

image How amusing my /boot/emonSD file seems to have disappeared! The latest image in my Downloads directory is emonSD-30Oct18.img.xz, so it's almost certainly that.

bwduncan commented 3 years ago

@borpin you are right it seems I had a local branch. However, when I pull the upstream master I get the same service file contents as you: python3, but looking in /opt/openenergymonitor, which does not exist.

There's a missing remote which the update script complains about. I'll fix that and report back.

bwduncan commented 3 years ago

OK that's fixed. No errors from the update script but still nothing looks like it would have created /opt/openenergymonitor for me... Is that expected?

borpin commented 3 years ago

Yes I think so - I think it is trying to move folk off the /home/emonpi folder. It is always best to run an update from the UI rather than do a git pull locally as things are fixed by the update scripts.

bwduncan commented 3 years ago

I'm confused. I clicked "Full Update" in the UI but nothing mentioned /opt/openergymonitor. Are you saying that's to be expected?

Where is the bit of code which is trying to move folk off /home/emonpi? This is my confusion, sorry.

alexandrecuer commented 3 years ago

I think it is this one

https://github.com/openenergymonitor/EmonScripts/blob/master/update/emonsd.sh

borpin commented 3 years ago

Where is the bit of code which is trying to move folk off /home/emonpi? This is my confusion, sorry.

It might be my misunderstanding that that was the case. That script above seems to assume that is the folder but the other script doesn't. The update script hierarchy is a bit impenetrable. - @TrystanLea may be able to shed some light.