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

added missing versions for features and fixed assembly of jetty certificate bundle #547

Merged
merged 2 commits into from
Feb 8, 2019

Conversation

kaikreuzer
Copy link
Member

Signed-off-by: Kai Kreuzer kai@openhab.org

…ficate bundle

Signed-off-by: Kai Kreuzer <kai@openhab.org>
pom.xml Outdated
@@ -60,6 +60,9 @@
<sat.version>0.5.0</sat.version>
<slf4j.version>1.7.21</slf4j.version>
<xtext.version>2.14.0</xtext.version>
<jetty.version>9.4.12.v20180830</jetty.version>
Copy link
Member

Choose a reason for hiding this comment

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

This Jetty version is used with Karaf 4.2.2 but not with Karaf 4.2.1 (still defined on line 59). See also the Karaf 4.2.2 upgrade changes.

If the goal is to make a working openhab-distro using the versions defined in this POM I think it's best to update Karaf to 4.2.2.

Copy link
Member Author

Choose a reason for hiding this comment

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

FTR, this version is required here.

I just see that a Jetty version is defined in https://github.com/openhab/openhab-core/blob/master/features/karaf/openhab-tp/pom.xml#L17, so maybe it is best to move this one to https://github.com/openhab/openhab-core/blob/master/features/karaf/openhab-core/pom.xml then.

It would make the overall setup much simpler though, if we could simply say that openhab-core requires Karaf 4.2.2. @maggu2810 what's your take on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved the properties to the openhab-core feature for now - would suggest to merge it as is then (as it fixes a concrete bug that prevents the distro from being built), while I am open to discuss a general restructuring of features & versions for the future.

Signed-off-by: Kai Kreuzer <kai@openhab.org>
@maggu2810
Copy link
Contributor

maggu2810 commented Feb 8, 2019

What's the reason to bundle all the bouncycastle classes to this bundle?
If bouncycastle is already present in the OSGi runtime this will duplicate the memory footprint.

--

I see it has been bundled before.
So, scope provided for the maven dependency should be the correct one.
Why has this scope been removed?

@maggu2810
Copy link
Contributor

maggu2810 commented Feb 8, 2019

I still rely on Karaf 4.2.1 and so on another Jetty version.
What's the reason to bump the minimum version requirement?

@kaikreuzer
Copy link
Member Author

What's the reason to bundle all the bouncycastle classes to this bundle?

It has always been like this and there was some good reason for it (which I don't remember now), so just let us leave it like that (it only broke by the bnd migration now, which I am fixing).

I also don't see the reason to bump the minimum version requirement.

Ok, I didn't touch that for the tp feature - so we should be good to merge.

@kaikreuzer
Copy link
Member Author

kaikreuzer commented Feb 8, 2019

Why has this scope been removed?

Because the shade plugin didn't work with it - only without it.

@kaikreuzer
Copy link
Member Author

FTR, I am not trying to improve things here, I am just fixing stuff that broke through #467 and which prevent getting back to a successful distro build (which we didn't have for 2 weeks now). We definitely need to get that working again and this PR is hopefully the last missing piece.

@maggu2810
Copy link
Contributor

I am just fixing stuff that broke through #467 and which prevent getting back to a successful distro build (which we didn't have for 2 weeks now).

No need to blame my work. Perhaps I would not break the distro build for two weeks if there has been a little more feedback from other guys.

@maggu2810
Copy link
Contributor

No one depends on org.openhab.core.io.jetty.certificate so it does not hurt, but without the scope the behaviour is changed.

scope compile (default) stays that the dependency needs to be present at compile time and runtime (it will be transitive).
scope provided stays that the dependency needs to be present at compile time but not at runtime (it will be not transitive)

As the bouncycastle packages are shaded, there should be no transitive dependency and no need that it needs to be present at runtime (because the respective packages are bundled).

This should to be improved -- adding packages of other bundles should be done using the bnd.bnd file instead of further Maven plugins.
But hey, I assume it has been my job by introducing the bnd support.

@maggu2810 maggu2810 merged commit 4d9de63 into openhab:master Feb 8, 2019
@kaikreuzer kaikreuzer deleted the vershade branch February 8, 2019 18:18
@kaikreuzer
Copy link
Member Author

No need to blame my work

Sorry, I didn't mean to blame you - we were all aware that things will break and have to be fixed, but my hope was that we are through by end of Jan with that, that's why I am meanwhile a bit nervous that there's still no distro build. I only wanted to point out that some things accidentially got lost and that this PR is bringing them back (and not meaning to do that in a future-proof and nice way).

So instead of blaming you, I only wanted to excuse that I am not fixing it in a proper way.

Thanks for merging!

@wborn wborn added this to the 2.5 milestone Feb 28, 2019
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
…ficate bundle (openhab#547)

Signed-off-by: Kai Kreuzer <kai@openhab.org>
GitOrigin-RevId: 4d9de63
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.

3 participants