[automation] Added script filename to engine, if engineIdentifier is a file

[jpg0]

Attempt to determine the the filename from engineIdentifier and set it as the filename of the script being evaluated. If the engineIdentifier is not a file, then ignore.

Signed-off-by: Jonathan Gilbert jpg@trillica.com

[5iver]

Could you please provide examples of how to access ScriptEngine.FILENAME in a script (Jython and JS)? This should go into the documentation, which would hopefully be included in your PR 😄. If ScriptEngine.FILENAME is not set, is the default value null? It would be great if FILENAME come through in tracebacks, instead of just <script>!

If not the script is not in a file, it would be possible to include the names of the rule and scripted condition or action, and the module's UID... like "Test rule: test scripted condition:55555eeeee".

I'm wondering if it might be useful to include this in a ScriptExtension to simplify access. I've also been thinking of other pieces of data that would be helpful in a ScriptExtension. E.g., I'd like to use the OH (or automation bundle) version in the helper libraries to only expose newer functionality to systems that support it.

[jpg0]

Could you please provide examples of how to access ScriptEngine.FILENAME in a script (Jython and JS)? This should go into the documentation, which would hopefully be included in your PR 😄. If ScriptEngine.FILENAME is not set, is the default value null? It would be great if FILENAME come through in tracebacks, instead of just <script>!

So I'm not exactly sure how you retrieve this from inside a script; I think that's dependent on the engine. The purpose of this is to supply it to the engine in a standard way so that the engine can use it when needed. The original (selfish) purpose of this was to get what you suggest: the filename in tracebacks. Setting it appears to acheive this.
Given this, and specifically that it's not designed to be a scriptwriter-accessible piece of information, I'm not sure that it should be part of OH docs?

If not the script is not in a file, it would be possible to include the names of the rule and scripted condition or action, and the module's UID... like "Test rule: test scripted condition:55555eeeee".

Would this be possible? I'm not sure where to get this information from at the point of script evaluation?

I'm wondering if it might be useful to include this in a ScriptExtension to simplify access. I've also been thinking of other pieces of data that would be helpful in a ScriptExtension. E.g., I'd like to use the OH (or automation bundle) version in the helper libraries to only expose newer functionality to systems that support it.

I think that it would, but my personal view is that the ScriptExtension stuff needs a bit more organisation. I feel that ideally a scriptwriter should be able to retrieve this type of information through an API which is present in the runtime, rather than requesting certain properties are injected. More like the APIs available in the JRE to inspect the environment (or the same in NodeJS or Python runtimes etc).

(A bit OT but it does clash a fair bit with the JS module system, what I'd really like to see would be something like:

const env = require('env');
let environment_description = `${env.current_file_name} running in ${env.runtime}`;

I haven't looked into how you could create an abstraction to inject unified, language-agnostic modules, but it would be very simple for JS given the module code I have.)

[jpg0]

Oops, I just realised that engineIdentifier was a URL not a File, so I've updated the parsing. I also added a check to only set it if it's not already set.

[5iver]

@cweitkamp, I have my IDE fired up to work on a Jython Script Engine addon, so I'll take a deeper look at this and test it out tonight. I don't see any harm in this PR, but something in the back of my head is telling me that there might be a better place to implement this... just need to look through some code. If the tracebacks show the script name, then this PR is very helpful!

[jpg0]

My 2c on how this should be done properly...

• The current implementation concerns itself with javax.scripting only. The 'API' to set the filename is as in the PR, so this is the right place to set it. Obviously this changes if it breaks out of javax.scripting-specific code, but I'm anticipating this is OH3 at a minimum.
• The way that the implementation gets the filename is not optimal. The problem here is that it's never actually supplied a filename, presumably because we want to support loading scripts from places which aren't filesystems. Currently only an engineIdentifier, scriptType and an InputStream are supplied. I would suggest that this is what needs to be fixed by passing more information in to the manager, either as additional params of a script context.
  (I didn't try to do this as I wanted to keep the PR minimally intrusive!)

[5iver]

My apologies... I have not had a chance to test this yet, but I plan to get to this over the weekend. However, I have put some time into the graaljs script engine addon (https://github.com/openhab/openhab2-addons/pull/6399) and have found several issue with the implementation. I doubt I will have time to completely review that one this weekend.

presumably because we want to support loading scripts from places which aren't filesystems

We do... JSON rules created through the file system and REST API (Paper UI) can have Scripted Actions and Scripted Conditions that use scripted automation. The script language to use is determined by the MimeType associated with the script type selected. IIRC, the file names and MimeTypes are assessed in ScriptFileWatcher.java, which may be able to be utilized here. But first, I want to actually see the script name being used in an exception.

[5iver]

The file names do not have to be unique, but the paths are...

Suggested change

	engine.getContext().setAttribute(ScriptEngine.FILENAME, maybeScriptFile.getName(), ScriptContext.ENGINE_SCOPE);
	engine.getContext().setAttribute(ScriptEngine.FILENAME, maybeScriptFile.getPath(), ScriptContext.ENGINE_SCOPE);

[jpg0]

Does you know how this looks in a stack? Here is a sample stack trace:

org.graalvm.polyglot.PolyglotException: Item 'LivingRoomLights_Light_Real' could not be found in the item registry
        at org.eclipse.smarthome.core.internal.items.ItemRegistryImpl.getItem(ItemRegistryImpl.java:82) ~[?:?]
        at <js>.getItem(items.js:347) ~[?:?]
        at <js>.:program(color_lights.js:12) ~[?:?]
        at org.graalvm.polyglot.Context.eval(Context.java:344) ~[?:?]
        <snip>

You can see the script part of it in the middle, with each frame starting with <js>. One frame was auto-filled by the engine, because it loaded the file (items.js), the other was populated by the code in this PR because it came from a stream as it was the entry-point script loaded by OH (color_lights.js).
I am asking whether you have seen what happens if you use the full path, because for the other frames, the engine only uses the name.

[5iver]

Using getPath(), before...

2019-11-24 22:42:12.190 [ERROR] [org.openhab.core.automation.module.script.internal.ScriptEngineManagerImpl] - Error during evaluation of script 'file:/opt/openhab2/conf/automation/jsr223/python/personal/test/test_script_2.py': NameError: name 'PersistenceExtensions' is not defined in <script> at line number 5

After...

2019-12-07 11:53:44.925 [ERROR] [org.openhab.core.automation.module.script.internal.ScriptEngineManagerImpl] - Error during evaluation of script 'file:/opt/openhab2/conf/automation/jsr223/python/personal/test/test_script_2.py': NameError: name 'PersistenceExtensions' is not defined in /opt/openhab2/conf/automation/jsr223/python/personal/test/test_script_2.py at line number 5

And here is a traceback before...

2019-11-20 19:54:31.471 [ERROR] [jsr223.jython] - Traceback (most recent call last):
  File "/opt/openhab2/conf/automation/lib/python/core/log.py", line 51, in wrapper
    return function(*args, **kwargs)
  File "/opt/openhab2/conf/automation/lib/python/core/rules.py", line 108, in execute
    self.callback(inputs.get('event'))
  File "<script>", line 36, in send_fire_tv_adb
NameError: global name 'Exec' is not defined

... and after...

2019-12-05 10:47:06.702 [ERROR] [jsr223.jython] - Traceback (most recent call last):
  File "/opt/openhab2/conf/automation/lib/python/core/log.py", line 51, in wrapper
    return function(*args, **kwargs)
  File "/opt/openhab2/conf/automation/lib/python/core/rules.py", line 108, in execute
    self.callback(inputs.get('event'))
  File "/opt/openhab2/conf/automation/jsr223/python/personal/misc.py", line 224, in lux_correction
    if not isinstance(event.itemState, QuantityType) or (event.itemState < QuantityType(u"1 W/m²") and items["Sun_Radiation_Diffuse"] > QuantityType(u"20 W/m²")) or (event.itemState > QuantityType(u"5 W/m²") and items["Sun_Radiation_Diffuse"] < QuantityType(u"1 W/m²")):# or event.itemState > items["Sun_Radiation_Total"] or (str(items["Mode"]) not in ["Night", "Late"] and not PersistenceExtensions.updatedSince(ir.getItem("Weather_SolarRadiation"), DateTime.now().minusHours(2))) or (event.itemState > QuantityType(u"3 W/m²") and str(items["Mode"]) in ["Night", "Late"]):
IllegalArgumentException: java.lang.IllegalArgumentException: Can not compare incompatible units.

[jpg0]

Interesting! What is producing the Python traceback? The Python engine? If you look at the example I gave, it only includes the filename in both the JS and Java frames. Digging into where these come from, the JS comes from my module loader (the javadoc specified 'the name of the source'), and the other frames come from the Java runtime (which also only includes the filename). Additionally the script engine param Javadoc specifies that it should be 'the name of the file being executed'.

Personally I don't think that usability matters too much here; whilst it's possible to have non-unique filenames, in practice you are never working on two files with the same name. I do however see that it's going to look weird to have a mixture of paths & names in either traceback.

One option here would be to pass the engineIdentifier to the ScriptEngineFactory and let it set it, this way we can choose different options for each language, depending on whether their standard formatting uses paths or names?

(As an aside, two things that I have missed and needed to workaround in the factory is (a) never seeing the engineIdentifier and (b) not being notified about disposal of engines.)

[5iver]

whilst it's possible to have non-unique filenames, in practice you are never working on two files with the same name.

While not extremely common, it is very possible to have scripts of the same name in different directories.

One option here would be to pass the engineIdentifier to the ScriptEngineFactory and let it set it

I don't see this as an option, since not everyone will be using custom ScriptEngineFactories. Groovy, Jython, jRuby, Kotlin, etc. can just be dropped into the classpath.

[jpg0]

Sure, I'm not saying that it's impossible, just that in practice it's rare enough that most language implementations don't print the full path. For the languages that I know that run on the JVM, only Python & Ruby print the full path. The others (Java, Kotlin, Groovy, Scala, Javascript, plus Xtend I guess) do not, and nor do languages like C & C++. (Personally I find the full path just means you get long, confusing frames, but I accept others may not.) Additionally the docs point to using the name rather than the path.

I will switch to path if you really think it's the right way to go, but it feels like an optimisation for Jython here, rather than 'most' languages.

As for letting the ScriptEngineFactory set it; maybe we would set it if they had not? So for any language that does not set it, use the most common (name), but if it's already been set (as for example Jython is in it's own bundle, so could set it as the path), then we leave it? This means that we get the expected/idiomatic behaviour for all languages except JRuby?

[5iver]

Additionally the docs point to using the name rather than the path.

Could you please provide a link? I cannot find any reference that states the path should not be used? Still, it's up to us to decide which is more functional for OH users.

I will switch to path if you really think it's the right way to go

👍 😜

but it feels like an optimisation for Jython here, rather than 'most' languages.

How so? All of the supported scripting languages can have non-unique names for scripts. I don't see anything Jython specific in a solution to provide a unique path rather than a file name to help troubleshoot an exception. Stacktraces are ugly to begin with and I don't think a full path makes it any uglier. Isn't this all irrelevant though, since stacktraces are neutered for automation?

As for letting the ScriptEngineFactory set it; maybe we would set it if they had not? So for any language that does not set it, use the most common (name), but if it's already been set (as for example Jython is in it's own bundle, so could set it as the path), then we leave it?

Jython does not set this, and there will be people who chose not to use the bundle, so it wold end up being <script>. How about defaulting to full path and having configuration options in the custom ScriptEngineFactories?

[jpg0]

Ok, links are:
javax.script: https://docs.oracle.com/javase/7/docs/api/javax/script/ScriptEngine.html#FILENAME
graal (examples and API calls): https://www.graalvm.org/sdk/javadoc/org/graalvm/polyglot/Source.html
^ these don't specify that you shouldn't use a path, they just say that you should use a name, and that is what the example uses too.

I say it's an optimisation for Jython because when the stack trace is dumped, it will look wrong for all languages except Jython (and JRuby). For JS it will look like this:

        at <js>.myScript(/here/is/where/the/full/path/to/the/script/will/be/file.js:11) ~[?:?]
        at <js>.myFunction(file1.js:347) ~[?:?]
        at <js>.:myOtherFunction(file2.js:12) ~[?:?]
        at <js>.:myFinalFunction(file3.js:12) ~[?:?]

While you are correct that currently we have no stack traces thrown from Java code, they can be from script code, so we'll still see this.

I'm not too fussed about which way the default goes and how we switch, but I did have an idea that may solve many of these challenges:

• Add parameter of engineIdentifier to ScriptEngineFactory.createScriptEngine(..)
• Add parameter of ScriptExtensionAccessor (an interface extracted from ScriptExtensionManager that provides readonly access) to the same method

This way, in this case, the ScriptEngineFactories can set the filename (the generic one could choose the default). It would also mean that they get some control over how presets are accessed, so would remove the need I had for the other PR about the scriptExtension.importPreset call. Happy to submit another PR if you think it would be the way to go.

[5iver]

I've tested this PR for a while now and I have not seen any issues. Having the script path in exceptions is helpful. I still feel the path should be used rather than the filename, since script file names are not unique. This is true for all languages.

If I were a maintainer here, I'd hold out on merging until this was corrected.

[kaikreuzer]

Hey guys, I didn't read through all your discussion, but I just wondered if there is some outcome like whether it is good to merge?

[jpg0]

@kaikreuzer the only thing that held this up was a decision related to supporting multiple scripting languages and whether to adopt a style specific to one or the other. I had originally implemented such that stack traces include the filename only (as they do in JS, and Java), yet Python includes the full file path.

Maybe this pushes at a larger approach that we decide on when supporting multiple languages where things diverge:

• Should we always allow the supplied language to specify the approach that it wants to take? (but then do we supply a default?)
• Should we always choose the approach we think is the best across languages? (even if it means inconsistency for some languages?)
• Should we choose a 'preferred' language and try to keep as close to it's standards? (which in this case would be Python or Java)

TBH I don't know how much these types of decision will come up in the future. There are certainly others though, e.g.:

• are host-provided interfaces camelCase or snake_case? (or something else)
• how is the scripting code laid out (specifically library paths)

[5iver]

You could read the discussion for more detail, but IMO there is a one more small change needed before this should be merged. The full path to the script should be used instead of just the name, so that users will know exactly which script caused the problem and where it is located. It is possible to have multiple scripts with the same name, so just getting the name is not very helpful, especially for a user who is not familiar with where the scripts are located.

[kaikreuzer]

You could read the discussion for more detail, but IMO there is a one more small change needed before this should be merged.

I am neither a JS nor a Python expert, so if you'd ask me for an opinion and decision, I'd say do it as much Java like as possible. For the change in question, the parameter is called FILENAME, so I'd say it should indeed be just the name and not the path.

But I'd honestly prefer that the both of you agree on a solution as it is something you are the experts on and you will make use of it in your code. As long as there is no compromise that satisfies both of you, I'd think this stays unmerged, which is probably the worst option.

[kaikreuzer]

I'd honestly prefer that the both of you agree on a solution

@jpg0 & @openhab-5iver Anybody willing to continue that discussion and come to a conclusion?

[jpg0]

I've been a bit busy, but just recently restarted looking at some of this. I planned to get everything in order getting the latest Graal version + Graal-provided module support against OH 3.x before looking at PRs. This is pretty much all done and working now (and much simpler than before). I'll close openhab/openhab-addons#6399 and open a number of PRs to try to get it in; this will be one of the first I'll handle.

[kaikreuzer]

Sounds great @jpg0, thanks for the update!

[jpg0]

My plan for this PR:

• Allow ScriptEngineFactorys to set the FILENAME if they like, this way they can format it to match the rest of their output
• If it's not set, then set it to the full path of the file by default (so matches Python)

For the former, there is currently no (easy) way to get at the engineIdentifier for a ScriptEngineFactory that is being configured to process it. There are a few other things that are likely to need this too, so I will first submit a change to support this, will link that PR to this PR, then once merged will update this PR to use it.

[boc-tothefuture]

@5iver pointed out that I opened a similar PR #1885 that I created based on development of my Jruby scripting implementation.

Looking at the comments here, it seems like there is a plan to let ScriptEngineFactory implementations set the filename in a configurable manner to handle some set of use cases and then to default to providing the full path.

It appears that some work is necessary and some changes need to be figured out and discussed to support the configurability of the filename.

Can we at this point just handle the default case? Which is very simple to implement and delivers value to our users (easier error resolution), and then handle the more complex configurable case?

This PR is better than mine in that it handles it prior to eval rather than where I handle it (I wasn't sure how to get the filename prior to eval).

Can we modify this PR to to do the full path and probably not catch the base exception class (unless we have to for some reason)?

Otherwise, if we want to keep this PR open to handle the configurable case, I am happy to move the logic over to my PR to handle the default case.

[jpg0]

So I just took a look at the code and how we can make the basic (full path) work, but allow for engines to override if they like.

I recently had #1837 merged, which adds the engineIdentifier to the engine context for ScriptEngines. The challenge here is ordering; this is set post-construction so it's not easy for a script engine to override it with a custom value. Saying this, the same problem exists for the other values, so I'm currently wrapping and lazily initialising my ScriptEngines. So if we go with this, maybe we should just set it like in this PR, but do it alongside setting the other bits of context, in the createScriptEngine method rather than loadScript. It means diverging from the default is a little convoluted for other script engine factories, but possible.

Happy for either me or you to do it: I think this PR has lost it's source branch anyway, so I suspect that I'd need to open a new regardless.

[boc-tothefuture]

Let me take a shot at merging these suggested changes in my PR. Will update when I have something to be looked at.