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

[addonsinfoprovider] Addon for providing addon-info of other addons #15780

Closed
wants to merge 16 commits into from

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Oct 20, 2023

This addon provides an AddonInfoProvider service which produces a list of AddonInfo information for all the other addons within the openhab-addons main distribution .kar file.

In particular it provides the AddonInfo 'countries', 'connection' and 'discovery-methods' fields for all addons (even those which have not yet been installed). To give a specific example, this addon is expected to be used by the Addon Suggestion Finder service as described here.

This addon scans the addon.xml information of ALL OH addons during the Maven build phase, and it creates an XML file which is built into this addon in the form of a resource file. When this addon is loaded, it reads the resource file and establishes an AddonInfoProvider service which produces the list of addons AddonInfo information from their respective addon.xml file.

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg added enhancement An enhancement or new feature for an existing add-on work in progress A PR that is not yet ready to be merged awaiting other PR Depends on another PR cre Coordinated Review Effort labels Oct 20, 2023
@andrewfg andrewfg requested a review from a team as a code owner October 20, 2023 16:29
@andrewfg andrewfg self-assigned this Oct 20, 2023
@andrewfg
Copy link
Contributor Author

Ping @mherwege

@andrewfg
Copy link
Contributor Author

Note that CI build fails because it depends on changes in core openhab/openhab-core#3806

@mherwege
Copy link
Contributor

@andrewfg Thank you, great work. I am sorry I have not been reactive lately. Work got me occupied. I hope I will be able to test both PR’s in the next few days.

One feedback already, I think you should exclude the generated addons.xml from the PR, as it should be created at build.

@andrewfg
Copy link
Contributor Author

exclude the generated addons.xml from the PR, as it should be created at build

Yes that makes sense. I need to read up on gitIgnore I suppose :(

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Looking forward to dig deeper into this and trying it out. Looks promising! I have added some quick feedback about the documentation.

bundles/org.openhab.misc.addonsuggestionfinder/README.md Outdated Show resolved Hide resolved

## Addon Developer Notes

If you want to your addon to scan the user's system then you need to include additional fields in your `src/main/resources/OH-INF/addon.xml` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be documented here, i.e. through the openhab-docs repository:
https://www.openhab.org/docs/developer/addons/addon.html#xml-structure-for-add-on-definitions

A link could be provided here, but probably the contents should be removed to avoid double maintenance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. But let us keep it here for now, until we make more progress on the total package.

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

andrewfg commented Oct 22, 2023

@mherwege I have a question to you: Currently I have the functionality split as follows..

  1. The special addon produces a list of candidates
  2. The core scans the candidates and produces a filtered list of actual suggestions
  3. The core delivers that filtered list via a rest api call

I am thinking that I should shift the functionality 2. from the core to the special addon. There would be two benefits — the core PR becomes smaller and therefore easier for OH core maintainers to overview and approve. The specific scanning and filtering functionality can become more generic. => WDYT?

@mherwege
Copy link
Contributor

@andrewfg I think the separation between core and addons you have right now is the correct separation. If I understand it well, core will base all its suggestions on info it gets from AddonInfo. This info can come from different sources, depending on where the addons are sourced from. E.g. if the AddonInfo received for marketplace addons also contains the discovery info, nothing would stop from these being suggested as well. If an external addon source is configured, it would have a similar effect. Moving the logic to the addons repo would mean this would have to be replicated.
Also, imagine another type of suggestions: addons that have country in their AddonInfo. These could for instance be alarm services or something like that. It would be easy enough to create another type of suggestion service in the same framework in core.

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 6, 2023

Some guidance on that for @andrewfg would be appreciated.

@kaikreuzer yes please.

To be precise..

Currently the Maven build process parses the 'addon.xml' file from the source sub folders in openhab-addons\bundles. So if we move this to core, then I would need to write Java code to extract the 'addon.xml' resource from within the .jar files within the respective .kar file i.e. the 'G:\OH4_TEST\addons\openhab-addons-4.1.0-SNAPSHOT.kar\repository\org\openhab\addons\bundles' file .. wherever it may be located.

We have a 'Russian doll' with three levels of stacking, so the questions are..

  1. How to find the .kar file
  2. How to find the .jar files inside the .kar file
  3. How to find (and read) the addon.xml file inside the .jar files

@J-N-K
Copy link
Member

J-N-K commented Nov 6, 2023

For aggregating XMLs (in this case the openhab-addons BOM), I added

<build>
    <pluginManagement>
      <plugins>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-antrun-plugin</artifactId>
          <version>1.8</version>
          <inherited>false</inherited>
          <executions>
            <execution>
              <id>create-bom</id>
              <goals>
                <goal>run</goal>
              </goals>
              <configuration>
                <target>
                  <copy file="${basedirRoot}/../../bundles/pom.xml" overwrite="true"
                    tofile="${basedirRoot}/../../bom/openhab-addons/pom.xml"/>
                  <!-- rewrite footer -->
                  <replaceregexp file="${basedirRoot}/../../bom/openhab-addons/pom.xml"
                    match="/modules[\s\S]*dependencies&gt;" replace="/dependencies&gt;"/>
                  <!-- rewrite header -->
                  <replaceregexp file="${basedirRoot}/../../bom/openhab-addons/pom.xml"
                    match="\S*parent[\s\S]*modules&gt;\S*" replace="header"/>
                  <replace file="{basedirRoot}/../../bom/openhab-addons/pom.xml">
                    <replacetoken>header</replacetoken>
                    <replacevalue><![CDATA[<parent>
    <groupId>org.openhab.addons.bom</groupId>
    <artifactId>org.openhab.addons.reactor.bom</artifactId>
    <version>${project.version}</version>
  </parent>

  <artifactId>org.openhab.addons.bom.openhab-addons</artifactId>
  <packaging>pom</packaging>

  <name>openHAB Add-ons :: BOM :: openHAB Add-ons</name>

  <dependencies>]]></replacevalue>
                  </replace>
                  <!-- rewrite content -->
                  <replace file="{basedirRoot}/../../bom/openhab-addons/pom.xml">
                    <replacetoken><![CDATA[<module>]]></replacetoken>
                    <replacevalue><![CDATA[<dependency>
      <groupId>org.openhab.addons.bundles</groupId>
      <artifactId>]]></replacevalue>
                  </replace>
                  <replace file="{basedirRoot}/../../bom/openhab-addons/pom.xml">
                    <replacetoken><![CDATA[</module>]]></replacetoken>
                    <replacevalue><![CDATA[</artifactId>
      <version>@dollar{project.version}</version>
    </dependency>]]></replacevalue>
                  </replace>
                  <replace file="{basedirRoot}/../../bom/openhab-addons/pom.xml">
                    <replacetoken>@dollar</replacetoken>
                    <replacevalue>$</replacevalue>
                  </replace>
                </target>
              </configuration>
            </execution>
          </executions>
        </plugin>
      </plugins>
    </pluginManagement>
  </build>

I think you can use something similar for aggregating discovery information from the addon.xml.

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 6, 2023

For aggregating XMLs (in this case the openhab-addons BOM), I added

@J-N-K thanks for the feedback. I think what you suggest is similar to what @mherwege suggested, and I implemented, where we use the Maven build process to create an addons.xml file at build time. However I think that @kaikreuzer is suggesting to do it with Java code at runtime. In which case see my post above, where I added some more precise questions.

@J-N-K
Copy link
Member

J-N-K commented Nov 6, 2023

I think @kaikreuzer suggests to create the XML at build time and to read that from core instead of implementing an add-on that provides this information. Maybe the KarafAddonService could implement the provider for that.

@mherwege
Copy link
Contributor

mherwege commented Nov 6, 2023

I think @kaikreuzer suggests to create the XML at build time and to read that from core

The addons.xml is already created in the maven addons build. I didn’t know how to get access to an xml generated in the addons repository build process from core. That’s why a special addon in the addons repository has been created. The better place would indeed be in core or distrib, if someone can help us understand how to get access to that xml file created during the addons build from another repository than the addons repository.

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 6, 2023

Maybe the KarafAddonService could implement the provider for that.

@J-N-K any thoughts about how?

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 7, 2023

simply read the aggregated addons.xml file where the distro puts it

@kaikreuzer I have been manually digging around inside 'openhab-addons-4.1.0-SNAPSHOT.kar' and I don't see any addons.xml in it. So is it perhaps embedded as a resource in some .jar file inside the .kar file? And if so which jar should I be looking for?

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 7, 2023

Current solution is this..

/**
 * The {@link AddonsInfoProviderInstaller} component to install the special AddonsInfoProvider addon.
 *
 * @author Andrew Fiddian-Green - Initial contribution
 */
@NonNullByDefault
@Component(immediate = true, name = AddonsInfoProviderInstaller.SERVICE_NAME)
public class AddonsInfoProviderInstaller {

    public static final String SERVICE_NAME = "Installer for the AddonsInfoProvider special addon";

    private static final String KARAF_ADDONS_SERVICE_ID = "karaf";
    private static final String ADDONS_INFO_PROVIDER_UID = "misc" + Addon.ADDON_SEPARATOR + "addonsinfoprovider";

    private boolean addonInstalled;

    public AddonsInfoProviderInstaller() {
    }

    @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC)
    public void addAddonService(AddonService addonService) {
        if (!addonInstalled && KARAF_ADDONS_SERVICE_ID.equals(addonService.getId())) {
            addonService.install(ADDONS_INFO_PROVIDER_UID);
            addonInstalled = true;
        }
    }

    public void removeAddonService(AddonService addonService) {
        if (addonInstalled && KARAF_ADDONS_SERVICE_ID.equals(addonService.getId())) {
            addonService.uninstall(ADDONS_INFO_PROVIDER_UID);
            addonInstalled = false;
        }
    }
}

@kaikreuzer
Copy link
Member

We have a 'Russian doll' with three levels of stacking, so the questions are..

I think we can design this actually much simpler.

If the file(s) are created at build time, the result can be made available as a Maven artifact (see here for an example.

The distro build can then easily pick up these artifacts and package them within the openHAB distro, e.g. within userdata/etc or anywhere near there. This can be done by adding those files to a Karaf feature like it is done here. Maybe the openhab-runtime-ui feature can be used for this or alternatively, a new feature could be added in this file and then be packaged as a boot feature in the distro.

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 7, 2023

design this actually much simpler.

Umm. @kaikreuzer that may look simple to you, but unfortunately not to me; I am a simple java programmer; and really have little clue about Karaf, Maven, or features. I shall need more hand holding please.

The distro build can then easily pick up these artifacts and package them

I think still the issue is that the addons.xml file will be created in the openhab-addons workspace during the openhab-addons build; then the openhab-distro build needs to know the path to the openhab-addons workspace in order to import that file; and then the openhab-core needs to be able to find that file within the distro..

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 7, 2023

For aggregating XMLs (in this case the openhab-addons BOM), I added

@J-N-K (BTW) from the time stamps of the (supposedly overwritten) pom.xml file, and by including some xml comments in the original, that should disappear if the file were overwritten, I am pretty sure that your example code is never actually run; the pom.xml file remains identical before and after the build .

@J-N-K
Copy link
Member

J-N-K commented Nov 7, 2023

Correct, that part is only executed if you do mvn create-bom. It is not run during the usual build.

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 7, 2023

do mvn create-bom

Actually do mvn create:bom I think..

But anyway it goes to prove that I am not the right person to try to do this work in the way that @kaikreuzer and you @J-N-K are asking for; I simply do not have the time or ability to go over the Maven, Artifact, Feature learning curve; perticularly when there is no useful documentation on those things. I shall have to put this PR into sleep mode unless someone can step in.

@mherwege
Copy link
Contributor

mherwege commented Nov 7, 2023

@andrewfg What you can already do is move the code for this AddonInfoProvider to core, whereby you read from an addons.xml file in userdata/etc.
The generation of the addons.xml file which is defined in the pom.xml of this addon needs to stay in this repository. I am not sure yet where the best place is. On top of the generation, that same build will need to make the resulting file an mvn artifact, as @kaikreuzer described.
A new PR in the openhab-distro repository needs to pick up the artifact and put it in userdata/etc so the AddonInfoProvider can read it at runtime.

I don't know much about karaf and mvn, but I am willing to try this in the next few weeks starting from the examples @kaikreuzer has provided. I will just need time for it, and time is scarce at the moment. So don't expect me to turn this around quickly.

In the mean time, just make the assumption there will be an addons.xml file in the userdata/etc folder at runtime and start from there in the core PR.

@kaikreuzer
Copy link
Member

I don't know much about karaf and mvn, but I am willing to try this in the next few weeks starting from the examples @kaikreuzer has provided.

Thanks for volunteering here, @mherwege! I think the examples I linked should give a good starting point as it is fairly standard Maven functionality. Feel free to ping me whenever you feel you get stuck and things do not work as you would expect them to work.

the openhab-core needs to be able to find that file within the distro.

@andrewfg That's really just classical reading a file from the filesystem, see e.g. here how to directly access the openHAB userdata folder.

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 8, 2023

@kaikreuzer / @mherwege two things..

  1. See this commit where I have moved the code to openhab-core
  2. Shall I (therefore) close this PR, or do you want to keep it open as a place holder?

@J-N-K
Copy link
Member

J-N-K commented Nov 8, 2023

@mherwege J-N-K@a9a79b0 But this needs to be refined a bit. IMO the XML should be more "standard" and the should be an XSD for that (which should be in core).

I'm currently preparing a commit for openhab-distro which installs the file in the correct location. Unfortunately I have to fix a build issue in openhab-addons before :-(

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 8, 2023

IMO the XML should be more "standard" and the should be an XSD for that (which should be in core).

@J-N-K can you be more precise about "more standard"?

In the core PR have already modified the XSD for addon.xml to include the new discovery methods. So I am happy to add another XSD for the over wrapper containing a list of those.

@J-N-K
Copy link
Member

J-N-K commented Nov 8, 2023

Currently it looks like

<addon-info-list>
  <addons>
    <addon:addon id="groovyscripting" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xmlns:addon="https://openhab.org/schemas/addon/v1.0.0"
	xsi:schemaLocation="https://openhab.org/schemas/addon/v1.0.0 https://openhab.org/schemas/addon-1.0.0.xsd">

	<type>automation</type>
	<name>Groovy Scripting</name>
	<description>This adds a Groovy script engine.</description>
	<connection>none</connection>
    </addon:addon>
...
  </addons>
</addon-info-list>

It should probably be something like

<?xml version="1.0" encoding="UTF-8"?>
<addon-info-list xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xmlns:addon="https://openhab.org/schemas/addon/v1.0.0"
	xsi:schemaLocation="https://openhab.org/schemas/addon/v1.0.0 https://openhab.org/schemas/addon-1.0.0.xsd">
  <addons>   
    <addon:addon id="groovyscripting">
	<type>automation</type>
	<name>Groovy Scripting</name>
	<description>This adds a Groovy script engine.</description>
	<connection>none</connection>
    </addon:addon>
...
  </addons>
</addon-info-list>

The namespace definition should not be added for each element. But this can be done by adjusting the header.xml and a bit improvement of the ant instructions.

And as I already said: there should be an XSD for the addon-info-list. BTW: Why did you add the addons element?

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 8, 2023

BTW: Why did you add the addons element?

With the addons element there is an outer addon-info-list element that contains one instance of an explicit list of addon elements (this is considered the correct way to do lists). Whereas without the addons element, the outer element is both the wrapper and implicitly also the list container (implicit lists are considered to be bad practice). And (therefore) the former is easier to deserialize using standard libraries, whereas the latter would require extra code.

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 8, 2023

@J-N-K I am writing the XSD now. =>I will let you know so you can put the schema reference in the header.xml part of your PR.

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 8, 2023

@J-N-K I committed the revised schemas here

Please note that the schema for addon-list imports the schema for addon; so I am not sure if the addon: schema prefix is required, or it is implicit in the nested schema; i.e. one or other of the xml samples would apply. I think version 1 is fine => WDYT?

Version 1 (without sub schema prefixes)

<?xml version="1.0" encoding="UTF-8"?>
<addon-list xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xmlns:addon-info-list="https://openhab.org/schemas/addon-info-list/v1.0.0"
	xsi:schemaLocation="https://openhab.org/schemas/addon-info-list-1.0.0.xsd">
  <addons>   
    <addon id="groovyscripting">
	<type>automation</type>
	<name>Groovy Scripting</name>
	<description>This adds a Groovy script engine.</description>
	<connection>none</connection>
    </addon>
...
  </addons>
</addon-list>

Version 2 (with sub schema prefixes)

<?xml version="1.0" encoding="UTF-8"?>
<addon-list xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xmlns:addon="https://openhab.org/schemas/addon/v1.0.0"
	xmlns:addon-info-list="https://openhab.org/schemas/addon-info-list/v1.0.0"
	xsi:schemaLocation="https://openhab.org/schemas/addon-list-1.0.0.xsd">
  <addons>   
    <addon:addon id="groovyscripting">
	<type>automation</type>
	<name>Groovy Scripting</name>
	<description>This adds a Groovy script engine.</description>
	<connection>none</connection>
    </addon:addon>
...
  </addons>
</addon-list>

@J-N-K J-N-K mentioned this pull request Nov 8, 2023
@lolodomo lolodomo removed the cre Coordinated Review Effort label Nov 9, 2023
@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 9, 2023

@andrewfg andrewfg closed this Nov 9, 2023
@andrewfg andrewfg removed enhancement An enhancement or new feature for an existing add-on work in progress A PR that is not yet ready to be merged awaiting other PR Depends on another PR labels Nov 9, 2023
@andrewfg andrewfg deleted the addon-suggestion-finder branch August 25, 2024 13:15
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.

6 participants