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

[automation] Ignore relative lib directories for scripts #2408

Merged
merged 1 commit into from
Jun 25, 2021

Conversation

jpg0
Copy link
Contributor

@jpg0 jpg0 commented Jun 18, 2021

ScriptFileWatcher can now be configured to ignore specific, relative directories. This is used so that it can ignore "node_modules" which is what JS is hardcoded to use as a library path.

(Note that this also disables generic library tracking, and instead leaves specific enablement up to script engine providers. I will re-enable this in the JS add-on.)

This change is required to allow JS scripts to use relative paths for libraries, to allow general support for 3rd party libraries and tooling.

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

@jpg0 jpg0 requested a review from a team as a code owner June 18, 2021 03:33
@jpg0 jpg0 changed the title Ignore relative lib directories for scripts [automation] Ignore relative lib directories for scripts Jun 18, 2021
@kaikreuzer
Copy link
Member

The PR build failure seems to be related, but I am not sure whether this is because it still has some outdated infos for the Karaf features. Wdyt? Is the build working locally for you?

@jpg0 jpg0 force-pushed the relative-js-libs branch from f5f83ba to 8df4c21 Compare June 24, 2021 04:40
@jpg0
Copy link
Contributor Author

jpg0 commented Jun 24, 2021

Build works for me, and I changed nothing related to the build and/or features/osgi. I've re-pushed (with no real changes) to try another build...

@jpg0 jpg0 force-pushed the relative-js-libs branch from a3a3622 to 8df4c21 Compare June 25, 2021 05:17
@cweitkamp cweitkamp added automation enhancement An enhancement or new feature of the Core labels Jun 25, 2021
Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
@jpg0 jpg0 force-pushed the relative-js-libs branch from 8df4c21 to 711e01f Compare June 25, 2021 05:19
@jpg0
Copy link
Contributor Author

jpg0 commented Jun 25, 2021

...rebased from latest main to try to rerun the build...

@kaikreuzer kaikreuzer merged commit 241a4f6 into openhab:main Jun 25, 2021
@kaikreuzer kaikreuzer added this to the 3.1 milestone Jun 25, 2021
kaikreuzer added a commit that referenced this pull request Jun 25, 2021
kaikreuzer added a commit that referenced this pull request Jun 25, 2021
@kaikreuzer
Copy link
Member

Sorry, I had to revert this, since there is indeed still a OSGi service dependency to DependencyTracker, which is no longer an OSGi service.

@kaikreuzer kaikreuzer removed the enhancement An enhancement or new feature of the Core label Jun 25, 2021
@kaikreuzer kaikreuzer removed this from the 3.1 milestone Jun 25, 2021
@jpg0
Copy link
Contributor Author

jpg0 commented Jun 27, 2021

@kaikreuzer you're right, apologies for that. What I am trying to do here is the minimum amount of change to allow a more flexible layout for JS scripting. The issue is that currently openHAB prescribes a single file layout arrangement for all scripting languages:

  • Multiple source directories (all directories under automation/jsr233 inc. subdirectories)
  • Multiple libraries directories (all directories under automation/lib inc. subdirectories)
  • Mixing of all language sources and libs within these directories
    So far, this has been acceptable (although I would say not ideal), but it makes things really hard for JS, particularly the 2nd point.

My personal feeling is that we should separate languages entirely (e.g. automation/jython, automation/js etc), because I think it's pretty unlikely that polyglot rules will be a priority (and can still be supported anyway), then optimise each layout for the language that it's targeting (and allow each language plugin to configure how it expects file to be laid out).

Anyway, I'm reluctant to introduce such a significant change at this point, which is why I'm making more minor changes. I'll redo this PR to pull dependency tracking out of ScriptFileWatcher (the dependency you identified) and have language plugins process the script dependencies. A little more complex, but not much.

@kaikreuzer
Copy link
Member

My personal feeling is that we should separate languages entirely

I agree, that would be a cleaner approach.

I'm reluctant to introduce such a significant change at this point

Maybe we could go there in two steps: First is to additionally support automation/js for JS rules and encourage users to move to this new structure. Second step would then be to drop support of the automation/jsr223 folder. Wdyt?

I'll redo this PR to pull dependency tracking out

Great, thanks!

@jpg0
Copy link
Contributor Author

jpg0 commented Jul 11, 2021

Yes - this would be a much better approach. I have never liked the deep hierarchy for scripts and libraries, and also wondered why users really need to see the jsr233 name.

In fact, I believe that @csowada already has a forked implementation that has done something like this. @csowada how far off to what we are discussing here are your changes? I'm thinking that we would:

  • Leave the current JS impl unchanged so the existing scripts don't break
  • Have the JS engine plugin to configure it's own ScriptFileWatcher and ScriptLibraryWatcher to support a parallel implementation in automation/js (and automation/js/node_modules)

@csowada
Copy link

csowada commented Jul 12, 2021

Hello @jpg0 ,

here is my change commit csowada/openhab2-addons@0b19023

But I think it is not compatible with the latest changes and it is a quick-and-dirty implementation. The main problem was that class ScriptFileWatcher is internal. So it was not possible to inherit the class and I had to double the code.

But this implementation is working here since several months.

ghys pushed a commit to ghys/openhab-core that referenced this pull request Sep 9, 2021
Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
ghys pushed a commit to ghys/openhab-core that referenced this pull request Sep 9, 2021
@wborn wborn added this to the 3.1 milestone Oct 9, 2021
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
GitOrigin-RevId: 241a4f6
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants