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

[transform.jinja] Properly use jinjava OSGi #17358

Merged
merged 1 commit into from
Sep 4, 2024
Merged

[transform.jinja] Properly use jinjava OSGi #17358

merged 1 commit into from
Sep 4, 2024

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Sep 1, 2024

Follow-up to #17356
Depends on openhab/openhab-osgiify#49

The former version did use the OSGiified bundle, but did still embed it. This properly adds it as a bundle dependency.

@J-N-K J-N-K requested a review from jochen314 as a code owner September 1, 2024 12:59
@lsiepel lsiepel added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 1, 2024
@J-N-K J-N-K added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 1, 2024
@jimtng
Copy link
Contributor

jimtng commented Sep 1, 2024

Thanks for the fix! I copied it similar to many other bindings. I guess they should all be changed from compile to provided too? Example: allplay, ambientweather, volumio, openhabcloud, pollytts, etc. They're all set to compile

@J-N-K J-N-K added the rebuild Triggers Jenkins PR build label Sep 2, 2024
@J-N-K
Copy link
Member Author

J-N-K commented Sep 2, 2024

Thanks for the fix! I copied it similar to many other bindings. I guess they should all be changed from compile to provided too? Example: allplay, ambientweather, volumio, openhabcloud, pollytts, etc. They're all set to compile

It depends. The standard configuration is "compile is embedded". However, if there is a file noEmbedDependencies.profile dependencies are never embedded.

@jimtng
Copy link
Contributor

jimtng commented Sep 2, 2024

What does this one apply to?

<exists>noEmbedDependencies.profile</exists>

@J-N-K
Copy link
Member Author

J-N-K commented Sep 2, 2024

In the "normal" build (i.e. profile NOT activated) the maven-dependency-plugin is configured to execute the embed-dependencies job

<execution>
<id>embed-dependencies</id>
<goals>
<goal>unpack-dependencies</goal>
</goals>
<configuration>
<includeScope>runtime</includeScope>
<includeTypes>jar</includeTypes>
<excludes>**/module-info.class</excludes>
<excludeGroupIds>javax.activation,org.apache.karaf.features,org.lastnpe.eea</excludeGroupIds>
<excludeArtifactIds>${dep.noembedding}</excludeArtifactIds>
<outputDirectory>${project.build.directory}/classes</outputDirectory>
<overWriteReleases>true</overWriteReleases>
<overWriteSnapshots>true</overWriteSnapshots>
<excludeTransitive>true</excludeTransitive>
<type>jar</type>
</configuration>
</execution>
which unpacks all dependencies (except those defined in the dep.noembedding property) with the scope compile to the target/classes directory. Everything in that directory is later included in the .jar. This happens on the process-sources phase of the build.

The profile sets the execution phase for the embed-dependencies job to none and no dependencies are unpacked (and as a result nothing is embedded).

@J-N-K
Copy link
Member Author

J-N-K commented Sep 2, 2024

There seems to be an issue with resolving the new artifact because it has the same version. I would suggest to wait if that fixes by itself in the next 48 hours, otherwise I'll prepare a new version 2.7.2_0 to resolve that.

@J-N-K J-N-K removed the rebuild Triggers Jenkins PR build label Sep 2, 2024
@J-N-K
Copy link
Member Author

J-N-K commented Sep 3, 2024

Why does it try to build mqtt.homeassistant tests? The build of the jinja transformation succeeded...

Depends on openhab/openhab-osgiify#49

Signed-off-by: Jan N. Klug <github@klug.nrw>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.google.re2j</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this use the osgiified version too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference between an OSGi bundle a a non-OSGi bundle is only the content of the MANIFEST.MF. In the case of an OSGi bundle the manifest contains a list of imported and exported packages, allowing the OSGi framework to decide what is needed to get this bundle running. What is done in the osgiify repository is essentially unpacking the .jar, examining the classes and creating a list of needed and provided packages, putting that together in a new MANIFEST.MF and finally package it again in a .jar. So from the classes contained, the osgiified bundle and the original bundle are the same. The additional statements are not used in the maven build, so we can stick to the original version.

The feature-verification checks if all required packages are available at runtime (i.e. it could wire all import statements to bundles). It takes the framework bundles (the openHAB core runtime bundles) and all bundles declared in the feature.xml to do this and there the additional statements are needed, otherwise it wouldn't know what is provided by the jinjava bundle, so we need to use the osgiified version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the details, this helped me to get a better understanding of how this fits together.

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty file noEmbedDependencies.profile can be removed from the PR i guess, otherwise LGTM

lsiepel

This comment was marked as duplicate.

@J-N-K
Copy link
Member Author

J-N-K commented Sep 4, 2024

No, the empty file is important. It activates the "no embedding" profile explained in #17358 (comment)

@lsiepel lsiepel merged commit c1a63ff into openhab:main Sep 4, 2024
5 checks passed
@lsiepel lsiepel added this to the 4.3 milestone Sep 4, 2024
@J-N-K J-N-K deleted the tj branch September 5, 2024 15:59
@J-N-K
Copy link
Member Author

J-N-K commented Sep 5, 2024

@jimtng If you need help in doing the same for the MQTT.homeassistant bundle, please ping me in that PR.

@jimtng
Copy link
Contributor

jimtng commented Sep 5, 2024

Integrating Jinjava into mqtt.homeassistant and removing its dependency on the JINJA transform isn't quite as straight forward, it seems. I had a preliminary look but haven't investigated how to actually bypass the need for jinja transform (addon).

My cursory look shows that jinja is only referenced for the availability topics, and I'm unfamiliar with the rest of the homeassistant mechanism. It isn't heavily docmented, and I'm not a user of this binding.

I think @ccutrer is in a much better position to tackle this one.

@ccutrer
Copy link
Contributor

ccutrer commented Sep 5, 2024

I've been thinking on it ever since this PR was created. Basically the homeassistant piece just takes the Jinja template from the Home Assistant discovery JSON blob, and sets the "transformation" setting of the underlying MQTT value to it, so that it automatically uses the Jinja transformation. For most components this is fine, since it's a single variable we send to the template, and that's what a transformation can handle. But for some components(template schema light bulbs in particular, but I think some other pieces of more complicated components like climate) this doesn't work, because the templates expect to be able to access multiple values (r, g, b, x, y, color_temp, h, s, b, etc.). But for complicated lights we use proxy MQTT Value objects anyway with callbacks, so it's not a problem to use Jinjava directly there. It'll take some thinking on how to use it for the basic case without complicating all of the components with proxy values with callbacks.

@ccutrer
Copy link
Contributor

ccutrer commented Sep 5, 2024

Okay, I've got a gameplan now. I've written it up in #17374, and also linked from there the other issues that it will solve.

digitaldan pushed a commit to digitaldan/openhab-addons that referenced this pull request Sep 24, 2024
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
Depends on openhab/openhab-osgiify#49

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
cipianpascu pushed a commit to cipianpascu/openhab-addons that referenced this pull request Jan 2, 2025
Depends on openhab/openhab-osgiify#49

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants