lburgazzoli / gradle-karaf-features-plugin

Apache License 2.0
4 stars 13 forks source link

Allow specifying which projects to generate a feature for #4

Closed sebersole closed 9 years ago

sebersole commented 9 years ago

In a multi-module project, your plugin picks up each module in order to generate a feature. You expect the plugin to be applied to the root project and then your task picks up each subproject.

I'd like to make a 2-part feature request and am even willing to send you a Pull Request if you are OK with the idea in principle. First, I would like to be able to apply the plugin at any level. Second, I would like the ability to define which specific projects get picked up for feature generation.

Some background. I am the lead developer for Hibernate. We have a module that aims to integrate Hibernate into OSGi containers. Until now we have used a very buggy approach for testing that based on Arquillian and embedded Felix. We are wanting to move to PaxExam and Karaf for testing. Additionally we would like to start publishing a Karaf feature file. Which lead me to you and your plugin :)

This OSGi integration is part of the hibernate-osgi module, which is part of the larger Hibernate ORM build. So I would like to apply your plugin there. Also, Hibernate has many modules; some should have a feature defined, some should not.

lburgazzoli commented 9 years ago

I agree.

This plugin has been initially developed as an helper plugin to do the initial and boring feature file creation so it is pretty limited but with your contribution, it could finally be more useful.

Please go ahead.

sebersole commented 9 years ago

On a related note, would you be OK if I looked at using the dependency tree as opposed to the flat list of depenency/artifacts in generating the bundles? In Hibernate testing we have had lots of problems with the ordering of bundles in features file if we try to generate them. Walking the dependency tree from the bottom (most distant transitively) to the top (out direct dependencies) should fix that.

Additionally, I'd like to propose either converting KarafFeaturesGenTaskExtension#wraps to a Map or adding a new extension field to allow such. The purpose would be to allow defining BND instructions for each wrap, which can then be appended to the wrap: url.

sebersole commented 9 years ago

https://github.com/lburgazzoli/lb-gradle-plugins/pull/5 - addresses the initial point about allowing to specify which projects to generate features from.

https://github.com/lburgazzoli/lb-gradle-plugins/pull/6 - addresses the order in which the bundle elements generated. As mentioned, I did not realize this would pick up the commit from PR 5 as well. If you agree to both of those, you can just apply PR 6

lburgazzoli commented 9 years ago

I agree, bundle ordering is sometime a pain so well, merged :-) A snapshot should be available soon on Sonatype OSS

sebersole commented 9 years ago

What do you think about the KarafFeaturesGenTaskExtension#wraps bit?

sebersole commented 9 years ago

Any clue as to why the Travis build failed for my commits?

On Wed, Apr 29, 2015 at 6:07 AM, Luca Burgazzoli notifications@github.com wrote:

I agree, bundle ordering is sometime a pain so well, merged :-) A snapshot should be available soon on Sonatype OSS

— Reply to this email directly or view it on GitHub https://github.com/lburgazzoli/lb-gradle-plugins/issues/4#issuecomment-97389480 .

lburgazzoli commented 9 years ago

I think it is because the repository is not the same as the one for which I've created the secrets so the deploy to Sonatype OSS failed :

Execution failed for task ':lb-karaf-features-gen:uploadArchives'.
> Could not publish configuration 'archives'
   > Error deploying artifact 'com.github.lburgazzoli:lb-karaf-features-gen:jar': Error deploying artifact: Failed to transfer file: https://oss.sonatype.org/content/repositories/snapshots/com/github/lburgazzoli/lb-karaf-features-gen/1.0.0.SNAPSHOT/lb-karaf-features-gen-1.0.0.20150429.034658-2.jar. Return 
code is: 401

I've saw that every time I had the secrets not properly set up for the repository.

About the wraps proposal, I think as first iteration a map would be enough but I'd like to review the plugin to make a little bit more fluent and in line with gradle style, something like:

bundle('com.google.guava:guava:.*') {
    wrap { 
        instruction 'Bundle-SymbolicName', 'xyz'
        instruction 'Bundle-Vendor', 'acme'
    }

    startLevel = 50 
    exclude = false | true
}
sebersole commented 9 years ago

I can definitely help with that bit too. I have developed and contributed to quite a few Gradle plugins now. What you propose is really nothing more than nested DSL extensions.

sebersole commented 9 years ago

I should say that I like your propsed DSL much better than what KarafFeaturesGenTaskExtension does currently. I just was not sure how open you'd be to massive changes in this plugin/task, especially in terms of DSL changes.

I'll work up something over the next few days (this one is more involved than the other 2).

lburgazzoli commented 9 years ago

That would be awesome. Please do it if you have a little time.

sebersole commented 9 years ago

I am doing a Hibernate release today. So I will have a little time today and more tomorrow. I should be able to do this by tomorrow.

On Wed, Apr 29, 2015 at 11:03 AM, Luca Burgazzoli notifications@github.com wrote:

That would be awesome. Please do it if you have a little time.

— Reply to this email directly or view it on GitHub https://github.com/lburgazzoli/lb-gradle-plugins/issues/4#issuecomment-97481283 .

lburgazzoli commented 9 years ago

I'm open to any change that would make the plugin better :-) and I definitively agree that the initial implementation is ugly. If you see anything that would make the DSL even better do not hesitate to propose it.

lburgazzoli commented 9 years ago

What do you think about a DSL like :

karaf {

    featuresDescriptor {
        name = 'my-karaf-feature-1'

        project(':project-1')
        project(':project-2')

        bundle('com.google.guava:guava:.*') {
            wrap { 
                instruction 'Bundle-SymbolicName', 'xyz'
                instruction 'Bundle-Vendor', 'acme'
            }

            startLevel = 50 
            exclude = false
        }
    }

    featuresDescriptor {
        name = 'my-karaf-feature-2'

        project(':project-3')

        bundle('com.sprinframework.:spring-.*:.*') {
            startLevel = 50 
            exclude = false
        }
    }

    karafArchive {
        name = 'my-karaf-kar'

        feature('my-karaf-feature-1')
        feature('my-karaf-feature-2')
    }
}
sebersole commented 9 years ago

Personally, I would stay away from just karaf. I know it's tempting, but it seems too broad and too much potential for clash with other plugin extensions. How about karafFeatureRepository?

I really like the more fluent approach to defining the actual features. Having project<->feature be 1-1 works for us, but this would be much more usable I think for users. Great thought!

What is the purpose of the feature elements inside karafArchive? Also, what do you intend with karafArchive? Are you seeing something more that just the "feature repository" (XML file) being generated? One thing I will need to do is to publish this repo file into Nexus alone with the hibernate-osgi jar. As far as the repo xml file, I need to specify a classifier and type. So would be great to account for those too here.

On Thu, Apr 30, 2015 at 5:30 AM, Luca Burgazzoli notifications@github.com wrote:

What do you think about a DSL like :

karaf {

featuresDescriptor {
    name = 'my-karaf-feature-1'

    project(':project-1')
    project(':project-2')

    bundle('com.google.guava:guava:.*') {
        wrap {
            instruction 'Bundle-SymbolicName', 'xyz'
            instruction 'Bundle-Vendor', 'acme'
        }

        startLevel = 50
        exclude = false
    }
}

featuresDescriptor {
    name = 'my-karaf-feature-2'

    project(':project-3')

    bundle('com.sprinframework.:spring-.*:.*') {
        startLevel = 50
        exclude = false
    }
}

karafArchive {
    name = 'my-karaf-kar'

    feature('my-karaf-feature-1')
    feature('my-karaf-feature-2')
}

}

— Reply to this email directly or view it on GitHub https://github.com/lburgazzoli/lb-gradle-plugins/issues/4#issuecomment-97732093 .

lburgazzoli commented 9 years ago

karafArchive should create a KAR which bundles feature files and repositories in a single compressed file you can deploy on Karaf which is very useful for deployment in environment where access to internet is not possible.

The purpose of the feature inside karafArchive is to define which features (and thus what jars) have to be included in the generated KAR

What about splitting the two concept in two top level nodes ?

karafFeatures {

    feature('my-karaf-feature-1') {
        project(':project-1')
        project(':project-2')

        bundle('com.google.guava:guava:.*') {
            wrap { 
                instruction 'Bundle-SymbolicName', 'xyz'
                instruction 'Bundle-Vendor', 'acme'
            }

            startLevel = 50 
            exclude = false
        }
    }

     feature('my-karaf-feature-2') {
        project(':project-3')

        bundle('com.sprinframework.:spring-.*:.*') {
            startLevel = 50 
            exclude = false
        }
    }
}

karafArchives {

    archive('my-karaf-kar') {
        feature('my-karaf-feature-1') {
            repo {
                exclude 'com.sprinframework.:spring-.*:.*'
            }
        }

        feature('my-karaf-feature-2')
    }
}

If project or feature are omitted by the respective task, the plugin should refer to the current context (i.e. a sub-project if the plugin is defined a sub-project level or the whole project if it is defined at root level)

sebersole commented 9 years ago

I know nothing about kar files. I have asked Brett Meyers who does most of the OSGi related work in Hibernate to date to chime in if he sees anything. So a kar is an archive with one (or more?) feature repository definitions and includes all the jars needed for the features defined in that (those) repository? If it does include the jars, I would assume that means different bundle urls, no? Meaning, not mvn: urls.

So given that information, here is what I would propose, generally:

karafFeatureGen {
    features {
        feature1 {
            name = 'my-karaf-feature-1'

            project(':project-1')
            project(':project-2')

            bundle('com.google.guava:guava:.*') {
                wrap { 
                    instruction 'Bundle-SymbolicName', 'xyz'
                    instruction 'Bundle-Vendor', 'acme'
                }

                startLevel = 50 
                exclude = false // inclusion should be default imo
            }

            ...
        }
    }

    archives {
        myKar {
               name = 'my-karaf-kar'
               ...
        }
    }
}

As far as archives, if the plan is to support just one (again no idea about these kars) then I'd suggest just an optional archive or kar instead, rather than archives (plural).

Some notes...

features is using a Gradle feature called NamedDomainObjectContainer which is how configurations, tasks, etc are implemented. Using that is nicer if you ever want to refer to these in your build (you could literally refer to the feature1 feature named above as $project.karafFeatureGen.features.feature1).

sebersole commented 9 years ago

Here is another (additional) idea...

Initially you had extraBundles. We sometimes need this as well to handle poorly defined bundle jars. For example, we use JBoss Logging which incorrectly requires log4j to be installed even though we do use log4j. So we'd obviously prefer to not add log4j as a transitive dep. But we do need to make sure it gets added to the feature. However, I think the notion of just naming the additional bundles can be improved, especially if you want to start bundling them into these kar archives. What I propose instead is to allow specifying Gradle Configuration(s) for this purpose:

configurations {
    karafExtras {
        description = 'Configuration for extra dependencies that need to be added to Karaf features'
    }
}

dependencies {
    ...

    karafExtras 'log4j:log4j...'
}

karafFeatureGen {
    features {
        feature1 {
            name = 'my-karaf-feature-1'

            project(':project-1')
            project(':project-2')

            extraBundle karafExtras

            ...
        }

        ...
    }
}

Another option, if we want to support just one configuration that applies to all features would be for the plugin to create that Configuration (which eliminates the need for the configurations bit).

We could also auto create multiple Configurations, one per defined feature (feature1KarafExtras e.g.) but that now becomes order dependent (you'd have to declare your dependencies after the karafFeatureGen block) which I generally try to avoid. The manual approach above certainly works for per-feature extras. We could do a combo too. Not sure how prevalent it is to need extras.

lburgazzoli commented 9 years ago

Yes kars can have multiple features file and there's no need to change the url as karaf will also search artifacts in the repository included in the kar (which looks like a standard maven repo). Then I'd like to have the option to create multiple kars (so leave it as plural) even tough most of the time I will create one per project.

I like the idea to use NamedDomainObjectContainer so I suggest to let name optional and take as example feature1 or myKar is name is not provided. Then yes inclusions should be default.

About extra bundles we should go for a combo like defining karafExtras by default and let developers define new configuration if they need it. The only think we should take into account is that the extra bundles may need to be tweaked like any other bundle (start level, bnd istructions, etc) so I suppose that extraBundle karafExtras only adds the listed dependencies to the dependencies this feature has to take into account so that I can still apply bundle customization

dependencies {
    karafExtras 'log4j:log4j:...'
}

karafFeatureGen {
    features {
        feature1 {
            project(':project-1')
            project(':project-2')

            extraBundle karafExtras

            bundle('log4j:log4j:.*') {
                wrap { 
                    instruction 'Bundle-SymbolicName', 'xyz'
                    instruction 'Bundle-Vendor', 'acme'
                }

                startLevel = 10
            }
        }
    }
}
sebersole commented 9 years ago

Yes, the bundle() bit is really all about customizing the <bundle/> defs. Bundles are only ever added through:

In regards to the "global karafExtras Configuration", you would not even have to add it to the feature via extraBundle karafExtras. We'd automatically pick that one up.

Regarding NamedDomainObjectContainer and naming the feature/kar... yes that is the normal convention when using NamedDomainObjectContainers.

I'll give support for kar files a go, but its not my itch and I won't use it so you'll have to verify it works.

sebersole commented 9 years ago

Also, I plan on just generating one features file. So even if you have multiple feature definitions, that will still be one feature file. That is what I need. That one features file is what I will publish to maven repo for Hibernate.

So how would that work with your multiple-kar desire? I assume you are thinking the feature file is generated based on how the features are grouped in your archives section?

I guess at a high-level, one option is to say that no archives defined indicates to do what I need (all features->one file). But this makes it difficult in terms of configuration. For example, I need to be able to specify the exact name of the features file. I would assume in the kar case that is not such a big concern. For example, this is what I started off with:

karafFeatures {
    featuresXmlFile = project.file( ... )
}

What I worry about is this:

karafFeatures {
    // totally meaningless config value now because archives defined...
    featuresXmlFile = project.file( ... )

    ...

    archives {
        ...
    }
}

ugh... cannot get this to render right

sebersole commented 9 years ago

One other thing... your file header currently assigns all copy rights to you individually. I have done a large chunk of the work on this plugin :) I would like to change the copyright statement to read along the lines of copyright being attributed to "contributors as indicated by the @author tags". Then add you and myself for authors. Are you ok with that?

lburgazzoli commented 9 years ago

Good point, let's go with a single features file and an optional single kar archive. About the header, I definitively agree so change the header as appropriate :-)

sebersole commented 9 years ago

Thinking about this some more.. I think what makes the most sense in terms of repository versus kar is to have 2 different tasks: generateKarafFeatureRepository and generateKarafArchive. The extension is the same in both cases. But splitting the tasks allows 2 things:

  1. segmenting the definition of the output file to the tasks
  2. holding on to state between feature generation which building a kar needs, but building a feature repository does not.

The first really comes down to this:

karafFeatures {
    featuresXmlFile = ...
    archiveFile = ...
}

What does that config mean?

Instead we would have:

karafFeatures {
    ...
}

tasks.generateKarafFeatureRepository {
    featuresXmlFile = ...
}

tasks.generateKarafArchive {
    archiveFile = ...
}

I guess what I am worried about is naming the xml file when an archive is going to be generated. What is the semantic of that. To me, if we are generating an archive, the name of the XML file should be irrelevant and generated in $buildDir/tmp just to be included in the archive. But maybe that is me being unfamiliar with kars.

sebersole commented 9 years ago

I had hoped to have this wrapped up, but unfortunately I am having problems getting my generated feature file to deploy properly (not new for me, I really kind of hate OSGi) and the guy who really knows OSGi and helps us out is not around until next week. I'll get back to this on Monday.

sebersole commented 9 years ago

In the meantime I pushed to my fork, if you wanted to see the changes so far.

https://github.com/sebersole/lb-gradle-plugins/tree/dsl-extensions

I also pushed my preliminary integration of this into Hibernate to my fork as well if you wanted to see this from a usage perspective, again so far.

https://github.com/sebersole/hibernate-core/blob/HHH-9699/hibernate-osgi/hibernate-osgi.gradle#L305-L349

lburgazzoli commented 9 years ago

The implementation looks very good so far and I think the feature file to be included in a kar can have any name but I will investigate it a little bit more.

sebersole commented 9 years ago

I noticed that you at one point were having a go at setting up PaxExam in Gradle ( I saw an old Gradle forum thread). I am trying to set up PaxExam+Karaf for testing of Hibernate OSGi support. We previously used a seriously buggy and fragile arquillian+felix setup that broke all the time. In fact this work is what feeds these contributions :) In the process I figured it would be great to have hibernate-osgi support Karaf via a published features file out of the box.

Anyway, I am having tons of problems getting this set up. Would you happen to have any ideas? I need to get this working as a means to validate the work I do here. It goes hand-in-hand. The basic gist is a problem getting RMI to work properly (apparently). The details are here : http://stackoverflow.com/questions/30033484/rmi-notboundexception-using-pax-exam-karaf-container

If you would rather chat about this somewhere other than this issue, you can ping me at steve (at) hibernate (dot) org

sebersole commented 9 years ago

Wow man. Thanks for the return favor. Ugh, but this has been my experience with the OSGi community as a whole...

lburgazzoli commented 9 years ago

@sebersole just sent you a few mails :-)

lburgazzoli commented 9 years ago

@sebersole any chance to get a PR :-) looking forward to use it

sebersole commented 9 years ago

Should finish up today. Have added a lot of features:

I am working on one more feature: support for "remapping" dependencies. E.g., in Hibernate I depend on Antlr, but rather than depending on <bundle>wrap:mvn:antlr/antlr/2.7.7</bundle> I am told it is better to depend on <bundle>mvn:org.apache.servicemix.bundles/org.apache.servicemix.bundles.antlr/2.7.7_5</bundle>. Part of the concern there is your use of pattern matching for selecting dependencies. It problem is it allows for matching multiple dependencies. In fact that is pretty much a mismatch for how the DSL extensions work now overall. All of the extensions assume one bundle{} definition per each dependency. To that end I actually plan to replace just using a String for selection:

karafFeatures {
features {
        myFeature {
            ...
            bundle {
                selector = 'antlr/*'
                ...
            }
        }
    }
}

with using group/name(/version?) for selection:

karafFeatures {
features {
        myFeature {
            ...
            bundle {
                match {
                    group = 'antlr'
                    module = 'antlr'
                    version = '2.7.7'
                }
                ...
            }
        }
    }
}

I also renamed the package to com.github.lburgazzoli.gradle.plugin.karaf.featureGen following better OSGi conventions oddly :)

sebersole commented 9 years ago

Another option for match is how Gradle does it in many places (consistency):

        bundle {
            match group: 'antlr', module: 'antlr', version: '2.7.7'
            ...
        }
lburgazzoli commented 9 years ago

Looks very promising. About matching I'd prefer the second version, maybe also

    match 'antlr:antlr:2.7.7'
sebersole commented 9 years ago

I don't know how Gradle parses them. I prefer to be explicit anyway

sebersole commented 9 years ago

Anyway, I went with the first option. It is backed by a Matcher class that then exposes matches methods

sebersole commented 9 years ago

Well actually I allowed both Map and Closure style config. If you want to investigate how to implement match 'antlr:antlr:2.7.7' as well (or to implement the String parsing yourself) thats fine

sebersole commented 9 years ago

Just sent the PR. As you apply that you will want to alter the build.gradle file as I added some entries to be able to publish SNAPSHOTs for testing; I left comments.

sebersole commented 9 years ago

Ultimately another feature that makes things easier is an option to define transitivity for a bundle{} definition. The intention is to allow saying that a certain dependency should be included but that its transitive dependencies should not be. Having these 2 separate does open up the possibility of a confusing mismatch where:

bundle {
    ...
    include = false
    transitive - true
}

Its misleading. A better option might be a 3-way switch, an enum:

enum Inclusion {
    /**
     * Include the matched dependency and its transitive deps as bundles
     */
    INCLUDE,
    /**
     * Include the matched dependency as a bundle, but exclude its transitive deps 
     */
    INCLUDE_NO_TRANSITIVITY,
    /**
     * Exclude the matched dependency and its transitive deps as bundles
     */
    EXCLUDE
}
lburgazzoli commented 9 years ago

Yes, the enum looks better. Do you have an example of the plugin in action so I can quickly test it and update the doc ?

sebersole commented 9 years ago

As I mentioned before I am using it in HIbernate : https://github.com/hibernate/hibernate-orm/blob/master/hibernate-osgi/hibernate-osgi.gradle#L112-L250

On Fri, May 15, 2015 at 1:55 AM, Luca Burgazzoli notifications@github.com wrote:

Yes, the enum looks better. Do you have an example of the plugin in action so I can quickly test it and update the doc ?

— Reply to this email directly or view it on GitHub https://github.com/lburgazzoli/lb-gradle-plugins/issues/4#issuecomment-102289948 .

lburgazzoli commented 9 years ago

I did a small change to your code to add the possibility to exclude bundles based on the group only and I've increased the build number to avoid confusions with the old version.

I've applied the plugin here https://github.com/lburgazzoli/lb-karaf-examples-jpa/blob/master/jpa-openjpa/build.gradle and I've noticed that:

I will have a look this week-end, if you have time before go ahead :-)

lburgazzoli commented 9 years ago

thx @sebersole for your awesome contribution