Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

At OH start, scripts beneath $OPENHAB_CONF/automation/jsr223/community/ run before those under jsr223/core/ #76

Closed
ScottKinSF opened this issue Jan 23, 2019 · 16 comments · Fixed by openhab/openhab-core#724
Labels
OH dependency Depends on OH bug or enhancement

Comments

@ScottKinSF
Copy link
Contributor

ScottKinSF commented Jan 23, 2019

When OH is started, any scripts under $OPENHAB_CONF/automation/jsr223/community/ are loaded and run before any script under $OPENHAB_CONF/automation/jsr223/core/, most notably jsr223/core/000_startup_delay.py.

Logs from my recent OH start:

2019-01-23 08:02:14.328 [INFO ][pool-1-thread-1][rt.internal.loader.ScriptFileWatcher] - Loading script 'community/openweathermap/owm_daily_forecast.py'
2019-01-23 08:02:27.480 [INFO ][pool-1-thread-1][rt.internal.loader.ScriptFileWatcher] - Loading script 'core/000_startup_delay.py'
2019-01-23 08:03:37.709 [INFO ][pool-1-thread-1][rt.internal.loader.ScriptFileWatcher] - Loading script 'core/components/100_DirectoryTrigger.py'

Possible fixes (untested):

  • Change the name of community to something like zzz_community
  • Move community/ to a new directory immediately below jsr223: jsr223/local/community.
@mjcumming
Copy link

@ScottKinSF What about just having a python(jython) script that the jython interpreter loads automatically and have that script load the various directories and modules under user control rather than relying on specific jython versions that seem to behave differently?

@ScottKinSF
Copy link
Contributor Author

@mjcumming, my reading of your comment makes me think you may be using a Jython jar other than the recommended jython-standalone-2.7.0.jar. If so, what version do you have installed in $OPENHAB_CONF/automation/jython, also, what load order do you see on your system?

@5iver
Copy link
Member

5iver commented Jan 24, 2019

Nice catch @ScottKinSF ! Can you please confirm that you are using 2.7.0? Have you seen my comments about 2.7.1? I've been restarting OH a lot, and noticed the OWM script running first, and I was attributing that to the updated Jython, but your findings indicate that the directory names ARE considered with 2.7.0. The initial documentation mentioned this (maybe @steve-bate could give us a hint as to the bug mentioned, or if/where OH is loading the scripts), but the OH doc claims directory names are not considered. In testing this early on, I was certain the directory names were not considered, so I removed it. So I screwed up, or something changed. This is unfortunate, since we just did the restructuring. My initial fix was to rename /jsr223/core to /jsr223/000_core.

Here is another related development, which would eliminate the need for all/most of the core modules, but that will be a while, and won't fix anything for any core modules that are not absorbed into the API.

@mjcumming , that does sound like a possibility, but I think it would be messy to put it in the Java, since not everyone uses these modules. For your solution, maybe we could put the startup delay script at the root of /jsr223/, and have it load the other component scripts, rather than renaming the core directory.

Let's look into whether it is OH or Jython that is determining the load order of the scripts.

@mjcumming
Copy link

@openhab-5iver I was thinking a .py script that loaded modules/scripts/components in directories not loaded by jython so we do not have to worry about relying on directory and file names to control the order of loading.

@5iver
Copy link
Member

5iver commented Jan 24, 2019

I was thinking a .py script

I could be wrong, but I'm pretty sure scripts have to be under /automation/ for OH to load them.

@mjcumming
Copy link

Probably not explaining what I was thinking....

If we only had Jython load one script in the automation folder, that script could load other modules in the order desired without worrying about naming conventions and alphabetical order. Ie. I have a script that loads all of the other scripts its needs to run - it doesn't rely on the Jython to load those scripts.

@ScottKinSF
Copy link
Contributor Author

Can you please confirm that you are using 2.7.0?

Confirmed, my installation currently uses jython-standalone-2.7.0.jar, SHA1 checksum: cdfb38bc6f8343bcf1d6accc2e1147e8e7b63b75.

@ScottKinSF
Copy link
Contributor Author

ScottKinSF commented Jan 24, 2019

the OH doc claims directory names are not considered

In my reading of the OH doc, I don't see such a claim. The doc doesn't seem to say anything about the ordering of the directory traversal during script loading other than it is top down. It would make sense to me that whatever process is performing the traversal and script loading has to have a repeatable method for sequencing through directories that are siblings at any given level of the $OPENHAB_CONF/automation/jsr223/ hierarchy, which to me would reasonably be directory names according to the ASCII collation sequence (case sensitive?).

[Update]
I just reread the first statement in the referenced OH doc link, and it does seem to be somewhat ambiguous: At system start time the scripts will be loaded in an order based on their name and then top-down through the directory hierarchy. From my experience, that statement is incorrect.

As you noted above, to fully understand the JSR223 script loading sequence, we need to find where the initial loading of scripts takes place.

@ScottKinSF
Copy link
Contributor Author

ScottKinSF commented Jan 24, 2019

Loading of JSR223 scripts happens in smarthome/bundles/automation/org.eclipse.smarthome.automation.module.script.rulesupport/src/main/java/org/eclipse/smarthome/automation/module/script/rulesupport/internal/loader/ScriptFileWatcher.java. The script files queued for loading are sorted using a case insensitive sort on their complete path (includes the file name at the end of the string) relative to $OPENHAB_CONF/automation/jsr223, which accounts for the ordering by directory (path.)

@5iver
Copy link
Member

5iver commented Jan 25, 2019

The text at the bottom made it less ambiguous for me...

the load order will be: 00script.py, 01/script1.py, script2.py, 01/scriptx.py

I'll start looking into this... I just wrapped up adding Jython and Groovy script actions in Paper UI, but waiting for ESH migration before submitting!

@ScottKinSF
Copy link
Contributor Author

ScottKinSF commented Jan 25, 2019

Based on my inspection of the relevant Java source code, that text in the documentation is in error. If the files under automation/jsr223/ are as shown in the example:

automation/jsr223/
  01/
    script1.py
    scriptx.py
  00script.py
  script2.py

the load order will be 00script.py, 01/script1.py, 01/scriptx.py, script2.py. Easy enough to test whether or not my reading of the Java source is correct.

@5iver
Copy link
Member

5iver commented Jan 25, 2019

I agree. The OH doc was originally submitted with that wording on 7/14/17, but there was a change on 7/24/17 that added the inclusion of the directory name, but the OH doc was never updated. It puzzles me how the old code would have prevented the loading of scripts with the same name.

The quick ugly fix is to prefix the directories with a number. The better fix, IMO, is to have ScriptFileWatcher.checkFiles() sort the scripts by filename, ignoring the directory name. But I'm still not fully committed to that.

@ScottKinSF
Copy link
Contributor Author

I like your preferred fix, sorting on file names. That is much easier to explain such that any user can understand the script loading order.

@5iver
Copy link
Member

5iver commented Apr 7, 2019

I'm updating the docs for something else, and remebered this still needed to be corrected, so I put together some test scripts. These files and directory structure...

.
├── 00
│   ├── 00
│   │   ├── 00_00_00.py
│   │   └── 01_00_00.py
│   ├── 00_00.py
│   ├── 01
│   │   ├── 00_00_01.py
│   │   └── 01_00_01.py
│   └── 01_00.py
├── 00.py
├── 01
│   ├── 00
│   │   ├── 00_01_00.py
│   │   └── 01_01_00.py
│   ├── 00_01.py
│   ├── 01
│   │   ├── 00_01_01.py
│   │   └── 01_01_01.py
│   └── 01_01.py
└── 01.py

... load in this order...

ScriptFileWatcher] - Script loaded: 00.py
ScriptFileWatcher] - Script loaded: 00/00/00_00_00.py
ScriptFileWatcher] - Script loaded: 00/00/01_00_00.py
ScriptFileWatcher] - Script loaded: 00/00_00.py
ScriptFileWatcher] - Script loaded: 00/01/00_00_01.py
ScriptFileWatcher] - Script loaded: 00/01/01_00_01.py
ScriptFileWatcher] - Script loaded: 00/01_00.py
ScriptFileWatcher] - Script loaded: 01.py
ScriptFileWatcher] - Script loaded: 01/00/00_01_00.py
ScriptFileWatcher] - Script loaded: 01/00/01_01_00.py
ScriptFileWatcher] - Script loaded: 01/00_01.py
ScriptFileWatcher] - Script loaded: 01/01/00_01_01.py
ScriptFileWatcher] - Script loaded: 01/01/01_01_01.py
ScriptFileWatcher] - Script loaded: 01/01_01.py

I have a couple OH PRs to wrap up, and will then put in a change to ScriptFileWatcher so that just the names of the files are considered. They would then load in this order...

ScriptFileWatcher] - Script loaded: 00.py
ScriptFileWatcher] - Script loaded: 00/00_00.py
ScriptFileWatcher] - Script loaded: 00/00/00_00_00.py
ScriptFileWatcher] - Script loaded: 00/01/00_00_01.py
ScriptFileWatcher] - Script loaded: 01/00_01.py
ScriptFileWatcher] - Script loaded: 01/00/00_01_00.py
ScriptFileWatcher] - Script loaded: 01/01/00_01_01.py
ScriptFileWatcher] - Script loaded: 01.py
ScriptFileWatcher] - Script loaded: 00/01_00.py
ScriptFileWatcher] - Script loaded: 00/00/01_00_00.py
ScriptFileWatcher] - Script loaded: 00/01/01_00_01.py
ScriptFileWatcher] - Script loaded: 01/01_01.py
ScriptFileWatcher] - Script loaded: 01/00/01_01_00.py
ScriptFileWatcher] - Script loaded: 01/01/01_01_01.py

@5iver
Copy link
Member

5iver commented Apr 15, 2019

If anyoe has a minute, I've submitted a PR to change the load order and could use some help communicating the need for the change... openhab/openhab-core#724.

@5iver
Copy link
Member

5iver commented Apr 18, 2019

Closed in openhab/openhab-core#724. Wahoo!

@5iver 5iver closed this as completed Apr 18, 2019
@5iver 5iver mentioned this issue May 7, 2019
@5iver 5iver unpinned this issue May 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OH dependency Depends on OH bug or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants