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

Fix suggestion finder installation #4209

Merged
merged 2 commits into from
May 4, 2024

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented May 3, 2024

It looks like #4206 didn't fix the issue completely.

Here is a complement, that so far works well for me. The issue was in the variable sequence of starting the AddonSuggestionService and KarafAddonFinderService.

@jlaur Can you have a look if this works for you as well?

@J-N-K Sorry for these iterations. Hopefully, it works out well this time.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege requested a review from a team as a code owner May 3, 2024 09:02
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@@ -80,7 +80,7 @@ public AddonSuggestionService(final @Reference ConfigurationAdmin configurationA
this.configurationAdmin = configurationAdmin;
this.localeProvider = localeProvider;

SUGGESTION_FINDERS.forEach(f -> baseFinderConfig.put(f, true));
SUGGESTION_FINDERS.forEach(f -> baseFinderConfig.put(f, false));
modified(config);

// Changes to the configuration are expected to call the {@link modified} method. This works well when running
Copy link
Contributor

Choose a reason for hiding this comment

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

Side question: Is this job still needed after #4188?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. The reason is that it refers to a configuration from a different component (to avoid proliferation of component configurations all separated out in the UI) and in Karaf that does not get synchronized reliably.

@jlaur
Copy link
Contributor

jlaur commented May 3, 2024

Can you have a look if this works for you as well?

I did a quick test run:

  • Latest snapshot (4064) installed.
  • The code from this PR compiled.
  • org.openhab.core.config.discovery.addon-4.2.0-SNAPSHOT.jar replaced.
  • cache and tmp removed.

and it still doesn't seem to work for me:

openhab> bundle:list -s | grep addon
158 │ Active │  80 │ 4.2.0.202405030306    │ org.openhab.core.addon
159 │ Active │  80 │ 4.2.0.202405030313    │ org.openhab.core.addon.marketplace
160 │ Active │  80 │ 4.2.0.202405030314    │ org.openhab.core.addon.marketplace.karaf
171 │ Active │  80 │ 4.2.0.202405031242    │ org.openhab.core.config.discovery.addon
172 │ Active │  80 │ 4.2.0.202405030309    │ org.openhab.core.config.discovery.addon.process

@mherwege
Copy link
Contributor Author

mherwege commented May 3, 2024

I did a quick test run:

  • Latest snapshot (4064) installed.
  • The code from this PR compiled.
  • org.openhab.core.config.discovery.addon-4.2.0-SNAPSHOT.jar replaced.
  • cache and tmp removed.

I did exactly that, same snapshot, compiled version (in Eclipse), and it is working for me. I am testing on Windows.

openhab> bundle:list -s | grep addon
158 │ Active │  80 │ 4.2.0.202405030306    │ org.openhab.core.addon
159 │ Active │  80 │ 4.2.0.202405030313    │ org.openhab.core.addon.marketplace
160 │ Active │  80 │ 4.2.0.202405030314    │ org.openhab.core.addon.marketplace.karaf
171 │ Active │  80 │ 4.2.0.202405030931    │ org.openhab.core.config.discovery.addon
172 │ Active │  80 │ 4.2.0.202405030309    │ org.openhab.core.config.discovery.addon.process
252 │ Active │  80 │ 4.2.0.202405030309    │ org.openhab.core.config.discovery.addon.ip
253 │ Active │  80 │ 4.2.0.202405030314    │ org.openhab.core.config.discovery.addon.mdns
254 │ Active │  80 │ 4.2.0.202405030310    │ org.openhab.core.config.discovery.addon.upnp
255 │ Active │  80 │ 4.2.0.202405030313    │ org.openhab.core.config.discovery.addon.usb

Notice the last 4, that only become available after a little wait.
It would be useful to monitor the trace log in your installation. Could it be that some of the packages are not downloaded yet, and it makes the feature installer fail? Look for an error on that.

@holgerfriedrich
Copy link
Member

@mherwege I did a full core build on Windows and prepared a distro zip w/o add-ons.
Though it seems to improve the loading of finders, on my machine the resolver gets confused which prevents OH from installing the add-ons.

I have checked with a lot of different versions starting even before #4188, all builds lead to system which runs successfully (including wizard and installation of plugins).
Rebasing this PR on op of upstream/main leads to the same problem and thus fails.

This might be a coincidence. Did you see those problems as well?

@mherwege
Copy link
Contributor Author

mherwege commented May 4, 2024

I didn’t do a full build, but when testing I saw a few of the errors like in #4158. As these are generated in the feature installer, using the feature installer for installing the finders may have aggregated the problem.
Unfortunately, I won’t have time to further investigate this weekend.

@holgerfriedrich
Copy link
Member

I did another test rolling back 4188 and 4206. Better, but not without resolver errors. Could install a few add-ons in the wizard, then broke down. So you might be right.

@J-N-K J-N-K added the bug An unexpected problem or unintended behavior of the Core label May 4, 2024
@J-N-K J-N-K merged commit 49cfb20 into openhab:main May 4, 2024
5 checks passed
@J-N-K J-N-K added this to the 4.2 milestone May 4, 2024
@mherwege mherwege deleted the suggestion_finder_fix branch May 4, 2024 17:06
@holgerfriedrich
Copy link
Member

I can confirm that it is working on S4065 on my RPI. All finders are back! 🎉

Details:
Upgrade to S4065 via apt worked.
Stopping OH and restart after cleaning the cache worked as well.
A few add-ons are installed there for testing, e.g. knx and mapdb. Installed new add-on zwave.
No resolver issues seen on this installation.

@jlaur
Copy link
Contributor

jlaur commented May 5, 2024

I can confirm that snapshot 4065 also works for me (in Windows), and was now able to retest #4036 successfully with this new version. 🎉

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/since-4-2-0-snapshot-build-4065-and-later-unable-to-resolve-root-missing-requirement/155842/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants