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

Feature/resolve split package osgi #89

Merged

Conversation

JulianFeinauer
Copy link
Contributor

This PR brings full OSGi support to PLC4X.
A bundle for the S7 driver is also added.

@JulianFeinauer
Copy link
Contributor Author

Its not intended to be merged, yet its. First, we should check it in-depth.

Copy link
Contributor

@chrisdutz chrisdutz left a comment

Choose a reason for hiding this comment

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

Hi Julian,
I think it would have been better to rename things from:
org.apache.plc4x.java.base.connection
To:
org.apache.plc4x.java.connection.tcp
org.apache.plc4x.java.connection.serial
org.apache.plc4x.java.connection.mock
...
as it sort of gets more and more concrete further down the package ...
What do you think? cause the way you currently have it, it's sort of an empty "connection" package for every type.

Copy link
Contributor

@chrisdutz chrisdutz left a comment

Choose a reason for hiding this comment

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

Hi Julian,
I think it would have been better to rename things from:
org.apache.plc4x.java.base.connection
To:
org.apache.plc4x.java.connection.tcp
org.apache.plc4x.java.connection.serial
org.apache.plc4x.java.connection.mock
...
as it sort of gets more and more concrete further down the package ...
What do you think? cause the way you currently have it, it's sort of an empty "connection" package for every type.

@JulianFeinauer
Copy link
Contributor Author

And I agree with you @chrisdutz with the naming. I just wanted to make the smalles changes possible which was base -> xxx.

@ottobackwards
Copy link
Contributor

How about throwing in a readme in the karaf features area?
I would have expected to see some documentation around OSGI support as well at the top level somewhere

@chrisdutz
Copy link
Contributor

We already had "plc4j/integrations/apache-karaf" modules and there we were using the goal "features-generate-descriptor" which generates the "feature.xml" ... the way you're currently doing it would require to manually maintain a second resource with dependency information.

@chrisdutz
Copy link
Contributor

Ok ... I compared the feature.xml you added manually, that's identical to the one generated by the plugin, except that it has one additional entry: scr

@JulianFeinauer JulianFeinauer force-pushed the feature/resolve-split-package-osgi branch from ac8b7e8 to a14fe10 Compare October 18, 2019 11:15
@JulianFeinauer JulianFeinauer force-pushed the feature/resolve-split-package-osgi branch from 95c37c0 to ecc5def Compare October 20, 2019 13:52
@chrisdutz chrisdutz merged commit 543de5c into apache:develop Oct 21, 2019
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.

None yet

5 participants