Open marcelerkel opened 5 years ago
CrazyIvan359
commented
5 years ago You should use relative links ln - rs to avoid breaking when in/out of the container. Make sure your script's working dir is the same as where the link will be for it to work.
Alternatively, putting the libs inside the container would cause them to be updated when you update the container. You would then be making links to a location only available in the container so wouldn't need relative.
Rick-Jongbloed
commented
5 years ago I haven't been here for a while, as i'm too busy setting up a solid Kubernetes cluster. However today i've been writing my own script exactly for this purpose.
I've got a few remarks:
if [ ! -z ${JYTHON_ALT_DL_URI} ]
then
wget -nv -O /tmp/jython-installer.jar ${JYTHON_ALT_DL_URI}
fi ## Which modules needs to be installed, leave empty to not install anything
ARG PYPI_MODULES="requests python-dateutl json"
[[ ! -z "${PYPI_MODULES}" ]] java -jar /opt/jython/jython.jar -m pip install ${PYPI_MODULES}Another comment regarding the other script:
5iver
commented
5 years ago I don't have the time to review ATM, but this...
Files like 000_startup_delay.py and personal/init.py might be overwritten, which can cause a working system to stop functioning properly.
... is a really good point that I had not thought of. Thank you for bringing this up, @Rick-Jongbloed! I've opened #218 for this.
000_StartupDelay.py should not be an issue, as it is a core file that nobody should be modifying. The configuration.py is distributed as configuration.py.example to avoid overwrites on upgrade.
Rick-Jongbloed
commented
5 years ago OK, i'm testing the jython script out and will post my findings in this post (I don't know how to do these fancy code reviews yet haha)
function download_jython_libraries() {
# Download and install Jython
if [ ! -d "${JYTHON_HOME}" ]; then
mkdir -p "${JYTHON_HOME}"A suggestion from my work on an install script: wget has a check timestamp option. Just run the wget command with it and it will only download if the remote file is newer.
Also look at #213 for a much simpler way to install Jython (and Groovy)
Rick-Jongbloed
commented
5 years ago Also look at #213 for a much simpler way to install Jython (and Groovy)
This will only work if the Jython jar file is the only jar file that needs to be loaded into OpenHAB this way (Which it usually is, but still, we should leave the option open that other jar files could be at that location. I like the way you used the registry file though :-)
CrazyIvan359
commented
5 years ago Credit should go to Scott for that, I just implemented it and and wrote the docs. You can have as many jars in that folder as you like, I see no issue
marcelerkel
commented
5 years ago How are you going to deal with an configuration upgrade when configuration files are changed by the end user? For me this is the main reason an upgrade is not really wanted: Files like 000_startup_delay.py and personal/init.py might be overwritten, which can cause a working system to stop functioning properly.
Like Scott mentioned, the 000_startup_delay.py is a core file and should not be updated by users.
The personal/init.py is never overwritten nor removed. You can even safely delete the entire openhab-libraries-directory without the file being removed as the personal and community directories are not part of any of the underlying directories.
The following files/directories are all kept outside of the helper libraries directory:
/openhab/conf/automation/[jsr223|lib]/python/personal
/openhab/conf/automation/[jsr223|lib]/python/community
/openhab/conf/automation/lib/python/configuration.py (if this file does not exist, it is copied from /openhab/conf/automation/openhab-helper-libraries/Core/automation/lib/python/configuration.py.example)
/openhab/conf/automation/[jsr223|lib]/python/core this is a symlink to /openhab/conf/automation/openhab-helper-libraries/Core/automation/[jsr223|lib]/python/core
So you can safely delete /openhab/conf/automation/openhab-helper-libraries and all that will happen is that the /openhab/conf/automation/lib/python/core symlink is now broken. Restart the container and the latest version of the openhab-helper-libraries-master.zip file is download from Github and installed.
As long as the /openhab/conf/automation/openhab-helper-libraries directory exists, no automatic upgrade of the helper libraries will be done.
For the community libraries that you want to make use of you basically have two options:
/openhab/conf/automation/[jsr223|lib]/python/community directory to the community contribution that you'd like to make use of. This will give you auto update if you delete the /openhab/conf/automation/openhab-helper-libraries and restart the container./openhab/conf/automation/[jsr223|lib]/python/community directory. This requires you to manually update by copying the new version in place when you want to update.Thanks for all the feedback so far! I'll try to incorporate it all tomorrow. To make inline comments go to the top of this page and just below the PR title you see the 'Files changed' "tab". If you click on it then you'll get to see the files. When you hover your mouse over the code a blue + icon will appear. Press it and you can type in your comment.
marcelerkel
commented
5 years ago A suggestion from my work on an install script: wget has a check timestamp option. Just run the wget command with it and it will only download if the remote file is newer.
I'm currently deleting the installer file after Jython has been installed. I don't think it is worth keeping it. Downloading it takes only a few seconds. I do intend to add Rick's suggestion to specify an alternate URI. If that is used I want to add a check to see if it is a download (URI starts with http) or a local file mounted into the container (in the latter case the script must not remove the file after installing because then it would remove the users original file).
Also look at #213 for a much simpler way to install Jython (and Groovy)
I already saw it but wasn't sure if that was the new way of doing the installation or just a suggestion Now that it is clear that that will be the new way I'll try to incorporate that as well.
I appreciate all the feedback!
CrazyIvan359
commented
5 years ago I'm currently deleting the installer file after Jython has been installed. I don't think it is worth keeping it. Downloading it takes only a few seconds.
Why not download the jar directly?
marcelerkel
commented
5 years ago Because I use the full installer which comes with pip and other tools to be able to install additional Python packages.
CrazyIvan359
commented
5 years ago Ah hah. Interesting. I haven't actually looked at the full install yet.
5iver
commented
5 years ago Now that it is clear that that will be the new way I'll try to incorporate that as well.
How is it clear now? 😉 I haven't responded to the PR, but I have serious reservations about changing the install in this way. I'll get to it today.
marcelerkel
commented
5 years ago How is it clear now?
Credit should go to Scott for that, I just implemented it and and wrote the docs. You can have as many jars in that folder as you like, I see no issue
Well I figured it it's your idea and CrazyIvan359 already did the documentation then that must be the way forward :) I'll hold off on that part for now.
5iver
commented
5 years ago IIRC, I had thrown it out to Michael as a possible simplification for use in an install script, but it wasn't thought out (but works!). The OH update script removes the runtime directory, so you'd need to reinstall Jython after every time. Best to stay with EXTRA_JAVA_OPTS for now.
I'm still getting something wrapped up and might have time to take a look at your script later this weekend, if things go well. Until then, is there any possibility to turn your script into something that could be used both with and without Docker?
CrazyIvan359
commented
5 years ago The OH update script removes the runtime directory
I wasn't aware of this before, but in the context of Docker that's the normal lifecycle of a container. For regular installs it's a problem though.
rkoshak
commented
5 years ago I'm concerned about the enable_next_generation_rule_engine function. It seems to assume that the user is only using add-ons.cfg to install bindings. I'm not super good at reading sed expressions but I'll assume it appends to the list instead of replacing the misc entry, which is ok. But if a user is only installing add-ons through the REST API, all of their previously installed misc add-ons will be uninstalled. That's not good.
The script should support both get types of users and not require the use of addons.cfg. it won't be as clean and simple but you can do rest calls using curl to figure out if adding the misc line to add-ons.cfg will be destructive and add it using a rest call.
marcelerkel
commented
5 years ago But if a user is only installing add-ons through the REST API, all of their previously installed misc add-ons will be uninstalled. That's not good.
I tested this just now and see what you mean. That is not good no. I find this very strange behavior, but I guess it is intentional and there is a reason why this works in the way it does. Also, you can't permanently uninstall the Rule Engine. If you uninstall it using Paper UI (which uses the REST API) then it will be removed until openHAB is restarted.
Unfortunately, curl cannot be used either because this script runs before openHAB is started. Hacking the jsondb seems a bit too much. We probably need to stick to documenting that the Rule Engine needs to be installed manually.
marcelerkel
commented
5 years ago ok, addons are not stored in the jsondb. Adding the rule engine to config/org/openhab/addons.config seems to work. I need to give this some thought. For now I will disable this functionality.
rkoshak
commented
5 years ago . I find this very strange behavior, but I guess it is intentional and there is a reason why this works in the way it does.
It has always worked this way and it stems from the fact that the text config files always take precedence over what has been done through PaperUI. As soon as you create an entry in addons.cfg, you need to specify all of your addons for that category in addons.cfg. OH will look at the file and make itself match the file (i.e. installing any missing and uninstalling any not listed).
Also, you can't permanently uninstall the Rule Engine. If you uninstall it using Paper UI (which uses the REST API) then it will be removed until openHAB is restarted.
I'm not sure if I'm understanding correctly, but you mean if you have the rules engine listed in addons.cfg and then uninstall it through PaperUI that it will get reinstalled on a restart of OH? If yes then indeed that is what will happen as openHAB tries to make itself match what is in addons.cfg.
I personally do use addons.cfg (I never got around to switching, probably will at some point). Have you confirmed that addons.config get's populated even when you are not using addons.cfg?
marcelerkel
commented
5 years ago I'm not sure if I'm understanding correctly, but you mean if you have the rules engine listed in addons.cfg and then uninstall it through PaperUI that it will get reinstalled on a restart of OH? If yes then indeed that is what will happen as openHAB tries to make itself match what is in addons.cfg.
Your understanding is correct.
I've come to the conclusion that enabling the Rule Engine via script is a no go. There are simply to many possibilities. It will be much clearer for a user to always manually enable the Rule Engine than it will be for him/her to understand under which circumstances the Rule Engine is automatically enabled or needs to be manually enabled.
Have you confirmed that addons.config get's populated even when you are not using addons.cfg?
Yes. If you enable the Rule Engine addon via PaperUI it is added to the $OPENHAB_USERDATA/config/org/openhab/addons.config file.
marcelerkel
commented
5 years ago Based on the feedback received I have updated both scripts. The only thing that I did not change is the check whether or not Jython is installed based on the existence of the $JYTHON_HOME directory. The complexity that this would add is in my opinion not worth it. The current test is unambiguous; when the directory exists then jython does not need to be installed, when it does not exist jython needs te be installed.
marcelerkel
commented
5 years ago Forgot to mention in the commit. When installing the jython libraries on either an Alpine based image or ARM architecture then pip is not installed (see jythontools/jython#108 as mentioned by Rick). When using the JYTHON_INSTALLER_OPTIONS variable this test is disabled. So if you build your own jython-installer.jar from the master branch, then simply set the installer configuration parameters to "-t standard -e demo doc src" to get the default behaviour and pip will be installed.
Rick-Jongbloed
commented
5 years ago @marcelerkel nice one, i've implemented your scripts and these are my findings for the JSR232 script:
java -jar ${JYTHON_HOME}/../jython-installer/jython-installer.jar -s -d ${JYTHON_HOME}/ -t standard -e demo doc src
java -jar ${JYTHON_HOME}/jython.jar -m pip install requests python-dateutilI think ensure pips is executed in the standard installation. See https://wiki.python.org/jython/JythonReleaseNotes
Jython 2.7rc2 Bugs fixed
- [ 2301 ] time.strftime now always returns a bytestring (fixes pip on Japanese locales)
- [ 2297 ] Updates Jython installer to use jython.exe
- [ 2298 ] Fix setuptools wheel bundled by ensurepip so it checks for Windows on Jython
- [ 2300 ] Fix bin/jython.py to not consume subcommand args New features > - Installer installs pip, setuptools by default, but custom builds can de-select. Does not change standalone usage. (Runs jython -m ensurepip as last install step.)
- Makes jython.py be the default launcher (as bin/jython) if CPython 2.7 is available.
Rick-Jongbloed
commented
5 years ago These are my findings for the upgrade script:
# declare -A LANGUAGES=( ["groovy"]="groovy" ["javascript"]="js" ["python"]="py" )
declare -A LANGUAGES=( ["python"]="py" )Thanks for updating the scripts!
rkoshak
commented
5 years ago @Rick-Jongbloed , the original submission did add to addons.cfg. The problem with that is you can't just blindly add any entry to that file because if the user doesn't use addons.cfg and installs everything through PaperUI, OH will uninstall everything else in the MISC category that the user has installed through PaperUI because addons.cfg always takes precedence. If you use addons.cfg at all, you have to use it for everything.
Marc determined that trying to work around this is far too complex and removed that from the script.
marcelerkel
commented
5 years ago You're not using a .sh extension on the scripts, therefore VScode is not properly detecting the script type.
It seems to be convention to not use a filename extension for cont-init.d scripts as can be seen in the s6-overlay github repo (https://github.com/just-containers/s6-overlay#executing-initialization-andor-finalization-tasks) and also non of the examples in the openhab-docker repo use filename extensions (https://github.com/openhab/openhab-docker/tree/master/contrib/cont-init.d).
This is similar to init scrips in /etc/init.d where the majority of the scripts do not have an extension.
Please define Jython_home at the top of the script so it's usable in the JYTHON_INSTALLER_URI path :-)
The JYTHON_HOME variable cannot be used in the JYTHON_INSTALLER_URI because that would mean that the path that JYTHON_HOME points to would already exist. That would bring us back to needing a different way to test if Jython is already installed, if so, which version, if the version differs, then remove it and its libraries in Lib, etc., etc.
I think this is very specific for your use case, 99.999% of the users of this script won't be affected by this.
My alternative Jython location is already local and not online, if defining an custom(alternative) installer file, why copy this to temp? You can install from the original location and just leave the file as it is?
My reasoning was that this way the script wouldn't need to keep track of how the installer jar ended up on the users system, what its filename was (always /tmp/jython-installer.jar) and whether or not it should be deleted once installed. However, I have updated this so that it no longer copies and deletes the file when it is local and not downloaded.
Typo: echo "ERROR: Failed to copy the helper libraries form
Thanks, fixed.
I don't like the workings of the function check_next_generation_rule_engine. If you can download and install this script, you should be able to edit addons.cfg yourself.
I'm not sure what you're trying to say here. All the check_next_generation_rule function does is check if the next gen rule engine is enabled or not and inform the user about it. If the next gen rule engine is not enabled then the script does not care how the user enables it, either via addons.cfg, REST API or by editing the addons.config. It just gives a hint where to find the information about how to enable it.
Anyway, I'm not too happy with this myself either. The script should enable the next gen rule engine if possible.
There are a number of problems though:
addons.cfg overrules addons.config (addons.config is where openHAB stores the addons, bindings, etc. that were enabled via the REST API). If addons are configured in both addons.cfg and addons.config then those configured in addons.config but not in addons.cfg will be uninstalled when openHAB starts.addons.config file does not exist yet. The script can't just add the next gen rule engine to addons.cfg because the user may (and probably will) use PaperUI/Habmin to install addons (if any).misc parameter in these files can be empty (I don't use any addons except for the next gen rule engine) so based on the misc parameter the script may not be able to tell which of the two files the user is using.I've redesigned the script so that it will try to enable the next gen rule engine automatically using the following algorithm:
addons.config exists and contains entries in the misc parameter then add ruleengine to list of addons in addons.config (if necessary)addons.cfg contains entries in the misc parameter then add ruleengine to list of addons in addons.cfg (if necessary)addons.config exists and contains entries in the binding parameter then add ruleengine to list of addons in addons.config (if necessary)addons.cfg contains entries in the binding parameter then add ruleengine to list of addons in addons.cfg (if necessary)Are you sure you need to add 'ensurepip'? In my current version this is sufficient.
Yes, to exclude pip from being installed on architectures/images on which it fails to install, e.g. 2.7.1 on Raspberry Pi or the Alpine based image on AMD64.
I don't understand why you write to /tmp first. I would unzip directly to the installation directory.
The script is designed so that it will only make changes to the users system when all prerequisites are met and that includes downloading and successfully unzipping the zip file. If any of this fails then the script will exit and no changes to the users system are made. So in case of failure the users system isn't in some halve installed state. It's either installed/updated or not changed.
I believe the function check_next_generation_rule_engine doesn't belong here. See my comment on the previous post.
See my previous answer :)
Perhaps a better way of configuring this (like with booleans js=true, py=true, etc)
Not in the way that I use it.
for LANGUAGE in "${!LANGUAGES[@]}"; do
if [ ! -f "${OPENHAB_AUTOMATION}/lib/${LANGUAGE}/configuration.${LANGUAGES[$LANGUAGE]}" ]; then
cp "${OPENHAB_HL_AUTOMATION}/lib/${LANGUAGE}/configuration.${LANGUAGES[$LANGUAGE]}.example" "${OPENHAB_AUTOMATION}/lib/${LANGUAGE}/configuration.${LANGUAGES[$LANGUAGE]}"
fi
doneThe error "ERROR: File or directory with name core already exists." Should say lib/=language=/core or jsr232/=language=/core.
I'll fix this.
Possibly remove the -x from the shebang for production use.
I removed the -x. I don't think this will make any difference because the script is sourced by the parent script which also has -x set. As such, it inherits this from the parent.
I need to do some more testing and will update the PR (probably early next week).
rkoshak
commented
5 years ago This is similar to init scrips in /etc/init.d where the majority of the scripts do not have an extension.
And you can't use the .sh extension for scripts you put into /etc/cron.daily (for example). Took me a long time figure out why my cron scripts weren't running because of that one.
I've redesigned the script so that it will try to enable the next gen rule engine automatically using the following algorithm:
Wouldn't you want to check the entries in the cfg file first?
Consider this scenario. I've installed a bunch of stuff including something from the misc category though addons.cfg.
Later I come along and decide to use this script. Because I've run before, addons.conf will have an entry for misc in addons.conf. Thus only 1 will run and ruleengine will only be added to addons.conf.
Then OH starts again and addons.cfg will get loaded and wipe out the change to addons.conf because ruleengine isn't there.
At a minimum I would expect you to need to do just if instead of else if for 2 if you can't change the order of the checking.
marcelerkel
commented
5 years ago Wouldn't you want to check the entries in the cfg file first?
Consider this scenario. I've installed a bunch of stuff including something from the misc category though addons.cfg.
Later I come along and decide to use this script. Because I've run before, addons.conf will have an entry for misc in addons.conf. Thus only 1 will run and ruleengine will only be added to addons.conf.
I was under the impression that if addons were configured in addons.cfg that they would not automagically appear in addons.conf but I was wrong (I confirmed this by testing).
So I will swap the order of 1 and 2, and also the order of 3 and 4 so that misc has precedence over bindings and addons.cfg over addons.config:
marcelerkel
commented
4 years ago I'm going to remove the enabling and verification of the NGRE. I simply do not fully understand how this works and as such I find it too dangerous to automatically enable the NRGE by simply modifying either addons.cfg or addons.config.
While testing I ran into a failure situation:
| bindings | misc | bindings | misc | Result |
|---|---|---|---|---|
| ok | ||||
| myopenhab | ok | |||
| zwave | ok | |||
| zwave | myopenhab | ok | ||
| myopenhab | fail |
(left bindings/misc is addons.config, right bindings/misc is addons.cfg)
In case of the failed test case, the misc line was removed from addons.config and the openHAB Cloud connector is not installed. However, the NRGE was (according to PaperUI) installed but cannot be uninstalled and since PaperUI thinks it's already installed it cannot be reinstalled either (restarting openHAB makes no difference). This means that the script could break the user's system. May be clearing tmp and cache will resolve the situation, but to me this simply isn't good enough.
Since apparently openHAB stores this information somewhere else as well, the script cannot rely on just these two configuration files to provide this info hence why I will also remove the verification of whether or not the NGRE is enabled.
marcelerkel
commented
4 years ago @openhab-5iver I need to check if this is still necessary, but most likely will not get to it before xmas. Next to my full time job I'm studying to obtain my Software Engineering BSc degree in the evenings/weekends, hence my absence here and on the forums lately (I kind of forgot about that I still needed to update this PR). I'm in my final year and hope to get my diploma at the end of June in 2020 so that I can get my life back :)
5iver
commented
4 years ago No worries from my side! I have a pile of PRs to review and this was first on the list. I think this PR will be obsolete, except for people who want a full install of Jython. I have an idea that might help with that... a configuration option in the addon to manually set python.home. Get back to studying and quit playing with OH!
wertzui
commented
4 years ago Is there any progress on this? I would really like an easy way to use Jython inside my dockerized openHAB :)
5iver
commented
4 years ago @wertzui, this script will be pretty much obsolete when the Jython bundle gets merged...
Two scripts cont.init.d scripts for the official openHAB Docker image.
The
10-jsr-jythonscript performs the following actions:jython-installer.jarif not installed already.*$py.classfiles upon container startup.EXTRA_JAVA_OPTSenvironment variable updated for Jython.You no longer need to build your own docker container based on the official openHAB image.
Minor issue: The
jython-installer.jarseems to have a problem when using the Alpine image when it installs PIP. When using the Debian image all works ok. I need to further investigate what is going wrong and may simply work around it by using thejython-standalone.jarwhen the script is run on the Alpine image.The
10-openhab-helper-librariesscript performs the following actions:configuration.[groovy|js|py]in place if it does not exist.This will script will download the helper libraries to the
/openhab/automation/openhab-helper-librariesdirectory when this directory does not exist. Before making any changes, the script does a sanity check to make sure that the required changes can be made. In case of a problem the script will abort before making any changes to the system.Updating the helper libraries is as easy as deleting the
/openhab/automation/openhab-helper-librariesdirectory and restarting the container.The script creates the following directory structure (where 'core' is a symlink the the indicated core directory in the openhab-helper-library):
/openhab/conf/automation/jsr223/groovy/community/openhab/conf/automation/jsr223/groovy/core -> /openhab/conf/automation/openhab-helper-libraries/Core/automation/jsr223/groovy/core/openhab/conf/automation/jsr223/groovy/personal/openhab/conf/automation/jsr223/javascript/community/openhab/conf/automation/jsr223/javascript/core -> /openhab/conf/automation/openhab-helper-libraries/Core/automation/jsr223/javascript/core/openhab/conf/automation/jsr223/javascript/personal/openhab/conf/automation/jsr223/python/community/openhab/conf/automation/jsr223/python/core -> /openhab/conf/automation/openhab-helper-libraries/Core/automation/jsr223/python/core/openhab/conf/automation/jsr223/python/personal/openhab/conf/automation/lib/groovy/community/openhab/conf/automation/lib/groovy/configuration.groovy/openhab/conf/automation/lib/groovy/core -> /openhab/conf/automation/openhab-helper-libraries/Core/automation/lib/groovy/core/openhab/conf/automation/lib/groovy/personal`/openhab/conf/automation/lib/javascript/community/openhab/conf/automation/lib/javascript/configuration.js/openhab/conf/automation/lib/javascript/core -> /openhab/conf/automation/openhab-helper-libraries/Core/automation/lib/javascript/core/openhab/conf/automation/lib/javascript/personal/openhab/conf/automation/lib/python/community/openhab/conf/automation/lib/python/configuration.py/openhab/conf/automation/lib/python/core -> /openhab/conf/automation/openhab-helper-libraries/Core/automation/lib/python/core/openhab/conf/automation/lib/python/personal/openhab/conf/automation/openhab-helper-librariesTo use the community scripts you either create a symlink or copy the files into the
/openhab/conf/automation/[jsr223|lib]/python/communitydirectory. Both thecommunityandpersonaldirectories have group write permissions so that if you add your own user to theopenhabgroup on the Docker host you have write permissions in these directories.When using a different directory structure on the Docker host compared to the one inside the container then the symlink to the
coredirectory in the openhab-helper-libraries will be broken. However, I think that is a small price to pay for the ease of updating the libraries. Just delete theopenhab-helper-librariesdirectory, restart the container and the latestopenhab-helper-libraries-master.zipfile will automatically be downloaded and installed.I have created two scripts so that those who don't want to use Python don't need to install the Jython libraries and those you want to use Python and not the openHAB Helper Libraries can do so as well. Each script can be used without using the other. There are not dependencies.
Build and tested on 2.5.0-SNAPSHOT Build #1654
When excepted as the default method for installing the helper libraries on Docker then I will of course update the documentation.