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

[infrastructure] Mavenized openHAB and integrated mavenized ESH repository #467

Merged
merged 27 commits into from
Jan 28, 2019
Merged

[infrastructure] Mavenized openHAB and integrated mavenized ESH repository #467

merged 27 commits into from
Jan 28, 2019

Conversation

maggu2810
Copy link
Contributor

This commit integrates the whole mavenized ESH repository ATM.
I would prefer to drop the ESH extensions later, because IIRC there are some openHAB bundles that depends on an extrension.
Perhaps we could drop the ESH extensions as soon as we moved some openHAB non framework bundles to the addons repository.

@maggu2810
Copy link
Contributor Author

I had to amend an additional change, so we can use the openHAB Oomph setup file after the merge has been done immediately (I hope I did not miss a change of it -- otherwise we can create additional PRs to fix it).

NOTICE Outdated Show resolved Hide resolved
bom/coap/pom.xml Outdated Show resolved Hide resolved
@maggu2810
Copy link
Contributor Author

For the naming fixes (I assume we will find a lot others) can (one of the alternatives)

  • you and the other developers create PRs to the source branch of this PR
  • this be merged and the fixes can be done by separate PRs against the master

otherwise it depends when I will find time to do it myself

@kaikreuzer
Copy link
Member

Sure, I'd suggest we do it as with any other PRs - comment on the PR and creating PRs against your branch with suggested changes until it is ready to be merged.

@maggu2810
Copy link
Contributor Author

Which one resolves a conversation in that project, the one that starts it or the one that "thinks" it has been fixed or one of the reviewer / committer?

@kaikreuzer
Copy link
Member

Anyone who "thinks" it is solved :-)

@wborn
Copy link
Member

wborn commented Jan 18, 2019

Thanks for your huge efforts @maggu2810! 👍

I had a bit of trouble to build your branch but after wiping my local Maven repository the build succeeded.

It kept throwing the following compilation error:

[INFO] Compiling 3 source files to /home/wouter/git/openhab/openhab-core/bundles/io/org.eclipse.smarthome.io.http.auth/target/classes
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /home/wouter/git/openhab/openhab-core/bundles/io/org.eclipse.smarthome.io.http.auth/src/main/java/org/eclipse/smarthome/io/http/auth/internal/AuthenticationHandler.java:[110,21] cannot find symbol
  symbol:   method getStatus()
  location: variable response of type javax.servlet.http.HttpServletResponse
[ERROR] /home/wouter/git/openhab/openhab-core/bundles/io/org.eclipse.smarthome.io.http.auth/src/main/java/org/eclipse/smarthome/io/http/auth/internal/AuthenticationHandler.java:[110,52] cannot find symbol
  symbol:   method getStatus()
  location: variable response of type javax.servlet.http.HttpServletResponse

Any reasons for downgrading Karaf to 4.2.1 (instead of 4.2.2)? We just upgraded to Karaf 4.2.2 with #458.

Because there are so many changes, the normal GitHub review UI seems unusable for adding comments.
Does someone know of another way to add inline comments with such huge PRs?

@maggu2810
Copy link
Contributor Author

Any reasons for downgrading Karaf to 4.2.1 (instead of 4.2.2)? We just upgraded to Karaf 4.2.2 with #458.

Hi @wborn,
I don't see any problem here.

openHAB Core has been moved to a framework and I don't see any reason why org.openhab.core.karaf shouldn't compile against Karaf 4.2.1.
I assume we could also depend on 4.1.1 or something similar.

Or do you use any feature that has not been present before?

You should differ between version that we compile against and the version that will be used in the product.

I don't think that any usage of Karaf 4.2.1 for the framework will break your Karaf 4.2.2 usage in the product.

But to answer your question: No, there has not been any reason. I assume it has been set by me on the migration path and I don't bumped it because I have not identified a problem.

@kaikreuzer
Copy link
Member

@maggu2810 Two quick questions:

  1. Where has the poms folder ended up - this so far contains the parent poms that are used for the add-ons build as well as the distro and are this critical to be present. If they are not part of openhab-core anymore, we should maybe move them to openhab-infrastructure or somewhere else for now.

  2. All bundles are still in the ESH namespace - do you plan to refactor them or are you expecting a PR from one of us against your branch for that?

@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/the-road-ahead-reintegrating-esh/64670/15

@maggu2810
Copy link
Contributor Author

@kaikreuzer

  1. I don't think we need should use a pom of openHAB Core as parent pom for the addons or the distribution. Perhaps a migration to openhab-infrastructure would make sense. The framework itself should not contain stuff that is not relevant for the framework.

  2. The BSN and automatic module name currently uses the artifact ID. The artifact ID has not been changed yet, because this would result into some collisions. There is now an "openHAB Core" and an "Eclipse SmartHome Core" bundle. There are UI specific Eclipse SmartHome bundles (Classic, Basic, Paper) and the fragment / patch bundle of openHAB. If we want to change the artifact ID and the BSN already, we need to move the UI bundles to openHAB Addons first (you stated a PR has been created for the ESH ones, but we need to also integrate the openHAB ones after that into the merged addons). Also the openHAB Core needs to be migrated into Eclipse SmartHome (and then renamed). We could also move the non framework relevant openHAB Core bundles in front of this one (I don't know if the "start" bundle is really a framework one and so on).

@kaikreuzer
Copy link
Member

  1. Well, the poms serve as a parent pom, which provide configuration for required external repos, SAT checks, etc. So imho it is closely related to the core framework as it is the required info for "how to build add-ons for the framework". But I am fine with moving this to the infrastructure project (@pfink, I assume that's ok with you as well).

  2. As discussed, I am already on it (removing the addon bundles from core), see Removed addons from core #469. So in your PR, you can directly remove those as well and the conflicts should be gone.

@maggu2810
Copy link
Contributor Author

maggu2810 commented Jan 19, 2019

  1. That's perhaps true for addons (e.g. bindings) that are provided by openHAB itself. If another one would like to develop addons for the framework (that are not under control of openHAB itself), he need to overwrite all openHAB related stuff that is part of the parent POM. So he needs to know the whole parent pom if there is stuff insert that needs be overwritten. If the POM it not intended to be used by non-openHAB owned bundles I don't think it should be part of this repo.

  2. Remaining conflict: org.openhab.core and org.eclipse.smarthome.core (that will be org.openhab.core after renaming). It will result into the same GAV and BSN

@maggu2810
Copy link
Contributor Author

After that we also need to merge the Karaf feature repositories.
I assume the prefix esh- should be renamed to openhab-. As I don't know anything about the openHAB features (which one exists), do you see here any conflicts?
Correct?

After the work has been done there will be two feature repos:

  • openhab-tp: the old esh-tp
  • openhab-core: the merge of esh-core and openhab-core

@wborn
Copy link
Member

wborn commented Jan 19, 2019

Or do you use any feature that has not been present before?

It's OK to have the framework on another version as long as there is another way (like Kai already suggested) to manage the Karaf/Jetty versions that are used in the distro.

Without the proper Karaf/Jetty ending up in the distro we'll have regressions like the Karaf SSH Console new line bug and Jetty ThreadPoolBudget warnings.

@maggu2810
Copy link
Contributor Author

That's a very important point, the framework must not dictate the exact version of Karaf and Jetty that needs to be used by a product.
I don't see any change that e.g. openHAB and the products I am maintaining (and perhaps other consumers of the framework) will use the same versions of Karaf and Jetty all the time.

@kaikreuzer
Copy link
Member

I assume the prefix esh- should be renamed to openhab-. As I don't know anything about the openHAB features (which one exists), do you see here any conflicts?

I am wondering whether we should maybe even use openhab-core- or openhabcore- as a prefix for all of them, so that it is easier to determine where they belong to / where to find them?

After the work has been done there will be two feature repos

Sounds right.

@maggu2810
Copy link
Contributor Author

I assume the prefix esh- should be renamed to openhab-. As I don't know anything about the openHAB features (which one exists), do you see here any conflicts?

I am wondering whether we should maybe even use openhab-core- or openhabcore- as a prefix for all of them, so that it is easier to determine where they belong to / where to find them?

I already think about it, but I did not yet know which is the best approach.
The consumer of features does not care which one provides it.

If you have a look at the whole Karaf features, you don't care which one provides it. The name should state what will be provided by the feature.

I start thinking about the whole thing as it has been about the group ID.
Should all core / framework bundles start with a group ID prefix of org.openhab.core instead only org.openhab? Using org.openhab and not org.openhab.core we don't know if a bundle that uses the group ID org.openhab.ui is part of the framework / core or if it is "some addon".

So, I am not against the suggested feature prefix, but if we agree, we have to change the group ID, too. Otherwise our argumentation is not really consistent.

@maggu2810
Copy link
Contributor Author

The current openHAB Core bundles depend on the models, too.
Will this bundle be moved to the distribution or the addons, too?

@kaikreuzer
Copy link
Member

Should all core / framework bundles start with a group ID prefix of org.openhab.core instead only org.openhab?

It imho makes sense to be able to differentiate between framework and add-on through the group ids. In ESH we had "esh.extension.*" for the addons, but I think using "org.openhab.core" for all framework parts is the better choice.

The current openHAB Core bundles depend on the models, too.

You mean the Xtext bundles? Yes, they are an integral part of the framework, just like the config/io/automation bundles as well.

@maggu2810
Copy link
Contributor Author

Another try... It seems my question has been not clear enough, so let me add more details:

There is a bundle with the name "openHAB Core" that uses the group ID org.openhab.core and the artifact ID org.openhab.core (https://github.com/openhab/openhab-core/blob/f9394266f2a572b684d2339cc6425bcc08c6a0f5/bundles/org.openhab.core/pom.xml).

There is a bundle with the name "Eclipse SmartHome Core" that uses the group ID org.eclipse.smarthome.core and the artifact ID org.eclipse.smarthome.core (https://github.com/eclipse/smarthome/blob/2239635027bf1e809b97dc3292f6da982e5daa0e/bundles/core/org.eclipse.smarthome.core/pom.xml).

If we merge the core bundles of openHAB and Eclipse Smarthome and replace "Eclipse SmartHome" with "openHAB" and "org.openhab.core" with "org.eclipse.smarthome.core", the bundles collide.

The esh.core bundle contains the real core and is referenced (transitive) by the most of the other bundles.

The oh.core bundle (correct me if I am wrong) is not really a core part of the whole smarthome framework. A product works perfect without it.

But okay, we change a lot of things now so perhaps both bundles should be merged into one. If all can coexist why should we care about.

The oh.core bundle imports model stuff (see `

org.eclipse.smarthome.model.core,
) I don't think this will result in a cycle or don't esh.model depend on esh.core itself.

You mean the Xtext bundles? Yes, they are an integral part of the framework, just like the config/io/automation bundles as well.

Sure, like config, IO and automation, it is part of the core but we can also create a product without the automation stuff.

I would veto if the most core bundle (what should be named "openHAB Core" similar to "Eclipse SmartHome Core" before) adds a hard dependency to the Xtext bundles.
Adding a requirement for Xtext to the base bundle of the framework is IMHO a really hard break to all consumers of the framework.

@maggu2810
Copy link
Contributor Author

About the group and artifact IDs to use, can you please give me a example?

The current approach has been to replace org.eclipse.smarthome with org.openhab.
So there will be org.openhab.core, org.openhab.ui, org.openhab.auth, org.openhab.io, ... groups that contains bundles.
If we replace with org.openhab.core it is fine for org.openhab.core.ui, org.openhab.core.auth, org.openhab.core.io, ... but I don't think we would like org.openhab.core.core.

@kaikreuzer
Copy link
Member

If we merge the core bundles of openHAB and Eclipse Smarthome and replace "Eclipse SmartHome" with "openHAB" and "org.openhab.core" with "org.eclipse.smarthome.core", the bundles collide.

Ok, sorry, I didn't see you were talking about those - yes, indeed, they are a collision (hopefully the only one).
Looking at the content of openhab.core, I'd think it'd make sense to actually get rid off the bundle completely. The DefaultSitemapProvider is of hardly any use (not sure anybody uses it...), the IconFormwarder should probably better be part of the compat1x bundle and the activator stuff could be discussed to be useful in the framework (e.g. for the LSP fix). Maybe the easiest solution for now would be to simply rename this bundle to openhab.boot or something and thus avoid the collision.

Adding a requirement for Xtext to the base bundle of the framework is IMHO a really hard break to all consumers of the framework.

That's clearly not wanted by anybody. The Xtext functionality is kept separate and must be kept this way.

but I don't think we would like org.openhab.core.core

... while it would be the most logical name. It is the core part of the core framework. I wouldn't mind that group id.

@kaikreuzer
Copy link
Member

... while it would be the most logical name. It is the core part of the core framework. I wouldn't mind that group id.

Sleeping over it and noticing that this will mean on the long run that those are also the package names, I question this statement myself.
Probably it is better to use org.openhab.core for all other parts (automation/config/io) and avoid the duplicate core for the "core" bundles then. Afaics, we won't have any collisions apart from the one we have already discussed.

@maggu2810
Copy link
Contributor Author

So you suggest to use the group ID org.openhab.core for all stuff in the openhab-core repository without any sublevels? One group ID for every artifact?
This should be possible because our artifact IDs already are unique around the whole directories.

Should we place all bundles directly in the directory without any further subdirectory between? Do we really need a directory ui, auth, automation, ...?

Please could you provide the group and artifact ID + directory layout for some bundles, so we could not misunderstand each other?

@kaikreuzer
Copy link
Member

So you suggest to use the group ID org.openhab.core for all stuff in the openhab-core repository without any sublevels? One group ID for every artifact?

No, I thought we will have group ids org.openhab.core.ui, org.openhab.core.auth, etc. and then instead of org.openhab.core.core simply org.openhab.core.

Should we place all bundles directly in the directory without any further subdirectory between? Do we really need a directory ui, auth, automation, ...?

I quite like the hierarchy instead of a very long flat list as it makes browsing a bit easier. So I wouldn't mind keeping it exactly as it is in your repo right now. If you think things get easier by having just the "bundles" folder and all bundles in it, it would be fine for me as well.

Just three examples, which should clarify be transferable to all the rest:

  1. bundles/core/org.openhab.core: groupId=org.openhab.core, artifactId=org.openhab.core
  2. bundles/core/org.openhab.core.voice: groupId=org.openhab.core, artifactId=org.openhab.core.voice
  3. bundles/ui/org.openhab.core.ui.dashboard: groupId=org.openhab.core.ui, artifactId=org.openhab.core.ui.dashboard

Does that make sense?

@pfink
Copy link
Contributor

pfink commented Jan 20, 2019

I don't think we need should use a pom of openHAB Core as parent pom for the addons or the distribution. Perhaps a migration to openhab-infrastructure would make sense. The framework itself should not contain stuff that is not relevant for the framework.

Well, the poms serve as a parent pom, which provide configuration for required external repos, SAT checks, etc. So imho it is closely related to the core framework as it is the required info for "how to build add-ons for the framework". But I am fine with moving this to the infrastructure project (@pfink, I assume that's ok with you as well).

It would be good to carve out stuff that is not version-specific. openhab/infrastructure is designed to be a component that really contains only version-unrelated infrastructure informations, because it is (on purpose!) not versioned and tagged together with openHAB. This could be the information e.g. where the base Git repository url is located or which Maven repositories openHAB is currently using. Use case: If openHAB decides one day to move to another SCM provider or to another artifact repository, this would affect all versions (also the older ones), so it's a not version-related change. In this case, of course it would be beneficial if the effort to make an older build work with the new environment is as low as possible. If we have the version-unrelated information at one place together, this effort would be minimal.

But, what would be a very bad smell is to put version-specific information there (e.g. which versions of maven plugins are currently used). This would make it very hard to get older builds running on new environments, because the newer plugins would break the older builds. So, you'd have to manually go over everything and find yourself what's version-related and what isn't.

That's perhaps true for addons (e.g. bindings) that are provided by openHAB itself. If another one would like to develop addons for the framework (that are not under control of openHAB itself), he need to overwrite all openHAB related stuff that is part of the parent POM. So he needs to know the whole parent pom if there is stuff insert that needs be overwritten.

Sorry, I don't understand the whole point. Where the parent POM is located has IMO nothing to do with the question whether a developer has to inherit his addon from this parent POM or not.

If the POM it not intended to be used by non-openHAB owned bundles I don't think it should be part of this repo.

I think it is intended to be used also by non-openHAB owned bundles. At least I'm not aware of compelling reasons why it shouldn't. And if other people don't want to use it, they can just do their own parent POM and inherit from there (or even don't use any parent POM at all). And yes, it would be easier for them if we carve out the pure version-unrelated infrastructure parts, as mentioned above 👍

@maggu2810
Copy link
Contributor Author

If openHAB decides one day to move to another SCM provider or to another artifact repository, this would affect all versions (also the older ones), so it's a not version-related change. In this case, of course it would be beneficial if the effort to make an older build work with the new environment is as low as possible. If we have the version-unrelated information at one place together, this effort would be minimal.

I don't understand how this will affect "older builds".
From a Maven perspective:

  • releases builds must not depend on snapshots
  • a release must never be changed

So, I assume the openHAB release "m" depends on the released infra version "x".
If the infra POM will be changed it needs another release of infra, so version "y".
openHAB (in its current development / master branch) can be changed to point to "y" now, but all versions of openHAB <= "m" will still rely on "x".

@Hilbrand
Copy link
Member

What about the still open points, like maven archetype and SAT? Or merge first and fix later?

@kaikreuzer
Copy link
Member

Or merge first and fix later?

That would be my plan.
Any volunteer for adding the maven archetype? Should probably be rather straight forward.
@maggu2810 Wrt SAT, you might be the best to tell what problems you came across and what might be the best way to include it - so your input is very welcome!

@maggu2810
Copy link
Contributor Author

@maggu2810 Wrt SAT, you might be the best to tell what problems you came across and what might be the best way to include it - so your input is very welcome!

I didn't test it yet because of the lack of time.
Perhaps enabling it again would be enough, but I assume as we moved ESH-INF there need to be some changes. I don't know which paths are hardcoded.

But I don't think it is a blocker and can be integrated also later.

@kaikreuzer kaikreuzer merged commit a37ccea into openhab:master Jan 28, 2019
@maggu2810
Copy link
Contributor Author

@kaikreuzer As long as there is no solution for #467 (comment), should we remove the .project files for that three projects, so they are not added to the projects after someone executed the IDE setup.

@maggu2810 maggu2810 deleted the mavenize-and-integrate-esh branch January 28, 2019 21:10
@kaikreuzer
Copy link
Member

Can you do a PR?

@maggu2810
Copy link
Contributor Author

maggu2810 commented Jan 29, 2019

Yes, I will do.

FYI: Without the index projects I still realized an interesting behavior of the IDE.
Interesting in that case that I don't have an explanation for it yet.
Yesterday on one of my machines I realized that there seems to be an endless loop building "org.openhab.core" (inspecting the "Progress" view of the Eclipse IDE).
I didn't realized it before and reflect what has been changed.
The only change I could identify is that I switched from "Oracle JDK" to "Adopt OpenJDK" to run the IDE.
On another machine running Oracle JDK 8 as IDE VM I did not realize that behavior.
I changed the other machine from "Adopt OpenJDK" to "Oracle JDK" 8 again and it seems to be solved.

I don't know if the specific Java distribution is really the root cause of it but we should remember, if someone complains.

@Hilbrand
Copy link
Member

@maggu2810 I did 'complain' about this already twice, see pure maven thread. So finally I don't feel alone anymore 🙂 I run:

openjdk version "1.8.0_191"
OpenJDK Runtime Environment (build 1.8.0_191-8u191-b12-0ubuntu0.18.04.1-b12)
OpenJDK 64-Bit Server VM (build 25.191-b12, mixed mode)

It makes development completely impossible, and why I, kind of, postponed looking into maven-only-build until some time later in the hope it was just because something temporary due to the wip status and nobody else seem to have that problem.

So it looks like specific Java distribution problem...

@maggu2810
Copy link
Contributor Author

and why I, kind of, postponed looking into maven-only-build until some time later in the hope it was just because something temporary due to the wip status and nobody else seem to have that problem.

Hm, isn't one of the pros of the maven only build that you don't rely on the Eclipse IDE (sure, for the formatter you still need to use the Eclipse IDE) but for "looking into" you could use every IDE that can handle Maven projects.

So it looks like specific Java distribution problem...

Please try again using the Oracle JDK 8 (202 should be the recent one).

<artifactId>org.openhab.core.bom.compile</artifactId>
<version>${project.version}</version>
<type>pom</type>
<scope>provided</scope>
Copy link
Member

Choose a reason for hiding this comment

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

I needed to change the scope to compile to be able to build it with maven on the command line. Same for provided below on compile-model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really?
I build the whole repository multiple times now and it works.

If the build does not work for you (using the current master), it would perhaps make sense to create an issue with the whole log.

provided should be present on compile time (similar to compile) but can be missing at runtime (different to compile).

<url>http://eclipse.github.io/smarthome/third-party/m2-repo/</url>
</repository>
<repository>
<id>bintray-maggu2810-maven</id>
Copy link
Member

Choose a reason for hiding this comment

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

The url to the maggu2810 repository should be removed. Is something like that still needed?

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 can be removed if you know about a repository that provides the dbus and unix bundle of matthw.
See:

<dependency>
<groupId>de.maggu2810.dbus</groupId>
<artifactId>dbus</artifactId>
<version>2.7.0</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>de.maggu2810.libmatthew</groupId>
<artifactId>unix</artifactId>
<version>0.5.0</version>
<scope>provided</scope>
</dependency>

@Hilbrand
Copy link
Member

Hilbrand commented Feb 3, 2019

I manually installed openjdk 8 version 201 and that seem to solve the continuous build problem.

@wborn wborn added this to the 2.5 milestone Feb 28, 2019
@cweitkamp cweitkamp added enhancement An enhancement or new feature of the Core infrastructure labels Dec 7, 2019
@cweitkamp cweitkamp changed the title mavenize openHAB and integrate mavenized ESH repository [infrastructure] Mavenized openHAB and integrated mavenized ESH repository Dec 7, 2019
@kaikreuzer kaikreuzer removed the enhancement An enhancement or new feature of the Core label Dec 13, 2019
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
* mavenize openHAB and integrate mavenized ESH repository

Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
GitOrigin-RevId: a37ccea
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.

None yet

8 participants