[automation] Sort by filename instead of path #724

5iver · 2019-04-15T04:18:12Z

Currently, the scripts are loaded by ScriptFileWatcher based on the lexicographical order of the absolute paths of the scripts. This makes it difficult to control the load order. This change bases the load order solely on the filename, as was originally used before ESH/#3855, and preserves the ability to use scripts with the same filename. This PR resolves openhab-scripters/openhab-helper-libraries#76.

Current load order

After this PR

Scripts will be loaded based on the filename. If more than one script have the same filename, their paths will also be taken into account for the sorting. This is especially helpful with the direcotry structure adopted by the Jython helper libraries. Currently, Community scripts (in a community directory), are being loaded before the Core scripts (in a core directory).

Directory structure and scripts

├── /automation/sr223
│   ├── 00
│   │   ├── 00
│   │   │   ├── 00_script.py
│   │   │   ├── 01_script.py
│   │   │   └── another.py
│   │   ├── 00_script.py
│   │   ├── 01
│   │   │   ├── 00_script.py
│   │   │   ├── 01_script.py
│   │   │   └── another.py
│   │   ├── 01_script.py
│   │   └── another.py
│   ├── 00_script.py
│   ├── 01
│   │   ├── 00
│   │   │   ├── 00_script.py
│   │   │   ├── 01_script.py
│   │   │   └── another.py
│   │   ├── 00_script.py
│   │   ├── 01
│   │   │   ├── 00_script.py
│   │   │   ├── 01_script.py
│   │   │   └── another.py
│   │   ├── 01_script.py
│   │   └── another.py
│   ├── 01_script.py
│   ├── another.py

Load order

/00_script.py
/00/00_script.py
/00/00/00_script.py
/00/01/00_script.py
/01/00_script.py
/01/00/00_script.py
/01/01/00_script.py
/01_script.py
/00/01_script.py
/00/00/01_script.py
/00/01/01_script.py
/01/01_script.py
/01/00/01_script.py
/01/01/01_script.py
/another.py
/00/another.py
/00/00/another.py
/00/01/another.py
/01/another.py
/01/00/another.py
/01/01/another.py

@lewie, I'm curious what your thoughts are about this change.

Signed-off-by: Scott Rushworth openhab@5iver.com

Currently, the scripts are loaded based on the lexicographical order of the absolute paths of the scripts. This makes it difficult to control the load order. This change bases the load order solely on the filename, as was originally used before eclipse-archived/smarthome#3855, and preserves the ability to use scripts with the same filename. Signed-off-by: Scott Rushworth <openhab@5iver.com>

maggu2810 · 2019-04-15T12:24:05Z

I cannot judge, which sort order makes most sense, but I would expect (what reason ever) a sort order like:

function load(dir):
* sort all files in the current directory by name and load file by file
* sort all directories in the current directory by name and call "load(subdir)" for each of them

So, go down the directory hierarchy after the files of that directory has been checked.

Your example:

/00_script.py
/01_script.py
/another.py
/00/00_script.py
/00/01_script.py
/00/another.py
/00/00/another.py
/00/00/00_script.py
/00/00/01_script.py
/00/01/another.py
/00/01/00_script.py
/00/01/01_script.py
/01/00_script.py
/01/01_script.py
/01/another.py
/01/00/00_script.py
/01/00/01_script.py
/01/00/another.py
/01/01/00_script.py
/01/01/01_script.py
/01/01/another.py

But I am not an user of the scripting engine, yet.

So perhaps there could be some answers of other ones.

5iver · 2019-04-15T21:45:41Z

A script loading order based on the directory structure (absolute path) is a lot harder to work with. Currently, with the load order based on the absolute path, the only way to have certain scripts load before any others, is to manipulate the directory names, or put them in the root directory and manipulate the file name. So, scripts in /000_Startup_Scripts/ would load before scripts in directories that are not prefixed with a number. Sorting by absolute paths also means that I cannot distribute a solution that you can just copy into /automation/jsr223/, if it contains anything that has a specific load order, because I do not know what your directory structure is.

This PR allows for me to distibute solutions that are contained in their own directories, which can be easily copy/pasted, and the load order, relative to other directories that have been distributed, are preserved because the load order is based off of the file name. The openhab-jython repo relies on a specific directory structure. Without this PR, users will have to modify the directory and file names so that the script load order works properly with their implementation. Specific example...

Currently, the owm_daily_forecast.py will load first, and fail, since the 000_starup_delay.py is required to run first and delay the loading of all other scripts until the environment is full loaded. The script will also fail since it uses a module that depends on 200_JythonItemProvider.py being loaded.

├── automation/jsr223
│   ├── core
│   │   ├── 000_startup_delay.py
│   │   └── components
│   │   ├── 200_JythonBindingInfoProvider.py
│   │   ├── 200_JythonExtensionProvider.py
│   │   ├── 200_JythonItemChannelLinkProvider.py
│   │   ├── 200_JythonItemProvider.py
│   │   ├── 200_JythonThingProvider.py
│   │   ├── 200_JythonThingTypeProvider.py
│   │   ├── 200_JythonTransform.py
│   │   ├── 300_DirectoryEventTrigger.py
│   │   ├── 300_OsgiEventTrigger.py
│   │   ├── 300_ShutdownTrigger.py
│   │   └── 300_StartupTrigger.py
│   ├── community
│   │   ├── openweathermap
│   │   │   └── owm_daily_forecast.py

In reading through this several times, it all still looks like it would be confusing for someone who is not using scripted automation. I'll see if I can't drum up some others to help communicate the benefits of this change.

openhab-bot · 2019-04-15T22:06:13Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/proposed-change-to-the-jsr223-script-loading-order/72548/1

mjcumming · 2019-04-16T01:32:23Z

We definitely need a reliable way to ensure scripts are loaded in the correct order. The proposed naming convention solves this problem without worrying about directory names or structure.

maggu2810 · 2019-04-17T16:43:45Z

Don't get me wrong, I don't argue for a specific sorting algorithm, I just want to understand why the one is better then the other.

A script loading order based on the directory structure (absolute path) is a lot harder to work with. Currently, with the load order based on the absolute path, the only way to have certain scripts load before any others, is to manipulate the directory names, or put them in the root directory and manipulate the file name. So, scripts in /000_Startup_Scripts/ would load before scripts in directories that are not prefixed with a number. Sorting by absolute paths also means that I cannot distribute a solution that you can just copy into /automation/jsr223/, if it contains anything that has a specific load order, because I do not know what your directory structure is.

Hm, I don't understand.
What is a "solution" that you distribute?
Why is it not possible to distribute if an absolute path based sorting is used that respect directory names and file names?
If we distribute all the scripts you need, you can ensure the load order by its file names and they just need to be added in the same directory.
You could also use different directories to group all your stuff and create an archive of this directory structure.
If you rely on other stuff that you don't distribute, why should it work using file names only -- you don't know how the files are named on a specific system. Or do you know file names someone is using but not directory names?
What's the difference between directory names and file names?

This PR allows for me to distibute solutions that are contained in their own directories, which can be easily copy/pasted, and the load order, relative to other directories that have been distributed, are preserved because the load order is based off of the file name. The openhab-jython repo relies on a specific directory structure. Without this PR, users will have to modify the directory and file names so that the script load order works properly with their implementation. Specific example...

Currently, the owm_daily_forecast.py will load first, and fail, since the 000_starup_delay.py is required to run first and delay the loading of all other scripts until the environment is full loaded. The script will also fail since it uses a module that depends on 200_JythonItemProvider.py being loaded.

Same here.
You state that the correct file names needs to be used otherwise the loading order will not work.
So the file name must not be changed.
Same can be true for directory names.

If my script is named 000a_foo.py it will be loaded first.
If my script is names 100_foo.py it will be loaded in front of the 200 ones.

You could also state that the script you provide needs to be placed in a directory "000" and the user must not use the root directory of jsr223 but a directory names that is lexically ordered after "000".

5iver · 2019-04-18T05:59:17Z

Don't get me wrong, I don't argue for a specific sorting algorithm, I just want to understand why the one is better then the other.

I understand your position. You want to be certain that the best solution is chosen, but don't have experience with scripted automation. You are the gatekeeper and we need and appreciate you!

What is a "solution" that you distribute?

The openhab2-jython repo contains community provided modules and scripts, which a user can install by "cherry picking" them out of the directory structure and copying to their OH installation. This is just a stepping stone until we have an automation extension service. The organization is about to be renamed to openHAB Scripters, and separate repos will be created for each scripting language and their helper libraries, using the same directory format.

In a test environent, setup the Jython helper libraires and scripts and you might see what I mean. I have not added much to the doc for the Community stuff, mainly because it doesn't work due to the current load order. I've also held up submitting LOTS of examples until we get something like this PR merged. The alternative is to add numeric prefixes to the directory names, which is rather ugly and does not solve the issue in all cases.

Say that we have renamed the core script directory with a numeric prefix so that these scripts will load first using the current load order. But there is a community script that needs to load in the middle of the core scripts...

├── automation/jsr223
│ ├── 000_core
│ │ ├── 01_loadme_first.py
│ │ ├── 03_loadme_third.py
│ ├── community
│ │ ├── 02_loadme_second.py

With the current load order, these scripts would load...

/000_core/01_loadme_first.py
/000_core/02_loadme_first.py
/community/02_loadme_second.py

There is currently no way to interweave the load order of scripts from separate directories. This PR provides this functionality by sorting by file name.

...org/openhab/core/automation/module/script/rulesupport/internal/loader/ScriptFileWatcher.java

maggu2810

As there is no one that complains about the changed loading order and if it is better for you...

openhab-bot · 2019-06-03T22:20:26Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/preparation-for-2-5m2/75738/1

openhab-bot · 2019-09-15T16:19:19Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/problems-after-install-of-jsr223-scripts/81969/2

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

Currently, the scripts are loaded based on the lexicographical order of the absolute paths of the scripts. This makes it difficult to control the load order. This change bases the load order solely on the filename, as was originally used before eclipse-archived/smarthome#3855, and preserves the ability to use scripts with the same filename. Signed-off-by: Scott Rushworth <openhab@5iver.com> GitOrigin-RevId: 77992e7

5iver mentioned this pull request Apr 15, 2019

At OH start, scripts beneath $OPENHAB_CONF/automation/jsr223/community/ run before those under jsr223/core/ openhab-scripters/openhab-helper-libraries#76

Closed

wborn added the automation label Apr 16, 2019

maggu2810 reviewed Apr 18, 2019

View reviewed changes

...org/openhab/core/automation/module/script/rulesupport/internal/loader/ScriptFileWatcher.java Show resolved Hide resolved

maggu2810 reviewed Apr 18, 2019

View reviewed changes

...org/openhab/core/automation/module/script/rulesupport/internal/loader/ScriptFileWatcher.java Show resolved Hide resolved

maggu2810 approved these changes Apr 18, 2019

View reviewed changes

maggu2810 merged commit 77992e7 into openhab:master Apr 18, 2019

5iver deleted the sort_by_filename branch April 18, 2019 18:25

5iver mentioned this pull request Apr 18, 2019

Update script load order openhab/openhab-docs#943

Merged

maggu2810 mentioned this pull request Apr 26, 2019

[Automation] Absolute datetime module type #683

Closed

5iver mentioned this pull request May 29, 2019

[automation] Scripts do not load at startup on Windows #841

Closed

wborn added this to the 2.5 milestone Jul 30, 2019

cweitkamp changed the title ~~[JSR223] Sort by filename instead of path~~ [automation] Sort by filename instead of path Dec 7, 2019

cweitkamp added the enhancement An enhancement or new feature of the Core label Dec 7, 2019

kaikreuzer removed the enhancement An enhancement or new feature of the Core label Dec 13, 2019

Rosi2143 pushed a commit to Rosi2143/openhab-core that referenced this pull request Dec 26, 2020

[configuration] Revert openhab#723 partially (openhab#724)

1f46424

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[automation] Sort by filename instead of path #724

[automation] Sort by filename instead of path #724

5iver commented Apr 15, 2019 •

edited

maggu2810 commented Apr 15, 2019 •

edited

5iver commented Apr 15, 2019

openhab-bot commented Apr 15, 2019

mjcumming commented Apr 16, 2019

maggu2810 commented Apr 17, 2019

5iver commented Apr 18, 2019 •

edited

maggu2810 left a comment

openhab-bot commented Jun 3, 2019

openhab-bot commented Sep 15, 2019

[automation] Sort by filename instead of path #724

[automation] Sort by filename instead of path #724

Conversation

5iver commented Apr 15, 2019 • edited

Current load order

After this PR

maggu2810 commented Apr 15, 2019 • edited

5iver commented Apr 15, 2019

openhab-bot commented Apr 15, 2019

mjcumming commented Apr 16, 2019

maggu2810 commented Apr 17, 2019

5iver commented Apr 18, 2019 • edited

maggu2810 left a comment

Choose a reason for hiding this comment

openhab-bot commented Jun 3, 2019

openhab-bot commented Sep 15, 2019

5iver commented Apr 15, 2019 •

edited

maggu2810 commented Apr 15, 2019 •

edited

5iver commented Apr 18, 2019 •

edited