Implementation pluggability

[kohsuke]

I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
check with the team if people think this makes sense (hopefully they would.)

The implementation approach that I'm thinking of is to have
DefaultXXXFactory discover other XXXFactory implementations through
META-INF/services and use them as delegates on methods like
"getDeployerClass". So the container implementations that would like to
hook into this would implement their own factories and write
META-INF/services entry.

I feel container developers would have to write more than they should,
but OTOH the deployer/container/etc uses tuples for keys, so doing
anything simpler means introducing another factory-like mechanism, which
I hesitate.


If it makes sense, I'll first prepare a patch for, say,
DefaultDeployerFactory. If that looks all right, I'll do the same for
other factories.


I'd like to eventually resurrect the GlassFish support work, but I think
this should be the first step, so that various people in the GlassFish
project can eat the dogfood before it can be considered for the
inclusion into Cargo core.

--
Kohsuke Kawaguchi
Sun Microsystems                   kohsuke.kawaguchi@...

[Matt Wringe]

On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
> check with the team if people think this makes sense (hopefully they would.)

Replied to the jira about how cargo already supports custom containers
and configuration.

>
> The implementation approach that I'm thinking of is to have
> DefaultXXXFactory discover other XXXFactory implementations through
> META-INF/services and use them as delegates on methods like
> "getDeployerClass". So the container implementations that would like to
> hook into this would implement their own factories and write
> META-INF/services entry.

I do not have a lot of experience with jar service providers so I don't
really know how feasible this is.

I really don't like the current factory implementation and would ideally
like to see cargo automatically determine the proper classes to use
based on a class naming scheme (and cargo is already using a naming
scheme, ie Tomcat6xInstalledLocalContainer is the container
implementation for Tomcat6 for a local installed container).

With the current factory implementation its too easy to make mistakes.

> I feel container developers would have to write more than they should,
> but OTOH the deployer/container/etc uses tuples for keys, so doing
> anything simpler means introducing another factory-like mechanism, which
> I hesitate.
>
>
> If it makes sense, I'll first prepare a patch for, say,
> DefaultDeployerFactory. If that looks all right, I'll do the same for
> other factories.
>
>
> I'd like to eventually resurrect the GlassFish support work,

great, we need someone with glassfish knowledge

>  but I think
> this should be the first step, so that various people in the GlassFish
> project can eat the dogfood before it can be considered for the
> inclusion into Cargo core.

I wouldn't worry about trying to get a perfectly working implementation
before considering contributing to cargo. You can always start with some
simple commands and work from there. If needed we can always add
documentation about it being a feature preview.





---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[kohsuke]

Matt Wringe wrote:
> On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>> check with the team if people think this makes sense (hopefully they would.)
>
> Replied to the jira about how cargo already supports custom containers
> and configuration.

Thanks. I added my comment there, too. In short, I think what you showed
me is a good feature, but IMHO it's not suffice.


>> The implementation approach that I'm thinking of is to have
>> DefaultXXXFactory discover other XXXFactory implementations through
>> META-INF/services and use them as delegates on methods like
>> "getDeployerClass". So the container implementations that would like to
>> hook into this would implement their own factories and write
>> META-INF/services entry.
>
> I do not have a lot of experience with jar service providers so I don't
> really know how feasible this is.

I do have a lot of experience, so FWIW, rest assured that it will work.


> I really don't like the current factory implementation and would ideally
> like to see cargo automatically determine the proper classes to use
> based on a class naming scheme (and cargo is already using a naming
> scheme, ie Tomcat6xInstalledLocalContainer is the container
> implementation for Tomcat6 for a local installed container).
>
> With the current factory implementation its too easy to make mistakes.

I guess the only problem with the naming convention is that all the
implementations would have to live in the "org.codehaus" package.

But if the Cargo project is OK for random people to be putting classes
in this package, this is probably better than META-INF/services look up,
because the container implementor needs to write less.

Oh, and you'd have to rename some of the existing classes, I think, like
JBossXXX, because it would have inconsistent case convention. And the
naming convention would be somewhat contrived to make it fit with the
current class names.

>> I feel container developers would have to write more than they should,
>> but OTOH the deployer/container/etc uses tuples for keys, so doing
>> anything simpler means introducing another factory-like mechanism, which
>> I hesitate.
>>
>>
>> If it makes sense, I'll first prepare a patch for, say,
>> DefaultDeployerFactory. If that looks all right, I'll do the same for
>> other factories.
>>
>>
>> I'd like to eventually resurrect the GlassFish support work,
>
> great, we need someone with glassfish knowledge
>
>>  but I think
>> this should be the first step, so that various people in the GlassFish
>> project can eat the dogfood before it can be considered for the
>> inclusion into Cargo core.
>
> I wouldn't worry about trying to get a perfectly working implementation
> before considering contributing to cargo. You can always start with some
> simple commands and work from there. If needed we can always add
> documentation about it being a feature preview.

I think the feature still makes sense. Code that works is one reason,
but to be honest, a bigger reason for me is the checkstyle rule. Things
might have changed now, but back when I contributed the embedded Tomcat
support, the checkstyle rules in Cargo project was very draconian. It
was really painful for me to make sure every check in satisfies all the
check style rules.

By working on a separate module, I'm hoping that I can postpone doing
that work until we integrate that back into the core (and perhaps at
that point I can ask for other people's help.)

--
Kohsuke Kawaguchi
Sun Microsystems                   kohsuke.kawaguchi@...

[Matt Wringe]

On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:

> Matt Wringe wrote:
> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
> >> check with the team if people think this makes sense (hopefully they would.)
> >
> > Replied to the jira about how cargo already supports custom containers
> > and configuration.
>
> Thanks. I added my comment there, too. In short, I think what you showed
> me is a good feature, but IMHO it's not suffice.
>
>
> >> The implementation approach that I'm thinking of is to have
> >> DefaultXXXFactory discover other XXXFactory implementations through
> >> META-INF/services and use them as delegates on methods like
> >> "getDeployerClass". So the container implementations that would like to
> >> hook into this would implement their own factories and write
> >> META-INF/services entry.
> >
> > I do not have a lot of experience with jar service providers so I don't
> > really know how feasible this is.
>
> I do have a lot of experience, so FWIW, rest assured that it will work.

ok :) could you explain a bit more how exactly it would work then?

>
>
> > I really don't like the current factory implementation and would ideally
> > like to see cargo automatically determine the proper classes to use
> > based on a class naming scheme (and cargo is already using a naming
> > scheme, ie Tomcat6xInstalledLocalContainer is the container
> > implementation for Tomcat6 for a local installed container).
> >
> > With the current factory implementation its too easy to make
> mistakes.
>
> I guess the only problem with the naming convention is that all the
> implementations would have to live in the "org.codehaus" package.

well, it could just look at the class name and not the package name.

>
> But if the Cargo project is OK for random people to be putting classes
> in this package,

I don't think it would be ok for people to be using
org.codehaus.cargo.... packages as it would give the false impression on
the code origins. Please see my comment above.

>  this is probably better than META-INF/services look up,
> because the container implementor needs to write less.
>
> Oh, and you'd have to rename some of the existing classes, I think, like
> JBossXXX, because it would have inconsistent case convention. And the
> naming convention would be somewhat contrived to make it fit with the
> current class names.

I am not entirely sure how it would work, I was just throwing out ideas.

>
>
> >> I feel container developers would have to write more than they should,
> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
> >> anything simpler means introducing another factory-like mechanism, which
> >> I hesitate.
> >>
> >>
> >> If it makes sense, I'll first prepare a patch for, say,
> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
> >> other factories.
> >>
> >>
> >> I'd like to eventually resurrect the GlassFish support work,
> >
> > great, we need someone with glassfish knowledge
> >
> >>  but I think
> >> this should be the first step, so that various people in the GlassFish
> >> project can eat the dogfood before it can be considered for the
> >> inclusion into Cargo core.
> >
> > I wouldn't worry about trying to get a perfectly working implementation
> > before considering contributing to cargo. You can always start with some
> > simple commands and work from there. If needed we can always add
> > documentation about it being a feature preview.
>
> I think the feature still makes sense. Code that works is one reason,
> but to be honest, a bigger reason for me is the checkstyle rule. Things
> might have changed now, but back when I contributed the embedded Tomcat
> support, the checkstyle rules in Cargo project was very draconian.

hehe, yes, I agree here.

> It
> was really painful for me to make sure every check in satisfies all the
> check style rules.

Its really not that bad when you use an ide to manage it for you. Right
now in eclipse I can basically just highlight the code and tell eclipse
to refactor it to fix the guidelines. And it also highlights the issues
and tells you what is wrong with them (although I can't seem to figure
out how to tell eclipse where the suppression file is).

>
> By working on a separate module, I'm hoping that I can postpone doing
> that work until we integrate that back into the core (and perhaps at
> that point I can ask for other people's help.)

You can also easily disabled it by passing a specific -D option during a
build.

If you are really worried about the checkstyle, we could even
temporarily disable it for the Glassfish part.


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[kohsuke]

Matt Wringe wrote:

> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
>> Matt Wringe wrote:
>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>> >> check with the team if people think this makes sense (hopefully they would.)
>> >
>> > Replied to the jira about how cargo already supports custom containers
>> > and configuration.
>>
>> Thanks. I added my comment there, too. In short, I think what you showed
>> me is a good feature, but IMHO it's not suffice.
>>
>>
>> >> The implementation approach that I'm thinking of is to have
>> >> DefaultXXXFactory discover other XXXFactory implementations through
>> >> META-INF/services and use them as delegates on methods like
>> >> "getDeployerClass". So the container implementations that would like to
>> >> hook into this would implement their own factories and write
>> >> META-INF/services entry.
>> >
>> > I do not have a lot of experience with jar service providers so I don't
>> > really know how feasible this is.
>>
>> I do have a lot of experience, so FWIW, rest assured that it will work.
> ok :) could you explain a bit more how exactly it would work then?

This is a standard convention used widely everywhere, such as in many
JCP APIs.

The technique is for the use cases where typically one wants to discover
implementations of some contract defined in the API. For example, in
JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
implementations (like Xerces) without having any prior knowledge about
such an implementation.

Here is the technique. The base contract is defined in terms of a Java
class/interface. Suppose this contract has the fully-qualified name of
org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
that like to be discovered will put a file with the name
"META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
in the jar file. This file is UTF-8 encoded, and it lists fully
qualified names of classes that implement this base interface/class.
Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
and its content may read "org.glassfish.cargo.DeployerFactoryImpl"

The discovery process takes one ClassLoader as a parameter. This
parameter is normally defaulted to
Thread.currentThread().getContextClassLoader(), but in more complex
scenarios the caller do need to be able to pass it in.

The API then lists up all
"META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
from resources, parse them all, and list up all the classes. It'll
instantiate them and use them, with suitable error handling in-between.

commons-discovery defines code to do this. Writing it from scratch is
also rather easy.

The end result is that all that needed to be done by the user is put
"cargo-glassfish-impl.jar" in the classpath.

>> > I really don't like the current factory implementation and would ideally
>> > like to see cargo automatically determine the proper classes to use
>> > based on a class naming scheme (and cargo is already using a naming
>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
>> > implementation for Tomcat6 for a local installed container).
>> >
>> > With the current factory implementation its too easy to make
>> mistakes.
>>
>> I guess the only problem with the naming convention is that all the
>> implementations would have to live in the "org.codehaus" package.
>
> well, it could just look at the class name and not the package name.

OK, so your idea of pluggability must be bit different than mine. I
suspect your approach is that somehow container implementations would
tell the factory about its class names, and the factory would figure out
the ID and the type from the class name. But this is missing how the
containers tell the factory their class names in the first place,
without asking the users' help --- that is the part that I'm trying to
eliminate.

What I was thinking was that the factory implementation now only know
about a class name convention, and whenever someone requests an
instantiation from some parameters, it just blindly tries to create a
class of certain names. If the class is there, it works, otherwise, it
won't.

So in the latter scheme, while you can try more than one class names,
you cannot really allow arbitrary package names or class names, simply
because the factory can't know about them all.


>> But if the Cargo project is OK for random people to be putting classes
>> in this package,
>
> I don't think it would be ok for people to be using
> org.codehaus.cargo.... packages as it would give the false impression on
> the code origins. Please see my comment above.

OK. Then that eliminates convention-only approach.


>>  this is probably better than META-INF/services look up,
>> because the container implementor needs to write less.
>>
>> Oh, and you'd have to rename some of the existing classes, I think, like
>> JBossXXX, because it would have inconsistent case convention. And the
>> naming convention would be somewhat contrived to make it fit with the
>> current class names.
>
> I am not entirely sure how it would work, I was just throwing out ideas.

OK.

>> >> I feel container developers would have to write more than they should,
>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
>> >> anything simpler means introducing another factory-like mechanism, which
>> >> I hesitate.
>> >>
>> >>
>> >> If it makes sense, I'll first prepare a patch for, say,
>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
>> >> other factories.
>> >>
>> >>
>> >> I'd like to eventually resurrect the GlassFish support work,
>> >
>> > great, we need someone with glassfish knowledge
>> >
>> >>  but I think
>> >> this should be the first step, so that various people in the GlassFish
>> >> project can eat the dogfood before it can be considered for the
>> >> inclusion into Cargo core.
>> >
>> > I wouldn't worry about trying to get a perfectly working implementation
>> > before considering contributing to cargo. You can always start with some
>> > simple commands and work from there. If needed we can always add
>> > documentation about it being a feature preview.
>>
>> I think the feature still makes sense. Code that works is one reason,
>> but to be honest, a bigger reason for me is the checkstyle rule. Things
>> might have changed now, but back when I contributed the embedded Tomcat
>> support, the checkstyle rules in Cargo project was very draconian.
>
> hehe, yes, I agree here.
>
>> It
>> was really painful for me to make sure every check in satisfies all the
>> check style rules.
>
> Its really not that bad when you use an ide to manage it for you. Right
> now in eclipse I can basically just highlight the code and tell eclipse
> to refactor it to fix the guidelines. And it also highlights the issues
> and tells you what is wrong with them (although I can't seem to figure
> out how to tell eclipse where the suppression file is).

Yes, I was hoping that at least there are some IDEs that understand
checkstyle rule files. Unfortunately, I don't think such a feature is
available on my IDE, hence the preference to postpone till the end.


>> By working on a separate module, I'm hoping that I can postpone doing
>> that work until we integrate that back into the core (and perhaps at
>> that point I can ask for other people's help.)
>
> You can also easily disabled it by passing a specific -D option during a
> build.

Yes. I'm aware of this, but again, the rule back then was that for me to
make a commit, it better passes the checkstyle.

> If you are really worried about the checkstyle, we could even
> temporarily disable it for the Glassfish part.

That's good to know :-)

But I really think it's better for the Cargo project as a whole it it
lets people write containers outside the Cargo project.

Think about Ant tasks, for example. If Ant didn't define a way for
people to write custom tasks outside the Apache Ant project itself, I
don't think it would have been this successful.

--
Kohsuke Kawaguchi
Sun Microsystems                   kohsuke.kawaguchi@...

[kohsuke]

Does this direction make sense?

I guess what I'm trying to avoid here is to spend time implementing this
feature, only to learn later that you don't agree with the direction.


Kohsuke Kawaguchi wrote:

> Matt Wringe wrote:
>> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
>>> Matt Wringe wrote:
>>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>>> >> check with the team if people think this makes sense (hopefully they would.)
>>> >
>>> > Replied to the jira about how cargo already supports custom containers
>>> > and configuration.
>>>
>>> Thanks. I added my comment there, too. In short, I think what you showed
>>> me is a good feature, but IMHO it's not suffice.
>>>
>>>
>>> >> The implementation approach that I'm thinking of is to have
>>> >> DefaultXXXFactory discover other XXXFactory implementations through
>>> >> META-INF/services and use them as delegates on methods like
>>> >> "getDeployerClass". So the container implementations that would like to
>>> >> hook into this would implement their own factories and write
>>> >> META-INF/services entry.
>>> >
>>> > I do not have a lot of experience with jar service providers so I don't
>>> > really know how feasible this is.
>>>
>>> I do have a lot of experience, so FWIW, rest assured that it will work.
>> ok :) could you explain a bit more how exactly it would work then?
>
> This is a standard convention used widely everywhere, such as in many
> JCP APIs.
>
> The technique is for the use cases where typically one wants to discover
> implementations of some contract defined in the API. For example, in
> JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
> implementations (like Xerces) without having any prior knowledge about
> such an implementation.
>
> Here is the technique. The base contract is defined in terms of a Java
> class/interface. Suppose this contract has the fully-qualified name of
> org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
> that like to be discovered will put a file with the name
> "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> in the jar file. This file is UTF-8 encoded, and it lists fully
> qualified names of classes that implement this base interface/class.
> Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
> and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
>
> The discovery process takes one ClassLoader as a parameter. This
> parameter is normally defaulted to
> Thread.currentThread().getContextClassLoader(), but in more complex
> scenarios the caller do need to be able to pass it in.
>
> The API then lists up all
> "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> from resources, parse them all, and list up all the classes. It'll
> instantiate them and use them, with suitable error handling in-between.
>
> commons-discovery defines code to do this. Writing it from scratch is
> also rather easy.
>
> The end result is that all that needed to be done by the user is put
> "cargo-glassfish-impl.jar" in the classpath.
>
>
>>> > I really don't like the current factory implementation and would ideally
>>> > like to see cargo automatically determine the proper classes to use
>>> > based on a class naming scheme (and cargo is already using a naming
>>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
>>> > implementation for Tomcat6 for a local installed container).
>>> >
>>> > With the current factory implementation its too easy to make
>>> mistakes.
>>>
>>> I guess the only problem with the naming convention is that all the
>>> implementations would have to live in the "org.codehaus" package.
>>
>> well, it could just look at the class name and not the package name.
>
> OK, so your idea of pluggability must be bit different than mine. I
> suspect your approach is that somehow container implementations would
> tell the factory about its class names, and the factory would figure out
> the ID and the type from the class name. But this is missing how the
> containers tell the factory their class names in the first place,
> without asking the users' help --- that is the part that I'm trying to
> eliminate.
>
> What I was thinking was that the factory implementation now only know
> about a class name convention, and whenever someone requests an
> instantiation from some parameters, it just blindly tries to create a
> class of certain names. If the class is there, it works, otherwise, it
> won't.
>
> So in the latter scheme, while you can try more than one class names,
> you cannot really allow arbitrary package names or class names, simply
> because the factory can't know about them all.
>
>
>>> But if the Cargo project is OK for random people to be putting classes
>>> in this package,
>>
>> I don't think it would be ok for people to be using
>> org.codehaus.cargo.... packages as it would give the false impression on
>> the code origins. Please see my comment above.
>
> OK. Then that eliminates convention-only approach.
>
>
>>>  this is probably better than META-INF/services look up,
>>> because the container implementor needs to write less.
>>>
>>> Oh, and you'd have to rename some of the existing classes, I think, like
>>> JBossXXX, because it would have inconsistent case convention. And the
>>> naming convention would be somewhat contrived to make it fit with the
>>> current class names.
>>
>> I am not entirely sure how it would work, I was just throwing out ideas.
>
> OK.
>
>
>>> >> I feel container developers would have to write more than they should,
>>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
>>> >> anything simpler means introducing another factory-like mechanism, which
>>> >> I hesitate.
>>> >>
>>> >>
>>> >> If it makes sense, I'll first prepare a patch for, say,
>>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
>>> >> other factories.
>>> >>
>>> >>
>>> >> I'd like to eventually resurrect the GlassFish support work,
>>> >
>>> > great, we need someone with glassfish knowledge
>>> >
>>> >>  but I think
>>> >> this should be the first step, so that various people in the GlassFish
>>> >> project can eat the dogfood before it can be considered for the
>>> >> inclusion into Cargo core.
>>> >
>>> > I wouldn't worry about trying to get a perfectly working implementation
>>> > before considering contributing to cargo. You can always start with some
>>> > simple commands and work from there. If needed we can always add
>>> > documentation about it being a feature preview.
>>>
>>> I think the feature still makes sense. Code that works is one reason,
>>> but to be honest, a bigger reason for me is the checkstyle rule. Things
>>> might have changed now, but back when I contributed the embedded Tomcat
>>> support, the checkstyle rules in Cargo project was very draconian.
>>
>> hehe, yes, I agree here.
>>
>>> It
>>> was really painful for me to make sure every check in satisfies all the
>>> check style rules.
>>
>> Its really not that bad when you use an ide to manage it for you. Right
>> now in eclipse I can basically just highlight the code and tell eclipse
>> to refactor it to fix the guidelines. And it also highlights the issues
>> and tells you what is wrong with them (although I can't seem to figure
>> out how to tell eclipse where the suppression file is).
>
> Yes, I was hoping that at least there are some IDEs that understand
> checkstyle rule files. Unfortunately, I don't think such a feature is
> available on my IDE, hence the preference to postpone till the end.
>
>
>>> By working on a separate module, I'm hoping that I can postpone doing
>>> that work until we integrate that back into the core (and perhaps at
>>> that point I can ask for other people's help.)
>>
>> You can also easily disabled it by passing a specific -D option during a
>> build.
>
> Yes. I'm aware of this, but again, the rule back then was that for me to
> make a commit, it better passes the checkstyle.
>
>> If you are really worried about the checkstyle, we could even
>> temporarily disable it for the Glassfish part.
>
> That's good to know :-)
>
> But I really think it's better for the Cargo project as a whole it it
> lets people write containers outside the Cargo project.
>
> Think about Ant tasks, for example. If Ant didn't define a way for
> people to write custom tasks outside the Apache Ant project itself, I
> don't think it would have been this successful.
>

--
Kohsuke Kawaguchi
Sun Microsystems                   kohsuke.kawaguchi@...

[Matt Wringe]

On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
> Does this direction make sense?
>
> I guess what I'm trying to avoid here is to spend time implementing this
> feature, only to learn later that you don't agree with the direction.
>
it makes sense to me :)

> Kohsuke Kawaguchi wrote:
> > Matt Wringe wrote:
> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
> >>> Matt Wringe wrote:
> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
> >>> >> check with the team if people think this makes sense (hopefully they would.)
> >>> >
> >>> > Replied to the jira about how cargo already supports custom containers
> >>> > and configuration.
> >>>
> >>> Thanks. I added my comment there, too. In short, I think what you showed
> >>> me is a good feature, but IMHO it's not suffice.
> >>>
> >>>
> >>> >> The implementation approach that I'm thinking of is to have
> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
> >>> >> META-INF/services and use them as delegates on methods like
> >>> >> "getDeployerClass". So the container implementations that would like to
> >>> >> hook into this would implement their own factories and write
> >>> >> META-INF/services entry.
> >>> >
> >>> > I do not have a lot of experience with jar service providers so I don't
> >>> > really know how feasible this is.
> >>>
> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
> >> ok :) could you explain a bit more how exactly it would work then?
> >
> > This is a standard convention used widely everywhere, such as in many
> > JCP APIs.
> >
> > The technique is for the use cases where typically one wants to discover
> > implementations of some contract defined in the API. For example, in
> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
> > implementations (like Xerces) without having any prior knowledge about
> > such an implementation.
> >
> > Here is the technique. The base contract is defined in terms of a Java
> > class/interface. Suppose this contract has the fully-qualified name of
> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
> > that like to be discovered will put a file with the name
> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> > in the jar file. This file is UTF-8 encoded, and it lists fully
> > qualified names of classes that implement this base interface/class.
> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
> >
> > The discovery process takes one ClassLoader as a parameter. This
> > parameter is normally defaulted to
> > Thread.currentThread().getContextClassLoader(), but in more complex
> > scenarios the caller do need to be able to pass it in.
> >
> > The API then lists up all
> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> > from resources, parse them all, and list up all the classes. It'll
> > instantiate them and use them, with suitable error handling in-between.
> >
> > commons-discovery defines code to do this. Writing it from scratch is
> > also rather easy.
> >
> > The end result is that all that needed to be done by the user is put
> > "cargo-glassfish-impl.jar" in the classpath.
> >
> >
> >>> > I really don't like the current factory implementation and would ideally
> >>> > like to see cargo automatically determine the proper classes to use
> >>> > based on a class naming scheme (and cargo is already using a naming
> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
> >>> > implementation for Tomcat6 for a local installed container).
> >>> >
> >>> > With the current factory implementation its too easy to make
> >>> mistakes.
> >>>
> >>> I guess the only problem with the naming convention is that all the
> >>> implementations would have to live in the "org.codehaus" package.
> >>
> >> well, it could just look at the class name and not the package name.
> >
> > OK, so your idea of pluggability must be bit different than mine. I
> > suspect your approach is that somehow container implementations would
> > tell the factory about its class names, and the factory would figure out
> > the ID and the type from the class name. But this is missing how the
> > containers tell the factory their class names in the first place,
> > without asking the users' help --- that is the part that I'm trying to
> > eliminate.
> >
> > What I was thinking was that the factory implementation now only know
> > about a class name convention, and whenever someone requests an
> > instantiation from some parameters, it just blindly tries to create a
> > class of certain names. If the class is there, it works, otherwise, it
> > won't.
> >
> > So in the latter scheme, while you can try more than one class names,
> > you cannot really allow arbitrary package names or class names, simply
> > because the factory can't know about them all.
> >
> >
> >>> But if the Cargo project is OK for random people to be putting classes
> >>> in this package,
> >>
> >> I don't think it would be ok for people to be using
> >> org.codehaus.cargo.... packages as it would give the false impression on
> >> the code origins. Please see my comment above.
> >
> > OK. Then that eliminates convention-only approach.
> >
> >
> >>>  this is probably better than META-INF/services look up,
> >>> because the container implementor needs to write less.
> >>>
> >>> Oh, and you'd have to rename some of the existing classes, I think, like
> >>> JBossXXX, because it would have inconsistent case convention. And the
> >>> naming convention would be somewhat contrived to make it fit with the
> >>> current class names.
> >>
> >> I am not entirely sure how it would work, I was just throwing out ideas.
> >
> > OK.
> >
> >
> >>> >> I feel container developers would have to write more than they should,
> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
> >>> >> anything simpler means introducing another factory-like mechanism, which
> >>> >> I hesitate.
> >>> >>
> >>> >>
> >>> >> If it makes sense, I'll first prepare a patch for, say,
> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
> >>> >> other factories.
> >>> >>
> >>> >>
> >>> >> I'd like to eventually resurrect the GlassFish support work,
> >>> >
> >>> > great, we need someone with glassfish knowledge
> >>> >
> >>> >>  but I think
> >>> >> this should be the first step, so that various people in the GlassFish
> >>> >> project can eat the dogfood before it can be considered for the
> >>> >> inclusion into Cargo core.
> >>> >
> >>> > I wouldn't worry about trying to get a perfectly working implementation
> >>> > before considering contributing to cargo. You can always start with some
> >>> > simple commands and work from there. If needed we can always add
> >>> > documentation about it being a feature preview.
> >>>
> >>> I think the feature still makes sense. Code that works is one reason,
> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
> >>> might have changed now, but back when I contributed the embedded Tomcat
> >>> support, the checkstyle rules in Cargo project was very draconian.
> >>
> >> hehe, yes, I agree here.
> >>
> >>> It
> >>> was really painful for me to make sure every check in satisfies all the
> >>> check style rules.
> >>
> >> Its really not that bad when you use an ide to manage it for you. Right
> >> now in eclipse I can basically just highlight the code and tell eclipse
> >> to refactor it to fix the guidelines. And it also highlights the issues
> >> and tells you what is wrong with them (although I can't seem to figure
> >> out how to tell eclipse where the suppression file is).
> >
> > Yes, I was hoping that at least there are some IDEs that understand
> > checkstyle rule files. Unfortunately, I don't think such a feature is
> > available on my IDE, hence the preference to postpone till the end.
> >
> >
> >>> By working on a separate module, I'm hoping that I can postpone doing
> >>> that work until we integrate that back into the core (and perhaps at
> >>> that point I can ask for other people's help.)
> >>
> >> You can also easily disabled it by passing a specific -D option during a
> >> build.
> >
> > Yes. I'm aware of this, but again, the rule back then was that for me to
> > make a commit, it better passes the checkstyle.
> >
> >> If you are really worried about the checkstyle, we could even
> >> temporarily disable it for the Glassfish part.
> >
> > That's good to know :-)
> >
> > But I really think it's better for the Cargo project as a whole it it
> > lets people write containers outside the Cargo project.
> >
> > Think about Ant tasks, for example. If Ant didn't define a way for
> > people to write custom tasks outside the Apache Ant project itself, I
> > don't think it would have been this successful.
> >
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[kohsuke]

Matt Wringe wrote:
> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
>> Does this direction make sense?
>>
>> I guess what I'm trying to avoid here is to spend time implementing this
>> feature, only to learn later that you don't agree with the direction.
>>
> it makes sense to me :)

Attached is the patch for this change that covers Deployable and
DeployableFactory. If this looks OK, I'll proceed with all the other
factory types.

See the test code for how the container implementation would look like.

>> Kohsuke Kawaguchi wrote:
>> > Matt Wringe wrote:
>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
>> >>> Matt Wringe wrote:
>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>> >>> >> check with the team if people think this makes sense (hopefully they would.)
>> >>> >
>> >>> > Replied to the jira about how cargo already supports custom containers
>> >>> > and configuration.
>> >>>
>> >>> Thanks. I added my comment there, too. In short, I think what you showed
>> >>> me is a good feature, but IMHO it's not suffice.
>> >>>
>> >>>
>> >>> >> The implementation approach that I'm thinking of is to have
>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
>> >>> >> META-INF/services and use them as delegates on methods like
>> >>> >> "getDeployerClass". So the container implementations that would like to
>> >>> >> hook into this would implement their own factories and write
>> >>> >> META-INF/services entry.
>> >>> >
>> >>> > I do not have a lot of experience with jar service providers so I don't
>> >>> > really know how feasible this is.
>> >>>
>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
>> >> ok :) could you explain a bit more how exactly it would work then?
>> >
>> > This is a standard convention used widely everywhere, such as in many
>> > JCP APIs.
>> >
>> > The technique is for the use cases where typically one wants to discover
>> > implementations of some contract defined in the API. For example, in
>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
>> > implementations (like Xerces) without having any prior knowledge about
>> > such an implementation.
>> >
>> > Here is the technique. The base contract is defined in terms of a Java
>> > class/interface. Suppose this contract has the fully-qualified name of
>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
>> > that like to be discovered will put a file with the name
>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>> > in the jar file. This file is UTF-8 encoded, and it lists fully
>> > qualified names of classes that implement this base interface/class.
>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
>> >
>> > The discovery process takes one ClassLoader as a parameter. This
>> > parameter is normally defaulted to
>> > Thread.currentThread().getContextClassLoader(), but in more complex
>> > scenarios the caller do need to be able to pass it in.
>> >
>> > The API then lists up all
>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>> > from resources, parse them all, and list up all the classes. It'll
>> > instantiate them and use them, with suitable error handling in-between.
>> >
>> > commons-discovery defines code to do this. Writing it from scratch is
>> > also rather easy.
>> >
>> > The end result is that all that needed to be done by the user is put
>> > "cargo-glassfish-impl.jar" in the classpath.
>> >
>> >
>> >>> > I really don't like the current factory implementation and would ideally
>> >>> > like to see cargo automatically determine the proper classes to use
>> >>> > based on a class naming scheme (and cargo is already using a naming
>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
>> >>> > implementation for Tomcat6 for a local installed container).
>> >>> >
>> >>> > With the current factory implementation its too easy to make
>> >>> mistakes.
>> >>>
>> >>> I guess the only problem with the naming convention is that all the
>> >>> implementations would have to live in the "org.codehaus" package.
>> >>
>> >> well, it could just look at the class name and not the package name.
>> >
>> > OK, so your idea of pluggability must be bit different than mine. I
>> > suspect your approach is that somehow container implementations would
>> > tell the factory about its class names, and the factory would figure out
>> > the ID and the type from the class name. But this is missing how the
>> > containers tell the factory their class names in the first place,
>> > without asking the users' help --- that is the part that I'm trying to
>> > eliminate.
>> >
>> > What I was thinking was that the factory implementation now only know
>> > about a class name convention, and whenever someone requests an
>> > instantiation from some parameters, it just blindly tries to create a
>> > class of certain names. If the class is there, it works, otherwise, it
>> > won't.
>> >
>> > So in the latter scheme, while you can try more than one class names,
>> > you cannot really allow arbitrary package names or class names, simply
>> > because the factory can't know about them all.
>> >
>> >
>> >>> But if the Cargo project is OK for random people to be putting classes
>> >>> in this package,
>> >>
>> >> I don't think it would be ok for people to be using
>> >> org.codehaus.cargo.... packages as it would give the false impression on
>> >> the code origins. Please see my comment above.
>> >
>> > OK. Then that eliminates convention-only approach.
>> >
>> >
>> >>>  this is probably better than META-INF/services look up,
>> >>> because the container implementor needs to write less.
>> >>>
>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
>> >>> JBossXXX, because it would have inconsistent case convention. And the
>> >>> naming convention would be somewhat contrived to make it fit with the
>> >>> current class names.
>> >>
>> >> I am not entirely sure how it would work, I was just throwing out ideas.
>> >
>> > OK.
>> >
>> >
>> >>> >> I feel container developers would have to write more than they should,
>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
>> >>> >> anything simpler means introducing another factory-like mechanism, which
>> >>> >> I hesitate.
>> >>> >>
>> >>> >>
>> >>> >> If it makes sense, I'll first prepare a patch for, say,
>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
>> >>> >> other factories.
>> >>> >>
>> >>> >>
>> >>> >> I'd like to eventually resurrect the GlassFish support work,
>> >>> >
>> >>> > great, we need someone with glassfish knowledge
>> >>> >
>> >>> >>  but I think
>> >>> >> this should be the first step, so that various people in the GlassFish
>> >>> >> project can eat the dogfood before it can be considered for the
>> >>> >> inclusion into Cargo core.
>> >>> >
>> >>> > I wouldn't worry about trying to get a perfectly working implementation
>> >>> > before considering contributing to cargo. You can always start with some
>> >>> > simple commands and work from there. If needed we can always add
>> >>> > documentation about it being a feature preview.
>> >>>
>> >>> I think the feature still makes sense. Code that works is one reason,
>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
>> >>> might have changed now, but back when I contributed the embedded Tomcat
>> >>> support, the checkstyle rules in Cargo project was very draconian.
>> >>
>> >> hehe, yes, I agree here.
>> >>
>> >>> It
>> >>> was really painful for me to make sure every check in satisfies all the
>> >>> check style rules.
>> >>
>> >> Its really not that bad when you use an ide to manage it for you. Right
>> >> now in eclipse I can basically just highlight the code and tell eclipse
>> >> to refactor it to fix the guidelines. And it also highlights the issues
>> >> and tells you what is wrong with them (although I can't seem to figure
>> >> out how to tell eclipse where the suppression file is).
>> >
>> > Yes, I was hoping that at least there are some IDEs that understand
>> > checkstyle rule files. Unfortunately, I don't think such a feature is
>> > available on my IDE, hence the preference to postpone till the end.
>> >
>> >
>> >>> By working on a separate module, I'm hoping that I can postpone doing
>> >>> that work until we integrate that back into the core (and perhaps at
>> >>> that point I can ask for other people's help.)
>> >>
>> >> You can also easily disabled it by passing a specific -D option during a
>> >> build.
>> >
>> > Yes. I'm aware of this, but again, the rule back then was that for me to
>> > make a commit, it better passes the checkstyle.
>> >
>> >> If you are really worried about the checkstyle, we could even
>> >> temporarily disable it for the Glassfish part.
>> >
>> > That's good to know :-)
>> >
>> > But I really think it's better for the Cargo project as a whole it it
>> > lets people write containers outside the Cargo project.
>> >
>> > Think about Ant tasks, for example. If Ant didn't define a way for
>> > people to write custom tasks outside the Apache Ant project itself, I
>> > don't think it would have been this successful.
>> >
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>
>

--
Kohsuke Kawaguchi
Sun Microsystems                   http://weblogs.java.net/blog/kohsuke/

Index: src/test/java/org/codehaus/cargo/generic/FactoryRegistryTest.java
===================================================================
--- src/test/java/org/codehaus/cargo/generic/FactoryRegistryTest.java (revision 0)
+++ src/test/java/org/codehaus/cargo/generic/FactoryRegistryTest.java (revision 0)
@@ -0,0 +1,41 @@
+/*
+ * ========================================================================
+ *
+ * Copyright 2006-2008 Vincent Massol.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * ========================================================================
+ */
+package org.codehaus.cargo.generic;
+
+import junit.framework.TestCase;
+import org.codehaus.cargo.generic.deployable.DefaultDeployableFactory;
+import org.codehaus.cargo.container.deployable.DeployableType;
+import org.codehaus.cargo.container.deployable.Deployable;
+
+/**
+ * Tests the discovery behavior.
+ *
+ * The class name can't be {@code AbstractFactoryRegistryTest} or else the test will be skipped.
+ *
+ * @version $Id: $
+ */
+public class FactoryRegistryTest extends TestCase
+{
+    public void test1() {
+        DefaultDeployableFactory f = new DefaultDeployableFactory(getClass().getClassLoader());
+        Deployable war = f.createDeployable("super-container", ".", DeployableType.WAR);
+        assertTrue(war instanceof SuperContainerWar);
+    }
+}
Index: src/test/java/org/codehaus/cargo/generic/TestFactoryRegistry.java
===================================================================
--- src/test/java/org/codehaus/cargo/generic/TestFactoryRegistry.java (revision 0)
+++ src/test/java/org/codehaus/cargo/generic/TestFactoryRegistry.java (revision 0)
@@ -0,0 +1,36 @@
+/*
+ * ========================================================================
+ *
+ * Copyright 2006-2008 Vincent Massol.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * ========================================================================
+ */
+package org.codehaus.cargo.generic;
+
+import org.codehaus.cargo.generic.deployable.DeployableFactory;
+import org.codehaus.cargo.container.deployable.DeployableType;
+
+/**
+ * To be discovered by {@link AbstractFactoryRegistry}.
+ *
+ * @version $Id: $
+ */
+public class TestFactoryRegistry extends AbstractFactoryRegistry
+{
+    protected void register(DeployableFactory factory)
+    {
+        factory.registerDeployable("super-container", DeployableType.WAR, SuperContainerWar.class);
+    }
+}
Index: src/test/java/org/codehaus/cargo/generic/SuperContainerWar.java
===================================================================
--- src/test/java/org/codehaus/cargo/generic/SuperContainerWar.java (revision 0)
+++ src/test/java/org/codehaus/cargo/generic/SuperContainerWar.java (revision 0)
@@ -0,0 +1,35 @@
+/*
+ * ========================================================================
+ *
+ * Copyright 2006-2008 Vincent Massol.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * ========================================================================
+ */
+package org.codehaus.cargo.generic;
+
+import org.codehaus.cargo.container.deployable.WAR;
+
+/**
+ * Used by {@link TestFactoryRegistry} to see if the discovery succeeds.
+ *
+ * @version $Id: $
+ */
+public class SuperContainerWar extends WAR
+{
+    public SuperContainerWar(String war)
+    {
+        super(war);
+    }
+}
Index: src/test/resources/META-INF/services/org.codehaus.cargo.generic.AbstractFactoryRegistry
===================================================================
--- src/test/resources/META-INF/services/org.codehaus.cargo.generic.AbstractFactoryRegistry (revision 0)
+++ src/test/resources/META-INF/services/org.codehaus.cargo.generic.AbstractFactoryRegistry (revision 0)
@@ -0,0 +1,2 @@
+# test for ABstractFactoryRegistry
+org.codehaus.cargo.generic.TestFactoryRegistry
\ No newline at end of file
Index: src/main/java/org/codehaus/cargo/generic/deployable/DefaultDeployableFactory.java
===================================================================
--- src/main/java/org/codehaus/cargo/generic/deployable/DefaultDeployableFactory.java (revision 1677)
+++ src/main/java/org/codehaus/cargo/generic/deployable/DefaultDeployableFactory.java (working copy)
@@ -30,6 +30,7 @@
 import org.codehaus.cargo.generic.spi.AbstractIntrospectionGenericHintFactory;
 import org.codehaus.cargo.generic.internal.util.RegistrationKey;
 import org.codehaus.cargo.generic.internal.util.SimpleContainerIdentity;
+import org.codehaus.cargo.generic.AbstractFactoryRegistry;
 
 import java.lang.reflect.Constructor;
 
@@ -60,12 +61,25 @@
          */
         public String deployable;
     }
-    
+
     /**
      * Register deployable classes mappings.
      */
     public DefaultDeployableFactory()
     {
+        this(null);
+    }
+
+    /**
+     * Register deployable classes mappings.
+     *
+     * @param classLoader
+     *      ClassLoader to discover implementations from. See
+     *      {@link AbstractFactoryRegistry#register(ClassLoader, DeployableFactory)}
+     *      for the details of what this value means.
+     */
+    public DefaultDeployableFactory(ClassLoader classLoader)
+    {
         // The WAR, EJB and EAR deployables are registered by default against all containers.
         // In order not to have to individually register against each container id we
         // create a fictitious default container id.
@@ -91,6 +105,8 @@
             "org.codehaus.cargo.container.geronimo.deployable.GeronimoEAR");
 
         // TODO: Register JBossWAR here when we add JBoss support
+
+        AbstractFactoryRegistry.register(classLoader, this);
     }
 
     /**
Index: src/main/java/org/codehaus/cargo/generic/AbstractFactoryRegistry.java
===================================================================
--- src/main/java/org/codehaus/cargo/generic/AbstractFactoryRegistry.java (revision 0)
+++ src/main/java/org/codehaus/cargo/generic/AbstractFactoryRegistry.java (revision 0)
@@ -0,0 +1,139 @@
+/*
+ * ========================================================================
+ *
+ * Copyright 2006-2008 Vincent Massol.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * ========================================================================
+ */
+package org.codehaus.cargo.generic;
+
+import org.apache.commons.discovery.jdk.JDKHooks;
+import org.apache.commons.discovery.resource.ClassLoaders;
+import org.apache.commons.discovery.tools.SPInterface;
+import org.apache.commons.discovery.tools.Service;
+import org.codehaus.cargo.generic.deployable.DeployableFactory;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.Iterator;
+import java.util.List;
+
+/**
+ * SPI to be implemented by container implementation to register implementations
+ * to their factories.
+ *
+ * <p>
+ * This class also provides static methods (to be used primarily within Cargo
+ * but can be also called directly by client apps) to discover all the implementations
+ * and register them to factories.
+ *
+ * <p>
+ * Client apps should normally use {@code DefaultXXXFactory} classes,
+ * like {@link org.codehaus.cargo.generic.deployable.DefaultDeployableFactory},
+ * which internally uses the discovery mechanism
+ *
+ * <p>
+ * Container implementors should override the 1-arg {@code register} methods to register
+ * its implementations to the given factory.
+ *
+ * @version $Id: $
+ */
+public abstract class AbstractFactoryRegistry
+{
+    /**
+     * Discovers all the {@link org.codehaus.cargo.container.deployable.Deployable}s and
+     * adds them to the given {@link DeployableFactory}.
+     *
+     * <p>
+     * The discovery is done by
+     * <a href="http://java.sun.com/j2se/1.3/docs/guide/jar/jar.html">the standard service loader
+     * mechanism</a>, by looking for
+     * <tt>/META-INF/services/org.codehaus.cargo.generic.AbstractFactoryRegistry</tt> files.
+     *
+     * @param classLoader
+     *      The class loader to be used to search service provide configuration files.
+     *      If null, the value defaults to the thread context classloader. If that's also null,
+     *      the value defaults to the class loader that loaded {@link FactoryRegistry}.
+     *      In the rare circumstance of that also being null (which means Cargo is loaded in the
+     *      bootstrap classloader), the value defaults to the system class loader.
+     * @param factory
+     *      The factory whose {@code register} method is invoked to receive
+     *      {@link org.codehaus.cargo.container.deployable.Deployable}s that
+     *      are discovered.
+     */
+    public static void register(ClassLoader classLoader, DeployableFactory factory)
+    {
+        for (Iterator itr = list(classLoader).iterator(); itr.hasNext();)
+        {
+            ((AbstractFactoryRegistry) itr.next()).register(factory);
+        }
+    }
+
+    /**
+     * Registers {@link org.codehaus.cargo.container.deployable.Deployable} implementations
+     * to the given {@link DeployableFactory}.
+     *
+     * @param factory
+     *      See {@link #register(ClassLoader, DeployableFactory)}
+     */
+    protected abstract void register(DeployableFactory factory);
+
+    /**
+     * Lists up {@link FactoryRegistry}s that are discovered.
+     *
+     * @param classLoader
+     *      See {@link #register(ClassLoader, DeployableFactory)} for more details.
+     * @return always non-null but can be empty.
+     */
+    private static List list(ClassLoader classLoader)
+    {
+        ClassLoader cl = classLoader;
+        if (cl == null)
+        {
+            cl = Thread.currentThread().getContextClassLoader();
+        }
+        if (cl == null)
+        {
+            cl = AbstractFactoryRegistry.class.getClassLoader();
+        }
+        if (cl == null)
+        {
+            cl = JDKHooks.getJDKHooks().getSystemClassLoader();
+        }
+        if (cl == null)
+        {
+            // this is not our day. bail out.
+            return Collections.EMPTY_LIST;
+        }
+
+        ClassLoaders loaders = new ClassLoaders();
+        loaders.put(cl);
+
+        List registries = new ArrayList();
+        Enumeration providers = Service.providers(
+                new SPInterface(AbstractFactoryRegistry.class), loaders);
+        while (providers.hasMoreElements())
+        {
+            Object provider = providers.nextElement();
+            if (provider instanceof AbstractFactoryRegistry)
+            {
+                registries.add(provider);
+            }
+        }
+
+        return registries;
+    }
+}
Index: pom.xml
===================================================================
--- pom.xml (revision 1677)
+++ pom.xml (working copy)
@@ -35,6 +35,11 @@
       <scope>compile</scope>
     </dependency>
     <dependency>
+      <groupId>commons-discovery</groupId>
+      <artifactId>commons-discovery</artifactId>
+      <version>0.4</version>
+    </dependency>
+    <dependency>
       <groupId>org.codehaus.cargo</groupId>
       <artifactId>cargo-core-api-container</artifactId>
       <version>${version}</version>

[kohsuke]

Any news on this? Should I assume that no news is a good news?

Kohsuke Kawaguchi wrote:

> Matt Wringe wrote:
>> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
>>> Does this direction make sense?
>>>
>>> I guess what I'm trying to avoid here is to spend time implementing this
>>> feature, only to learn later that you don't agree with the direction.
>>>
>> it makes sense to me :)
>
> Attached is the patch for this change that covers Deployable and
> DeployableFactory. If this looks OK, I'll proceed with all the other
> factory types.
>
> See the test code for how the container implementation would look like.
>
>
>>> Kohsuke Kawaguchi wrote:
>>> > Matt Wringe wrote:
>>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
>>> >>> Matt Wringe wrote:
>>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>>> >>> >> check with the team if people think this makes sense (hopefully they would.)
>>> >>> >
>>> >>> > Replied to the jira about how cargo already supports custom containers
>>> >>> > and configuration.
>>> >>>
>>> >>> Thanks. I added my comment there, too. In short, I think what you showed
>>> >>> me is a good feature, but IMHO it's not suffice.
>>> >>>
>>> >>>
>>> >>> >> The implementation approach that I'm thinking of is to have
>>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
>>> >>> >> META-INF/services and use them as delegates on methods like
>>> >>> >> "getDeployerClass". So the container implementations that would like to
>>> >>> >> hook into this would implement their own factories and write
>>> >>> >> META-INF/services entry.
>>> >>> >
>>> >>> > I do not have a lot of experience with jar service providers so I don't
>>> >>> > really know how feasible this is.
>>> >>>
>>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
>>> >> ok :) could you explain a bit more how exactly it would work then?
>>> >
>>> > This is a standard convention used widely everywhere, such as in many
>>> > JCP APIs.
>>> >
>>> > The technique is for the use cases where typically one wants to discover
>>> > implementations of some contract defined in the API. For example, in
>>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
>>> > implementations (like Xerces) without having any prior knowledge about
>>> > such an implementation.
>>> >
>>> > Here is the technique. The base contract is defined in terms of a Java
>>> > class/interface. Suppose this contract has the fully-qualified name of
>>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
>>> > that like to be discovered will put a file with the name
>>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>>> > in the jar file. This file is UTF-8 encoded, and it lists fully
>>> > qualified names of classes that implement this base interface/class.
>>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
>>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
>>> >
>>> > The discovery process takes one ClassLoader as a parameter. This
>>> > parameter is normally defaulted to
>>> > Thread.currentThread().getContextClassLoader(), but in more complex
>>> > scenarios the caller do need to be able to pass it in.
>>> >
>>> > The API then lists up all
>>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>>> > from resources, parse them all, and list up all the classes. It'll
>>> > instantiate them and use them, with suitable error handling in-between.
>>> >
>>> > commons-discovery defines code to do this. Writing it from scratch is
>>> > also rather easy.
>>> >
>>> > The end result is that all that needed to be done by the user is put
>>> > "cargo-glassfish-impl.jar" in the classpath.
>>> >
>>> >
>>> >>> > I really don't like the current factory implementation and would ideally
>>> >>> > like to see cargo automatically determine the proper classes to use
>>> >>> > based on a class naming scheme (and cargo is already using a naming
>>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
>>> >>> > implementation for Tomcat6 for a local installed container).
>>> >>> >
>>> >>> > With the current factory implementation its too easy to make
>>> >>> mistakes.
>>> >>>
>>> >>> I guess the only problem with the naming convention is that all the
>>> >>> implementations would have to live in the "org.codehaus" package.
>>> >>
>>> >> well, it could just look at the class name and not the package name.
>>> >
>>> > OK, so your idea of pluggability must be bit different than mine. I
>>> > suspect your approach is that somehow container implementations would
>>> > tell the factory about its class names, and the factory would figure out
>>> > the ID and the type from the class name. But this is missing how the
>>> > containers tell the factory their class names in the first place,
>>> > without asking the users' help --- that is the part that I'm trying to
>>> > eliminate.
>>> >
>>> > What I was thinking was that the factory implementation now only know
>>> > about a class name convention, and whenever someone requests an
>>> > instantiation from some parameters, it just blindly tries to create a
>>> > class of certain names. If the class is there, it works, otherwise, it
>>> > won't.
>>> >
>>> > So in the latter scheme, while you can try more than one class names,
>>> > you cannot really allow arbitrary package names or class names, simply
>>> > because the factory can't know about them all.
>>> >
>>> >
>>> >>> But if the Cargo project is OK for random people to be putting classes
>>> >>> in this package,
>>> >>
>>> >> I don't think it would be ok for people to be using
>>> >> org.codehaus.cargo.... packages as it would give the false impression on
>>> >> the code origins. Please see my comment above.
>>> >
>>> > OK. Then that eliminates convention-only approach.
>>> >
>>> >
>>> >>>  this is probably better than META-INF/services look up,
>>> >>> because the container implementor needs to write less.
>>> >>>
>>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
>>> >>> JBossXXX, because it would have inconsistent case convention. And the
>>> >>> naming convention would be somewhat contrived to make it fit with the
>>> >>> current class names.
>>> >>
>>> >> I am not entirely sure how it would work, I was just throwing out ideas.
>>> >
>>> > OK.
>>> >
>>> >
>>> >>> >> I feel container developers would have to write more than they should,
>>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
>>> >>> >> anything simpler means introducing another factory-like mechanism, which
>>> >>> >> I hesitate.
>>> >>> >>
>>> >>> >>
>>> >>> >> If it makes sense, I'll first prepare a patch for, say,
>>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
>>> >>> >> other factories.
>>> >>> >>
>>> >>> >>
>>> >>> >> I'd like to eventually resurrect the GlassFish support work,
>>> >>> >
>>> >>> > great, we need someone with glassfish knowledge
>>> >>> >
>>> >>> >>  but I think
>>> >>> >> this should be the first step, so that various people in the GlassFish
>>> >>> >> project can eat the dogfood before it can be considered for the
>>> >>> >> inclusion into Cargo core.
>>> >>> >
>>> >>> > I wouldn't worry about trying to get a perfectly working implementation
>>> >>> > before considering contributing to cargo. You can always start with some
>>> >>> > simple commands and work from there. If needed we can always add
>>> >>> > documentation about it being a feature preview.
>>> >>>
>>> >>> I think the feature still makes sense. Code that works is one reason,
>>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
>>> >>> might have changed now, but back when I contributed the embedded Tomcat
>>> >>> support, the checkstyle rules in Cargo project was very draconian.
>>> >>
>>> >> hehe, yes, I agree here.
>>> >>
>>> >>> It
>>> >>> was really painful for me to make sure every check in satisfies all the
>>> >>> check style rules.
>>> >>
>>> >> Its really not that bad when you use an ide to manage it for you. Right
>>> >> now in eclipse I can basically just highlight the code and tell eclipse
>>> >> to refactor it to fix the guidelines. And it also highlights the issues
>>> >> and tells you what is wrong with them (although I can't seem to figure
>>> >> out how to tell eclipse where the suppression file is).
>>> >
>>> > Yes, I was hoping that at least there are some IDEs that understand
>>> > checkstyle rule files. Unfortunately, I don't think such a feature is
>>> > available on my IDE, hence the preference to postpone till the end.
>>> >
>>> >
>>> >>> By working on a separate module, I'm hoping that I can postpone doing
>>> >>> that work until we integrate that back into the core (and perhaps at
>>> >>> that point I can ask for other people's help.)
>>> >>
>>> >> You can also easily disabled it by passing a specific -D option during a
>>> >> build.
>>> >
>>> > Yes. I'm aware of this, but again, the rule back then was that for me to
>>> > make a commit, it better passes the checkstyle.
>>> >
>>> >> If you are really worried about the checkstyle, we could even
>>> >> temporarily disable it for the Glassfish part.
>>> >
>>> > That's good to know :-)
>>> >
>>> > But I really think it's better for the Cargo project as a whole it it
>>> > lets people write containers outside the Cargo project.
>>> >
>>> > Think about Ant tasks, for example. If Ant didn't define a way for
>>> > people to write custom tasks outside the Apache Ant project itself, I
>>> > don't think it would have been this successful.
>>> >
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe from this list, please visit:
>>
>>     http://xircles.codehaus.org/manage_email
>>
>>
>>
>
>

--
Kohsuke Kawaguchi
Sun Microsystems                   http://weblogs.java.net/blog/kohsuke/

[Matt Wringe]

On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
> Any news on this? Should I assume that no news is a good news?

Sorry for no response from me, I looked at it briefly before and havn't
had a chance to look into into it deeper. From what I saw it looked ok.

> Kohsuke Kawaguchi wrote:
> > Matt Wringe wrote:
> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
> >>> Does this direction make sense?
> >>>
> >>> I guess what I'm trying to avoid here is to spend time implementing this
> >>> feature, only to learn later that you don't agree with the direction.
> >>>
> >> it makes sense to me :)
> >
> > Attached is the patch for this change that covers Deployable and
> > DeployableFactory. If this looks OK, I'll proceed with all the other
> > factory types.
> >
> > See the test code for how the container implementation would look like.
> >
> >
> >>> Kohsuke Kawaguchi wrote:
> >>> > Matt Wringe wrote:
> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> Matt Wringe wrote:
> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
> >>> >>> >
> >>> >>> > Replied to the jira about how cargo already supports custom containers
> >>> >>> > and configuration.
> >>> >>>
> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
> >>> >>> me is a good feature, but IMHO it's not suffice.
> >>> >>>
> >>> >>>
> >>> >>> >> The implementation approach that I'm thinking of is to have
> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
> >>> >>> >> META-INF/services and use them as delegates on methods like
> >>> >>> >> "getDeployerClass". So the container implementations that would like to
> >>> >>> >> hook into this would implement their own factories and write
> >>> >>> >> META-INF/services entry.
> >>> >>> >
> >>> >>> > I do not have a lot of experience with jar service providers so I don't
> >>> >>> > really know how feasible this is.
> >>> >>>
> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
> >>> >> ok :) could you explain a bit more how exactly it would work then?
> >>> >
> >>> > This is a standard convention used widely everywhere, such as in many
> >>> > JCP APIs.
> >>> >
> >>> > The technique is for the use cases where typically one wants to discover
> >>> > implementations of some contract defined in the API. For example, in
> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
> >>> > implementations (like Xerces) without having any prior knowledge about
> >>> > such an implementation.
> >>> >
> >>> > Here is the technique. The base contract is defined in terms of a Java
> >>> > class/interface. Suppose this contract has the fully-qualified name of
> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
> >>> > that like to be discovered will put a file with the name
> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
> >>> > qualified names of classes that implement this base interface/class.
> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
> >>> >
> >>> > The discovery process takes one ClassLoader as a parameter. This
> >>> > parameter is normally defaulted to
> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
> >>> > scenarios the caller do need to be able to pass it in.
> >>> >
> >>> > The API then lists up all
> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >>> > from resources, parse them all, and list up all the classes. It'll
> >>> > instantiate them and use them, with suitable error handling in-between.
> >>> >
> >>> > commons-discovery defines code to do this. Writing it from scratch is
> >>> > also rather easy.
> >>> >
> >>> > The end result is that all that needed to be done by the user is put
> >>> > "cargo-glassfish-impl.jar" in the classpath.
> >>> >
> >>> >
> >>> >>> > I really don't like the current factory implementation and would ideally
> >>> >>> > like to see cargo automatically determine the proper classes to use
> >>> >>> > based on a class naming scheme (and cargo is already using a naming
> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
> >>> >>> > implementation for Tomcat6 for a local installed container).
> >>> >>> >
> >>> >>> > With the current factory implementation its too easy to make
> >>> >>> mistakes.
> >>> >>>
> >>> >>> I guess the only problem with the naming convention is that all the
> >>> >>> implementations would have to live in the "org.codehaus" package.
> >>> >>
> >>> >> well, it could just look at the class name and not the package name.
> >>> >
> >>> > OK, so your idea of pluggability must be bit different than mine. I
> >>> > suspect your approach is that somehow container implementations would
> >>> > tell the factory about its class names, and the factory would figure out
> >>> > the ID and the type from the class name. But this is missing how the
> >>> > containers tell the factory their class names in the first place,
> >>> > without asking the users' help --- that is the part that I'm trying to
> >>> > eliminate.
> >>> >
> >>> > What I was thinking was that the factory implementation now only know
> >>> > about a class name convention, and whenever someone requests an
> >>> > instantiation from some parameters, it just blindly tries to create a
> >>> > class of certain names. If the class is there, it works, otherwise, it
> >>> > won't.
> >>> >
> >>> > So in the latter scheme, while you can try more than one class names,
> >>> > you cannot really allow arbitrary package names or class names, simply
> >>> > because the factory can't know about them all.
> >>> >
> >>> >
> >>> >>> But if the Cargo project is OK for random people to be putting classes
> >>> >>> in this package,
> >>> >>
> >>> >> I don't think it would be ok for people to be using
> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
> >>> >> the code origins. Please see my comment above.
> >>> >
> >>> > OK. Then that eliminates convention-only approach.
> >>> >
> >>> >
> >>> >>>  this is probably better than META-INF/services look up,
> >>> >>> because the container implementor needs to write less.
> >>> >>>
> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
> >>> >>> naming convention would be somewhat contrived to make it fit with the
> >>> >>> current class names.
> >>> >>
> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
> >>> >
> >>> > OK.
> >>> >
> >>> >
> >>> >>> >> I feel container developers would have to write more than they should,
> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
> >>> >>> >> I hesitate.
> >>> >>> >>
> >>> >>> >>
> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
> >>> >>> >> other factories.
> >>> >>> >>
> >>> >>> >>
> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
> >>> >>> >
> >>> >>> > great, we need someone with glassfish knowledge
> >>> >>> >
> >>> >>> >>  but I think
> >>> >>> >> this should be the first step, so that various people in the GlassFish
> >>> >>> >> project can eat the dogfood before it can be considered for the
> >>> >>> >> inclusion into Cargo core.
> >>> >>> >
> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
> >>> >>> > before considering contributing to cargo. You can always start with some
> >>> >>> > simple commands and work from there. If needed we can always add
> >>> >>> > documentation about it being a feature preview.
> >>> >>>
> >>> >>> I think the feature still makes sense. Code that works is one reason,
> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
> >>> >>
> >>> >> hehe, yes, I agree here.
> >>> >>
> >>> >>> It
> >>> >>> was really painful for me to make sure every check in satisfies all the
> >>> >>> check style rules.
> >>> >>
> >>> >> Its really not that bad when you use an ide to manage it for you. Right
> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
> >>> >> and tells you what is wrong with them (although I can't seem to figure
> >>> >> out how to tell eclipse where the suppression file is).
> >>> >
> >>> > Yes, I was hoping that at least there are some IDEs that understand
> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
> >>> > available on my IDE, hence the preference to postpone till the end.
> >>> >
> >>> >
> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
> >>> >>> that work until we integrate that back into the core (and perhaps at
> >>> >>> that point I can ask for other people's help.)
> >>> >>
> >>> >> You can also easily disabled it by passing a specific -D option during a
> >>> >> build.
> >>> >
> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
> >>> > make a commit, it better passes the checkstyle.
> >>> >
> >>> >> If you are really worried about the checkstyle, we could even
> >>> >> temporarily disable it for the Glassfish part.
> >>> >
> >>> > That's good to know :-)
> >>> >
> >>> > But I really think it's better for the Cargo project as a whole it it
> >>> > lets people write containers outside the Cargo project.
> >>> >
> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
> >>> > people to write custom tasks outside the Apache Ant project itself, I
> >>> > don't think it would have been this successful.
> >>> >
> >>>
> >>>
> >>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe from this list, please visit:
> >>
> >>     http://xircles.codehaus.org/manage_email
> >>
> >>
> >>
> >
> >
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[kohsuke]

Matt Wringe wrote:
> On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
>> Any news on this? Should I assume that no news is a good news?
>
> Sorry for no response from me, I looked at it briefly before and havn't
> had a chance to look into into it deeper. From what I saw it looked ok.

Thanks. I'll proceed and do the same for all the types.

>
>
>> Kohsuke Kawaguchi wrote:
>> > Matt Wringe wrote:
>> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
>> >>> Does this direction make sense?
>> >>>
>> >>> I guess what I'm trying to avoid here is to spend time implementing this
>> >>> feature, only to learn later that you don't agree with the direction.
>> >>>
>> >> it makes sense to me :)
>> >
>> > Attached is the patch for this change that covers Deployable and
>> > DeployableFactory. If this looks OK, I'll proceed with all the other
>> > factory types.
>> >
>> > See the test code for how the container implementation would look like.
>> >
>> >
>> >>> Kohsuke Kawaguchi wrote:
>> >>> > Matt Wringe wrote:
>> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
>> >>> >>> Matt Wringe wrote:
>> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
>> >>> >>> >
>> >>> >>> > Replied to the jira about how cargo already supports custom containers
>> >>> >>> > and configuration.
>> >>> >>>
>> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
>> >>> >>> me is a good feature, but IMHO it's not suffice.
>> >>> >>>
>> >>> >>>
>> >>> >>> >> The implementation approach that I'm thinking of is to have
>> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
>> >>> >>> >> META-INF/services and use them as delegates on methods like
>> >>> >>> >> "getDeployerClass". So the container implementations that would like to
>> >>> >>> >> hook into this would implement their own factories and write
>> >>> >>> >> META-INF/services entry.
>> >>> >>> >
>> >>> >>> > I do not have a lot of experience with jar service providers so I don't
>> >>> >>> > really know how feasible this is.
>> >>> >>>
>> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
>> >>> >> ok :) could you explain a bit more how exactly it would work then?
>> >>> >
>> >>> > This is a standard convention used widely everywhere, such as in many
>> >>> > JCP APIs.
>> >>> >
>> >>> > The technique is for the use cases where typically one wants to discover
>> >>> > implementations of some contract defined in the API. For example, in
>> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
>> >>> > implementations (like Xerces) without having any prior knowledge about
>> >>> > such an implementation.
>> >>> >
>> >>> > Here is the technique. The base contract is defined in terms of a Java
>> >>> > class/interface. Suppose this contract has the fully-qualified name of
>> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
>> >>> > that like to be discovered will put a file with the name
>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
>> >>> > qualified names of classes that implement this base interface/class.
>> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
>> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
>> >>> >
>> >>> > The discovery process takes one ClassLoader as a parameter. This
>> >>> > parameter is normally defaulted to
>> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
>> >>> > scenarios the caller do need to be able to pass it in.
>> >>> >
>> >>> > The API then lists up all
>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>> >>> > from resources, parse them all, and list up all the classes. It'll
>> >>> > instantiate them and use them, with suitable error handling in-between.
>> >>> >
>> >>> > commons-discovery defines code to do this. Writing it from scratch is
>> >>> > also rather easy.
>> >>> >
>> >>> > The end result is that all that needed to be done by the user is put
>> >>> > "cargo-glassfish-impl.jar" in the classpath.
>> >>> >
>> >>> >
>> >>> >>> > I really don't like the current factory implementation and would ideally
>> >>> >>> > like to see cargo automatically determine the proper classes to use
>> >>> >>> > based on a class naming scheme (and cargo is already using a naming
>> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
>> >>> >>> > implementation for Tomcat6 for a local installed container).
>> >>> >>> >
>> >>> >>> > With the current factory implementation its too easy to make
>> >>> >>> mistakes.
>> >>> >>>
>> >>> >>> I guess the only problem with the naming convention is that all the
>> >>> >>> implementations would have to live in the "org.codehaus" package.
>> >>> >>
>> >>> >> well, it could just look at the class name and not the package name.
>> >>> >
>> >>> > OK, so your idea of pluggability must be bit different than mine. I
>> >>> > suspect your approach is that somehow container implementations would
>> >>> > tell the factory about its class names, and the factory would figure out
>> >>> > the ID and the type from the class name. But this is missing how the
>> >>> > containers tell the factory their class names in the first place,
>> >>> > without asking the users' help --- that is the part that I'm trying to
>> >>> > eliminate.
>> >>> >
>> >>> > What I was thinking was that the factory implementation now only know
>> >>> > about a class name convention, and whenever someone requests an
>> >>> > instantiation from some parameters, it just blindly tries to create a
>> >>> > class of certain names. If the class is there, it works, otherwise, it
>> >>> > won't.
>> >>> >
>> >>> > So in the latter scheme, while you can try more than one class names,
>> >>> > you cannot really allow arbitrary package names or class names, simply
>> >>> > because the factory can't know about them all.
>> >>> >
>> >>> >
>> >>> >>> But if the Cargo project is OK for random people to be putting classes
>> >>> >>> in this package,
>> >>> >>
>> >>> >> I don't think it would be ok for people to be using
>> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
>> >>> >> the code origins. Please see my comment above.
>> >>> >
>> >>> > OK. Then that eliminates convention-only approach.
>> >>> >
>> >>> >
>> >>> >>>  this is probably better than META-INF/services look up,
>> >>> >>> because the container implementor needs to write less.
>> >>> >>>
>> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
>> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
>> >>> >>> naming convention would be somewhat contrived to make it fit with the
>> >>> >>> current class names.
>> >>> >>
>> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
>> >>> >
>> >>> > OK.
>> >>> >
>> >>> >
>> >>> >>> >> I feel container developers would have to write more than they should,
>> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
>> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
>> >>> >>> >> I hesitate.
>> >>> >>> >>
>> >>> >>> >>
>> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
>> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
>> >>> >>> >> other factories.
>> >>> >>> >>
>> >>> >>> >>
>> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
>> >>> >>> >
>> >>> >>> > great, we need someone with glassfish knowledge
>> >>> >>> >
>> >>> >>> >>  but I think
>> >>> >>> >> this should be the first step, so that various people in the GlassFish
>> >>> >>> >> project can eat the dogfood before it can be considered for the
>> >>> >>> >> inclusion into Cargo core.
>> >>> >>> >
>> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
>> >>> >>> > before considering contributing to cargo. You can always start with some
>> >>> >>> > simple commands and work from there. If needed we can always add
>> >>> >>> > documentation about it being a feature preview.
>> >>> >>>
>> >>> >>> I think the feature still makes sense. Code that works is one reason,
>> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
>> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
>> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
>> >>> >>
>> >>> >> hehe, yes, I agree here.
>> >>> >>
>> >>> >>> It
>> >>> >>> was really painful for me to make sure every check in satisfies all the
>> >>> >>> check style rules.
>> >>> >>
>> >>> >> Its really not that bad when you use an ide to manage it for you. Right
>> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
>> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
>> >>> >> and tells you what is wrong with them (although I can't seem to figure
>> >>> >> out how to tell eclipse where the suppression file is).
>> >>> >
>> >>> > Yes, I was hoping that at least there are some IDEs that understand
>> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
>> >>> > available on my IDE, hence the preference to postpone till the end.
>> >>> >
>> >>> >
>> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
>> >>> >>> that work until we integrate that back into the core (and perhaps at
>> >>> >>> that point I can ask for other people's help.)
>> >>> >>
>> >>> >> You can also easily disabled it by passing a specific -D option during a
>> >>> >> build.
>> >>> >
>> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
>> >>> > make a commit, it better passes the checkstyle.
>> >>> >
>> >>> >> If you are really worried about the checkstyle, we could even
>> >>> >> temporarily disable it for the Glassfish part.
>> >>> >
>> >>> > That's good to know :-)
>> >>> >
>> >>> > But I really think it's better for the Cargo project as a whole it it
>> >>> > lets people write containers outside the Cargo project.
>> >>> >
>> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
>> >>> > people to write custom tasks outside the Apache Ant project itself, I
>> >>> > don't think it would have been this successful.
>> >>> >
>> >>>
>> >>>
>> >>
>> >>
>> >> ---------------------------------------------------------------------
>> >> To unsubscribe from this list, please visit:
>> >>
>> >>     http://xircles.codehaus.org/manage_email
>> >>
>> >>
>> >>
>> >
>> >
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>
>

--
Kohsuke Kawaguchi
Sun Microsystems                   http://weblogs.java.net/blog/kohsuke/

[kohsuke]

Kohsuke Kawaguchi wrote:
> Matt Wringe wrote:
>> On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
>>> Any news on this? Should I assume that no news is a good news?
>>
>> Sorry for no response from me, I looked at it briefly before and havn't
>> had a chance to look into into it deeper. From what I saw it looked ok.
>
> Thanks. I'll proceed and do the same for all the types.

... and here is the patch that does the same thing for all factory types.

>>> Kohsuke Kawaguchi wrote:
>>> > Matt Wringe wrote:
>>> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
>>> >>> Does this direction make sense?
>>> >>>
>>> >>> I guess what I'm trying to avoid here is to spend time implementing this
>>> >>> feature, only to learn later that you don't agree with the direction.
>>> >>>
>>> >> it makes sense to me :)
>>> >
>>> > Attached is the patch for this change that covers Deployable and
>>> > DeployableFactory. If this looks OK, I'll proceed with all the other
>>> > factory types.
>>> >
>>> > See the test code for how the container implementation would look like.
>>> >
>>> >
>>> >>> Kohsuke Kawaguchi wrote:
>>> >>> > Matt Wringe wrote:
>>> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
>>> >>> >>> Matt Wringe wrote:
>>> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>>> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>>> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
>>> >>> >>> >
>>> >>> >>> > Replied to the jira about how cargo already supports custom containers
>>> >>> >>> > and configuration.
>>> >>> >>>
>>> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
>>> >>> >>> me is a good feature, but IMHO it's not suffice.
>>> >>> >>>
>>> >>> >>>
>>> >>> >>> >> The implementation approach that I'm thinking of is to have
>>> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
>>> >>> >>> >> META-INF/services and use them as delegates on methods like
>>> >>> >>> >> "getDeployerClass". So the container implementations that would like to
>>> >>> >>> >> hook into this would implement their own factories and write
>>> >>> >>> >> META-INF/services entry.
>>> >>> >>> >
>>> >>> >>> > I do not have a lot of experience with jar service providers so I don't
>>> >>> >>> > really know how feasible this is.
>>> >>> >>>
>>> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
>>> >>> >> ok :) could you explain a bit more how exactly it would work then?
>>> >>> >
>>> >>> > This is a standard convention used widely everywhere, such as in many
>>> >>> > JCP APIs.
>>> >>> >
>>> >>> > The technique is for the use cases where typically one wants to discover
>>> >>> > implementations of some contract defined in the API. For example, in
>>> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
>>> >>> > implementations (like Xerces) without having any prior knowledge about
>>> >>> > such an implementation.
>>> >>> >
>>> >>> > Here is the technique. The base contract is defined in terms of a Java
>>> >>> > class/interface. Suppose this contract has the fully-qualified name of
>>> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
>>> >>> > that like to be discovered will put a file with the name
>>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>>> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
>>> >>> > qualified names of classes that implement this base interface/class.
>>> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
>>> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
>>> >>> >
>>> >>> > The discovery process takes one ClassLoader as a parameter. This
>>> >>> > parameter is normally defaulted to
>>> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
>>> >>> > scenarios the caller do need to be able to pass it in.
>>> >>> >
>>> >>> > The API then lists up all
>>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>>> >>> > from resources, parse them all, and list up all the classes. It'll
>>> >>> > instantiate them and use them, with suitable error handling in-between.
>>> >>> >
>>> >>> > commons-discovery defines code to do this. Writing it from scratch is
>>> >>> > also rather easy.
>>> >>> >
>>> >>> > The end result is that all that needed to be done by the user is put
>>> >>> > "cargo-glassfish-impl.jar" in the classpath.
>>> >>> >
>>> >>> >
>>> >>> >>> > I really don't like the current factory implementation and would ideally
>>> >>> >>> > like to see cargo automatically determine the proper classes to use
>>> >>> >>> > based on a class naming scheme (and cargo is already using a naming
>>> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
>>> >>> >>> > implementation for Tomcat6 for a local installed container).
>>> >>> >>> >
>>> >>> >>> > With the current factory implementation its too easy to make
>>> >>> >>> mistakes.
>>> >>> >>>
>>> >>> >>> I guess the only problem with the naming convention is that all the
>>> >>> >>> implementations would have to live in the "org.codehaus" package.
>>> >>> >>
>>> >>> >> well, it could just look at the class name and not the package name.
>>> >>> >
>>> >>> > OK, so your idea of pluggability must be bit different than mine. I
>>> >>> > suspect your approach is that somehow container implementations would
>>> >>> > tell the factory about its class names, and the factory would figure out
>>> >>> > the ID and the type from the class name. But this is missing how the
>>> >>> > containers tell the factory their class names in the first place,
>>> >>> > without asking the users' help --- that is the part that I'm trying to
>>> >>> > eliminate.
>>> >>> >
>>> >>> > What I was thinking was that the factory implementation now only know
>>> >>> > about a class name convention, and whenever someone requests an
>>> >>> > instantiation from some parameters, it just blindly tries to create a
>>> >>> > class of certain names. If the class is there, it works, otherwise, it
>>> >>> > won't.
>>> >>> >
>>> >>> > So in the latter scheme, while you can try more than one class names,
>>> >>> > you cannot really allow arbitrary package names or class names, simply
>>> >>> > because the factory can't know about them all.
>>> >>> >
>>> >>> >
>>> >>> >>> But if the Cargo project is OK for random people to be putting classes
>>> >>> >>> in this package,
>>> >>> >>
>>> >>> >> I don't think it would be ok for people to be using
>>> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
>>> >>> >> the code origins. Please see my comment above.
>>> >>> >
>>> >>> > OK. Then that eliminates convention-only approach.
>>> >>> >
>>> >>> >
>>> >>> >>>  this is probably better than META-INF/services look up,
>>> >>> >>> because the container implementor needs to write less.
>>> >>> >>>
>>> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
>>> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
>>> >>> >>> naming convention would be somewhat contrived to make it fit with the
>>> >>> >>> current class names.
>>> >>> >>
>>> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
>>> >>> >
>>> >>> > OK.
>>> >>> >
>>> >>> >
>>> >>> >>> >> I feel container developers would have to write more than they should,
>>> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
>>> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
>>> >>> >>> >> I hesitate.
>>> >>> >>> >>
>>> >>> >>> >>
>>> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
>>> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
>>> >>> >>> >> other factories.
>>> >>> >>> >>
>>> >>> >>> >>
>>> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
>>> >>> >>> >
>>> >>> >>> > great, we need someone with glassfish knowledge
>>> >>> >>> >
>>> >>> >>> >>  but I think
>>> >>> >>> >> this should be the first step, so that various people in the GlassFish
>>> >>> >>> >> project can eat the dogfood before it can be considered for the
>>> >>> >>> >> inclusion into Cargo core.
>>> >>> >>> >
>>> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
>>> >>> >>> > before considering contributing to cargo. You can always start with some
>>> >>> >>> > simple commands and work from there. If needed we can always add
>>> >>> >>> > documentation about it being a feature preview.
>>> >>> >>>
>>> >>> >>> I think the feature still makes sense. Code that works is one reason,
>>> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
>>> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
>>> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
>>> >>> >>
>>> >>> >> hehe, yes, I agree here.
>>> >>> >>
>>> >>> >>> It
>>> >>> >>> was really painful for me to make sure every check in satisfies all the
>>> >>> >>> check style rules.
>>> >>> >>
>>> >>> >> Its really not that bad when you use an ide to manage it for you. Right
>>> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
>>> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
>>> >>> >> and tells you what is wrong with them (although I can't seem to figure
>>> >>> >> out how to tell eclipse where the suppression file is).
>>> >>> >
>>> >>> > Yes, I was hoping that at least there are some IDEs that understand
>>> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
>>> >>> > available on my IDE, hence the preference to postpone till the end.
>>> >>> >
>>> >>> >
>>> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
>>> >>> >>> that work until we integrate that back into the core (and perhaps at
>>> >>> >>> that point I can ask for other people's help.)
>>> >>> >>
>>> >>> >> You can also easily disabled it by passing a specific -D option during a
>>> >>> >> build.
>>> >>> >
>>> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
>>> >>> > make a commit, it better passes the checkstyle.
>>> >>> >
>>> >>> >> If you are really worried about the checkstyle, we could even
>>> >>> >> temporarily disable it for the Glassfish part.
>>> >>> >
>>> >>> > That's good to know :-)
>>> >>> >
>>> >>> > But I really think it's better for the Cargo project as a whole it it
>>> >>> > lets people write containers outside the Cargo project.
>>> >>> >
>>> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
>>> >>> > people to write custom tasks outside the Apache Ant project itself, I
>>> >>> > don't think it would have been this successful.
>>> >>> >
>>> >>>
>>> >>>
>>> >>
>>> >>
>>> >> ---------------------------------------------------------------------
>>> >> To unsubscribe from this list, please visit:
>>> >>
>>> >>     http://xircles.codehaus.org/manage_email
>>> >>
>>> >>
>>> >>
>>> >
>>> >
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe from this list, please visit:
>>
>>     http://xircles.codehaus.org/manage_email
>>
>>
>>
>
>

--
Kohsuke Kawaguchi
Sun Microsystems                   http://weblogs.java.net/blog/kohsuke/

Index: src/test/java/org/codehaus/cargo/generic/FactoryRegistryTest.java
===================================================================
--- src/test/java/org/codehaus/cargo/generic/FactoryRegistryTest.java (revision 0)
+++ src/test/java/org/codehaus/cargo/generic/FactoryRegistryTest.java (revision 0)
@@ -0,0 +1,52 @@
+/*
+ * ========================================================================
+ *
+ * Copyright 2006-2008 Vincent Massol.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * ========================================================================
+ */
+package org.codehaus.cargo.generic;
+
+import junit.framework.TestCase;
+import org.codehaus.cargo.container.ContainerType;
+import org.codehaus.cargo.container.configuration.ConfigurationCapability;
+import org.codehaus.cargo.container.configuration.ConfigurationType;
+import org.codehaus.cargo.container.deployable.Deployable;
+import org.codehaus.cargo.container.deployable.DeployableType;
+import org.codehaus.cargo.generic.configuration.ConfigurationCapabilityFactory;
+import org.codehaus.cargo.generic.configuration.DefaultConfigurationCapabilityFactory;
+import org.codehaus.cargo.generic.deployable.DefaultDeployableFactory;
+
+/**
+ * Tests the discovery behavior.
+ *
+ * The class name can't be {@code AbstractFactoryRegistryTest} or else the test will be skipped.
+ *
+ * @version $Id: $
+ */
+public class FactoryRegistryTest extends TestCase
+{
+    public void test1() {
+        DefaultDeployableFactory f = new DefaultDeployableFactory(getClass().getClassLoader());
+        Deployable war = f.createDeployable("super-container", ".", DeployableType.WAR);
+        assertTrue(war instanceof SuperContainerWar);
+    }
+
+    public void test2() {
+        ConfigurationCapabilityFactory f = new DefaultConfigurationCapabilityFactory(getClass().getClassLoader());
+        ConfigurationCapability cc = f.createConfigurationCapability("super-container", ContainerType.INSTALLED, ConfigurationType.STANDALONE);
+        assertTrue(cc instanceof SuperConfigurationCapability);
+    }
+}
Index: src/test/java/org/codehaus/cargo/generic/TestFactoryRegistry.java
===================================================================
--- src/test/java/org/codehaus/cargo/generic/TestFactoryRegistry.java (revision 0)
+++ src/test/java/org/codehaus/cargo/generic/TestFactoryRegistry.java (revision 0)
@@ -0,0 +1,63 @@
+/*
+ * ========================================================================
+ *
+ * Copyright 2006-2008 Vincent Massol.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * ========================================================================
+ */
+package org.codehaus.cargo.generic;
+
+import org.codehaus.cargo.generic.deployable.DeployableFactory;
+import org.codehaus.cargo.generic.configuration.ConfigurationCapabilityFactory;
+import org.codehaus.cargo.generic.configuration.ConfigurationFactory;
+import org.codehaus.cargo.generic.deployer.DeployerFactory;
+import org.codehaus.cargo.generic.packager.PackagerFactory;
+import org.codehaus.cargo.container.deployable.DeployableType;
+import org.codehaus.cargo.container.ContainerType;
+import org.codehaus.cargo.container.configuration.ConfigurationType;
+
+/**
+ * To be discovered by {@link AbstractFactoryRegistry}.
+ *
+ * @version $Id: $
+ */
+public class TestFactoryRegistry extends AbstractFactoryRegistry
+{
+    protected void register(DeployableFactory factory)
+    {
+        factory.registerDeployable("super-container", DeployableType.WAR, SuperContainerWar.class);
+    }
+
+    protected void register(ConfigurationCapabilityFactory factory)
+    {
+        factory.registerConfigurationCapability("super-container", ContainerType.INSTALLED,
+            ConfigurationType.STANDALONE, SuperConfigurationCapability.class);
+    }
+
+    protected void register(ConfigurationFactory factory) {
+    }
+
+    protected void register(DeployerFactory factory) {
+    }
+
+    protected void register(PackagerFactory factory) {
+    }
+
+    protected void register(ContainerFactory factory) {
+    }
+
+    protected void register(ContainerCapabilityFactory factory) {
+    }
+}
Index: src/test/java/org/codehaus/cargo/generic/SuperConfigurationCapability.java
===================================================================
--- src/test/java/org/codehaus/cargo/generic/SuperConfigurationCapability.java (revision 0)
+++ src/test/java/org/codehaus/cargo/generic/SuperConfigurationCapability.java (revision 0)
@@ -0,0 +1,43 @@
+/*
+ * ========================================================================
+ *
+ * Copyright 2006-2008 Vincent Massol.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * ========================================================================
+ */
+package org.codehaus.cargo.generic;
+
+import org.codehaus.cargo.container.configuration.ConfigurationCapability;
+
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Used by {@link TestFactoryRegistry} to see if the discovery succeeds.
+ *
+ * @version $Id: $
+ */
+public class SuperConfigurationCapability implements ConfigurationCapability
+{
+    public boolean supportsProperty(String propertyName)
+    {
+        return true;
+    }
+
+    public Map getProperties()
+    {
+        return new HashMap();
+    }
+}
Index: src/test/java/org/codehaus/cargo/generic/SuperContainerWar.java
===================================================================
--- src/test/java/org/codehaus/cargo/generic/SuperContainerWar.java (revision 0)
+++ src/test/java/org/codehaus/cargo/generic/SuperContainerWar.java (revision 0)
@@ -0,0 +1,35 @@
+/*
+ * ========================================================================
+ *
+ * Copyright 2006-2008 Vincent Massol.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * ========================================================================
+ */
+package org.codehaus.cargo.generic;
+
+import org.codehaus.cargo.container.deployable.WAR;
+
+/**
+ * Used by {@link TestFactoryRegistry} to see if the discovery succeeds.
+ *
+ * @version $Id: $
+ */
+public class SuperContainerWar extends WAR
+{
+    public SuperContainerWar(String war)
+    {
+        super(war);
+    }
+}
Index: src/test/resources/META-INF/services/org.codehaus.cargo.generic.AbstractFactoryRegistry
===================================================================
--- src/test/resources/META-INF/services/org.codehaus.cargo.generic.AbstractFactoryRegistry (revision 0)
+++ src/test/resources/META-INF/services/org.codehaus.cargo.generic.AbstractFactoryRegistry (revision 0)
@@ -0,0 +1,2 @@
+# test for ABstractFactoryRegistry
+org.codehaus.cargo.generic.TestFactoryRegistry
\ No newline at end of file
Index: src/main/java/org/codehaus/cargo/generic/DefaultContainerFactory.java
===================================================================
--- src/main/java/org/codehaus/cargo/generic/DefaultContainerFactory.java (revision 1677)
+++ src/main/java/org/codehaus/cargo/generic/DefaultContainerFactory.java (working copy)
@@ -60,6 +60,19 @@
      */
     public DefaultContainerFactory()
     {
+        this(null);
+    }
+
+    /**
+     * Register packager name mappings.
+     *
+     * @param classLoader
+     *      ClassLoader to discover implementations from. See
+     *      {@link AbstractFactoryRegistry#register(ClassLoader, ContainerFactory)}
+     *      for the details of what this value means.
+     */
+    public DefaultContainerFactory(ClassLoader classLoader)
+    {
         super();
 
         // Note: We register containers using introspection so that we don't have to depend on
@@ -140,6 +153,8 @@
             "org.codehaus.cargo.container.weblogic.WebLogic8xInstalledLocalContainer");
         registerContainer("weblogic9x", ContainerType.INSTALLED,
             "org.codehaus.cargo.container.weblogic.WebLogic9xInstalledLocalContainer");
+
+        AbstractFactoryRegistry.register(classLoader, this);
     }
 
     /**
Index: src/main/java/org/codehaus/cargo/generic/deployable/DefaultDeployableFactory.java
===================================================================
--- src/main/java/org/codehaus/cargo/generic/deployable/DefaultDeployableFactory.java (revision 1677)
+++ src/main/java/org/codehaus/cargo/generic/deployable/DefaultDeployableFactory.java (working copy)
@@ -30,6 +30,7 @@
 import org.codehaus.cargo.generic.spi.AbstractIntrospectionGenericHintFactory;
 import org.codehaus.cargo.generic.internal.util.RegistrationKey;
 import org.codehaus.cargo.generic.internal.util.SimpleContainerIdentity;
+import org.codehaus.cargo.generic.AbstractFactoryRegistry;
 
 import java.lang.reflect.Constructor;
 
@@ -66,6 +67,19 @@
      */
     public DefaultDeployableFactory()
     {
+        this(null);
+    }
+
+    /**
+     * Register deployable classes mappings.
+     *
+     * @param classLoader
+     *      ClassLoader to discover implementations from. See
+     *      {@link AbstractFactoryRegistry#register(ClassLoader, DeployableFactory)}
+     *      for the details of what this value means.
+     */
+    public DefaultDeployableFactory(ClassLoader classLoader)
+    {
         // The WAR, EJB and EAR deployables are registered by default against all containers.
         // In order not to have to individually register against each container id we
         // create a fictitious default container id.
@@ -91,6 +105,8 @@
             "org.codehaus.cargo.container.geronimo.deployable.GeronimoEAR");
 
         // TODO: Register JBossWAR here when we add JBoss support
+
+        AbstractFactoryRegistry.register(classLoader, this);
     }
 
     /**
Index: src/main/java/org/codehaus/cargo/generic/DefaultContainerCapabilityFactory.java
===================================================================
--- src/main/java/org/codehaus/cargo/generic/DefaultContainerCapabilityFactory.java (revision 1677)
+++ src/main/java/org/codehaus/cargo/generic/DefaultContainerCapabilityFactory.java (working copy)
@@ -22,6 +22,7 @@
 import org.codehaus.cargo.generic.spi.AbstractIntrospectionGenericHintFactory;
 import org.codehaus.cargo.generic.internal.util.RegistrationKey;
 import org.codehaus.cargo.generic.internal.util.SimpleContainerIdentity;
+import org.codehaus.cargo.generic.packager.PackagerFactory;
 import org.codehaus.cargo.container.ContainerCapability;
 
 import java.lang.reflect.Constructor;
@@ -40,6 +41,19 @@
      */
     public DefaultContainerCapabilityFactory()
     {
+        this(null);
+    }
+
+    /**
+     * Register container capability name mappings.
+     *
+     * @param classLoader
+     *      ClassLoader to discover implementations from. See
+     *      {@link AbstractFactoryRegistry#register(ClassLoader, ContainerCapabilityFactory)}
+     *      for the details of what this value means.
+     */
+    public DefaultContainerCapabilityFactory(ClassLoader classLoader)
+    {
         super();
 
         // Note: We register container capabilities using introspection so that we don't have to
@@ -100,6 +114,8 @@
             "org.codehaus.cargo.container.internal.J2EEContainerCapability");
         registerContainerCapability("weblogic9x",
             "org.codehaus.cargo.container.internal.J2EEContainerCapability");
+
+        AbstractFactoryRegistry.register(classLoader, this);
     }
 
     /**
Index: src/main/java/org/codehaus/cargo/generic/deployer/DefaultDeployerFactory.java
===================================================================
--- src/main/java/org/codehaus/cargo/generic/deployer/DefaultDeployerFactory.java (revision 1677)
+++ src/main/java/org/codehaus/cargo/generic/deployer/DefaultDeployerFactory.java (working copy)
@@ -29,6 +29,7 @@
 import org.codehaus.cargo.generic.spi.AbstractIntrospectionGenericHintFactory;
 import org.codehaus.cargo.generic.internal.util.RegistrationKey;
 import org.codehaus.cargo.generic.internal.util.SimpleContainerIdentity;
+import org.codehaus.cargo.generic.AbstractFactoryRegistry;
 
 import java.lang.reflect.Constructor;
 
@@ -59,6 +60,19 @@
      */
     public DefaultDeployerFactory()
     {
+        this(null);
+    }
+
+    /**
+     * Register deployer name mappings.
+     *
+     * @param classLoader
+     *      ClassLoader to discover implementations from. See
+     *      {@link AbstractFactoryRegistry#register(ClassLoader, DeployerFactory)}
+     *      for the details of what this value means.
+     */
+    public DefaultDeployerFactory(ClassLoader classLoader)
+    {
         super();
 
         // Note: Sorted by container id alphabetical order
@@ -116,6 +130,7 @@
         registerDeployer("tomcat6x", DeployerType.REMOTE,
             "org.codehaus.cargo.container.tomcat.Tomcat6xRemoteDeployer");
 
+        AbstractFactoryRegistry.register(classLoader, this);
     }
 
     /**
Index: src/main/java/org/codehaus/cargo/generic/AbstractFactoryRegistry.java
===================================================================
--- src/main/java/org/codehaus/cargo/generic/AbstractFactoryRegistry.java (revision 0)
+++ src/main/java/org/codehaus/cargo/generic/AbstractFactoryRegistry.java (revision 0)
@@ -0,0 +1,289 @@
+/*
+ * ========================================================================
+ *
+ * Copyright 2006-2008 Vincent Massol.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * ========================================================================
+ */
+package org.codehaus.cargo.generic;
+
+import org.apache.commons.discovery.jdk.JDKHooks;
+import org.apache.commons.discovery.resource.ClassLoaders;
+import org.apache.commons.discovery.tools.SPInterface;
+import org.apache.commons.discovery.tools.Service;
+import org.codehaus.cargo.generic.deployable.DeployableFactory;
+import org.codehaus.cargo.generic.configuration.ConfigurationFactory;
+import org.codehaus.cargo.generic.configuration.ConfigurationCapabilityFactory;
+import org.codehaus.cargo.generic.deployer.DeployerFactory;
+import org.codehaus.cargo.generic.packager.PackagerFactory;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.Iterator;
+import java.util.List;
+
+/**
+ * SPI to be implemented by container implementation to register implementations
+ * to their factories.
+ *
+ * <p>
+ * This class also provides static methods (to be used primarily within Cargo
+ * but can be also called directly by client apps) to discover all the implementations
+ * and register them to factories.
+ *
+ * <p>
+ * Client apps should normally use {@code DefaultXXXFactory} classes,
+ * like {@link org.codehaus.cargo.generic.deployable.DefaultDeployableFactory},
+ * which internally uses the discovery mechanism
+ *
+ * <p>
+ * Container implementors should override the 1-arg {@code register} methods to register
+ * its implementations to the given factory.
+ *
+ * @version $Id: $
+ */
+public abstract class AbstractFactoryRegistry
+{
+    /**
+     * Discovers all the {@link org.codehaus.cargo.container.deployable.Deployable}s and
+     * adds them to the given {@link DeployableFactory}.
+     *
+     * <p>
+     * The discovery is done by
+     * <a href="http://java.sun.com/j2se/1.3/docs/guide/jar/jar.html">the standard service loader
+     * mechanism</a>, by looking for
+     * <tt>/META-INF/services/org.codehaus.cargo.generic.AbstractFactoryRegistry</tt> files.
+     *
+     * @param classLoader
+     *      The class loader to be used to search service provide configuration files.
+     *      If null, the value defaults to the thread context classloader. If that's also null,
+     *      the value defaults to the class loader that loaded {@link AbstractFactoryRegistry}.
+     *      In the rare circumstance of that also being null (which means Cargo is loaded in the
+     *      bootstrap classloader), the value defaults to the system class loader.
+     * @param factory
+     *      The factory whose {@code register} method is invoked to receive
+     *      {@link org.codehaus.cargo.container.deployable.Deployable}s that
+     *      are discovered.
+     */
+    public static void register(ClassLoader classLoader, DeployableFactory factory)
+    {
+        for (Iterator itr = list(classLoader).iterator(); itr.hasNext();)
+        {
+            ((AbstractFactoryRegistry) itr.next()).register(factory);
+        }
+    }
+
+    /**
+     * See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     *
+     * @param classLoader
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     * @param factory
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     */
+    public static void register(ClassLoader classLoader, ConfigurationFactory factory)
+    {
+        for (Iterator itr = list(classLoader).iterator(); itr.hasNext();)
+        {
+            ((AbstractFactoryRegistry) itr.next()).register(factory);
+        }
+    }
+
+    /**
+     * See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     *
+     * @param classLoader
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     * @param factory
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     */
+    public static void register(ClassLoader classLoader, ConfigurationCapabilityFactory factory)
+    {
+        for (Iterator itr = list(classLoader).iterator(); itr.hasNext();)
+        {
+            ((AbstractFactoryRegistry) itr.next()).register(factory);
+        }
+    }
+
+    /**
+     * See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     *
+     * @param classLoader
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     * @param factory
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     */
+    public static void register(ClassLoader classLoader, DeployerFactory factory)
+    {
+        for (Iterator itr = list(classLoader).iterator(); itr.hasNext();)
+        {
+            ((AbstractFactoryRegistry) itr.next()).register(factory);
+        }
+    }
+
+    /**
+     * See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     *
+     * @param classLoader
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     * @param factory
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     */
+    public static void register(ClassLoader classLoader, PackagerFactory factory)
+    {
+        for (Iterator itr = list(classLoader).iterator(); itr.hasNext();)
+        {
+            ((AbstractFactoryRegistry) itr.next()).register(factory);
+        }
+    }
+
+    /**
+     * See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     *
+     * @param classLoader
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     * @param factory
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     */
+    public static void register(ClassLoader classLoader, ContainerFactory factory)
+    {
+        for (Iterator itr = list(classLoader).iterator(); itr.hasNext();)
+        {
+            ((AbstractFactoryRegistry) itr.next()).register(factory);
+        }
+    }
+
+    /**
+     * See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     *
+     * @param classLoader
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     * @param factory
+     *      See {@link #register(ClassLoader, DeployableFactory)} for the semantics.
+     */
+    public static void register(ClassLoader classLoader, ContainerCapabilityFactory factory)
+    {
+        for (Iterator itr = list(classLoader).iterator(); itr.hasNext();)
+        {
+            ((AbstractFactoryRegistry) itr.next()).register(factory);
+        }
+    }
+
+
+
+    /**
+     * Registers {@link org.codehaus.cargo.container.deployable.Deployable} implementations
+     * to the given {@link DeployableFactory}.
+     *
+     * @param factory
+     *      See {@link #register(ClassLoader, DeployableFactory)}
+     */
+    protected abstract void register(DeployableFactory factory);
+
+    /**
+     * See {@link #register(DeployableFactory)} for the semantics.
+     *
+     * @param factory
+     *      See {@link #register(DeployableFactory)}
+     */
+    protected abstract void register(ConfigurationCapabilityFactory factory);
+
+    /**
+     * See {@link #register(DeployableFactory)} for the semantics.
+     *
+     * @param factory
+     *      See {@link #register(DeployableFactory)}
+     */
+    protected abstract void register(ConfigurationFactory factory);
+
+    /**
+     * See {@link #register(DeployableFactory)} for the semantics.
+     *
+     * @param factory
+     *      See {@link #register(DeployableFactory)}
+     */
+    protected abstract void register(DeployerFactory factory);
+
+    /**
+     * See {@link #register(DeployableFactory)} for the semantics.
+     *
+     * @param factory
+     *      See {@link #register(DeployableFactory)}
+     */
+    protected abstract void register(PackagerFactory factory);
+
+    /**
+     * See {@link #register(DeployableFactory)} for the semantics.
+     *
+     * @param factory
+     *      See {@link #register(DeployableFactory)}
+     */
+    protected abstract void register(ContainerFactory factory);
+
+    /**
+     * See {@link #register(DeployableFactory)} for the semantics.
+     *
+     * @param factory
+     *      See {@link #register(DeployableFactory)}
+     */
+    protected abstract void register(ContainerCapabilityFactory factory);
+
+    /**
+     * Lists up {@link AbstractFactoryRegistry}s that are discovered.
+     *
+     * @param classLoader
+     *      See {@link #register(ClassLoader, DeployableFactory)} for more details.
+     * @return always non-null but can be empty.
+     */
+    private static List list(ClassLoader classLoader)
+    {
+        ClassLoader cl = classLoader;
+        if (cl == null)
+        {
+            cl = Thread.currentThread().getContextClassLoader();
+        }
+        if (cl == null)
+        {
+            cl = AbstractFactoryRegistry.class.getClassLoader();
+        }
+        if (cl == null)
+        {
+            cl = JDKHooks.getJDKHooks().getSystemClassLoader();
+        }
+        if (cl == null)
+        {
+            // this is not our day. bail out.
+            return Collections.EMPTY_LIST;
+        }
+
+        ClassLoaders loaders = new ClassLoaders();
+        loaders.put(cl);
+
+        List registries = new ArrayList();
+        Enumeration providers = Service.providers(
+                new SPInterface(AbstractFactoryRegistry.class), loaders);
+        while (providers.hasMoreElements())
+        {
+            Object provider = providers.nextElement();
+            if (provider instanceof AbstractFactoryRegistry)
+            {
+                registries.add(provider);
+            }
+        }
+
+        return registries;
+    }
+}
Index: src/main/java/org/codehaus/cargo/generic/configuration/DefaultConfigurationFactory.java
===================================================================
--- src/main/java/org/codehaus/cargo/generic/configuration/DefaultConfigurationFactory.java (revision 1677)
+++ src/main/java/org/codehaus/cargo/generic/configuration/DefaultConfigurationFactory.java (working copy)
@@ -26,6 +26,7 @@
 import org.codehaus.cargo.generic.spi.AbstractIntrospectionGenericHintFactory;
 import org.codehaus.cargo.generic.internal.util.RegistrationKey;
 import org.codehaus.cargo.generic.internal.util.FullContainerIdentity;
+import org.codehaus.cargo.generic.AbstractFactoryRegistry;
 import org.codehaus.cargo.util.FileHandler;
 import org.codehaus.cargo.util.DefaultFileHandler;
 
@@ -62,6 +63,19 @@
      */
     public DefaultConfigurationFactory()
     {
+        this(null);
+    }
+
+    /**
+     * Register configuration name mappings.
+     *
+     * @param classLoader
+     *      ClassLoader to discover implementations from. See
+     *      {@link AbstractFactoryRegistry#register(ClassLoader, ConfigurationFactory)}
+     *      for the details of what this value means.
+     */
+    public DefaultConfigurationFactory(ClassLoader classLoader)
+    {
         super();
 
         // Note: Sorted by container id alphabetical order
@@ -161,6 +175,8 @@
             "org.codehaus.cargo.container.weblogic.WebLogicStandaloneLocalConfiguration");
         registerConfiguration("weblogic9x", ContainerType.INSTALLED, ConfigurationType.EXISTING,
             "org.codehaus.cargo.container.weblogic.WebLogicExistingLocalConfiguration");
+
+        AbstractFactoryRegistry.register(classLoader, this);
     }
 
     /**
Index: src/main/java/org/codehaus/cargo/generic/configuration/DefaultConfigurationCapabilityFactory.java
===================================================================
--- src/main/java/org/codehaus/cargo/generic/configuration/DefaultConfigurationCapabilityFactory.java (revision 1677)
+++ src/main/java/org/codehaus/cargo/generic/configuration/DefaultConfigurationCapabilityFactory.java (working copy)
@@ -22,6 +22,7 @@
 import org.codehaus.cargo.generic.spi.AbstractIntrospectionGenericHintFactory;
 import org.codehaus.cargo.generic.internal.util.RegistrationKey;
 import org.codehaus.cargo.generic.internal.util.FullContainerIdentity;
+import org.codehaus.cargo.generic.AbstractFactoryRegistry;
 import org.codehaus.cargo.container.configuration.ConfigurationType;
 import org.codehaus.cargo.container.configuration.ConfigurationCapability;
 import org.codehaus.cargo.container.ContainerType;
@@ -42,6 +43,19 @@
      */
     public DefaultConfigurationCapabilityFactory()
     {
+        this(null);
+    }
+
+    /**
+     * Register configuration capability name mappings.
+     *
+     * @param classLoader
+     *      ClassLoader to discover implementations from. See
+     *      {@link AbstractFactoryRegistry#register(ClassLoader, ConfigurationCapabilityFactory)}
+     *      for the details of what this value means.
+     */
+    public DefaultConfigurationCapabilityFactory(ClassLoader classLoader)
+    {
         super();
         // Note: We register configuration capabilities using introspection so that we don't have to
         // depend on those classes at build time nor at runtime. More specifically this allows a
@@ -189,6 +203,8 @@
         registerConfigurationCapability("weblogic9x", ContainerType.INSTALLED,
             ConfigurationType.EXISTING, "org.codehaus.cargo.container.weblogic.internal."
                 + "WebLogicExistingLocalConfigurationCapability");
+
+        AbstractFactoryRegistry.register(classLoader, this);
     }
 
     /**
Index: src/main/java/org/codehaus/cargo/generic/packager/DefaultPackagerFactory.java
===================================================================
--- src/main/java/org/codehaus/cargo/generic/packager/DefaultPackagerFactory.java (revision 1677)
+++ src/main/java/org/codehaus/cargo/generic/packager/DefaultPackagerFactory.java (working copy)
@@ -22,6 +22,7 @@
 import org.codehaus.cargo.generic.spi.AbstractIntrospectionGenericHintFactory;
 import org.codehaus.cargo.generic.internal.util.RegistrationKey;
 import org.codehaus.cargo.generic.internal.util.SimpleContainerIdentity;
+import org.codehaus.cargo.generic.AbstractFactoryRegistry;
 import org.codehaus.cargo.container.ContainerException;
 import org.codehaus.cargo.container.packager.PackagerType;
 import org.codehaus.cargo.container.packager.Packager;
@@ -55,6 +56,19 @@
      */
     public DefaultPackagerFactory()
     {
+        this(null);
+    }
+
+    /**
+     * Register packager name mappings.
+     *
+     * @param classLoader
+     *      ClassLoader to discover implementations from. See
+     *      {@link AbstractFactoryRegistry#register(ClassLoader, PackagerFactory)}
+     *      for the details of what this value means.
+     */
+    public DefaultPackagerFactory(ClassLoader classLoader)
+    {
         super();
 
         // Note: Sorted by container id alphabetical order
@@ -68,6 +82,7 @@
         registerPackager("tomcat6x", PackagerType.DIRECTORY,
             "org.codehaus.cargo.container.tomcat.TomcatDirectoryPackager");
 
+        AbstractFactoryRegistry.register(classLoader, this);
     }
 
     /**
Index: pom.xml
===================================================================
--- pom.xml (revision 1677)
+++ pom.xml (working copy)
@@ -35,6 +35,11 @@
       <scope>compile</scope>
     </dependency>
     <dependency>
+      <groupId>commons-discovery</groupId>
+      <artifactId>commons-discovery</artifactId>
+      <version>0.4</version>
+    </dependency>
+    <dependency>
       <groupId>org.codehaus.cargo</groupId>
       <artifactId>cargo-core-api-container</artifactId>
       <version>${version}</version>

[Matt Wringe]

On Tue, 2008-08-05 at 09:36 -0700, Kohsuke Kawaguchi wrote:

> Kohsuke Kawaguchi wrote:
> > Matt Wringe wrote:
> >> On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
> >>> Any news on this? Should I assume that no news is a good news?
> >>
> >> Sorry for no response from me, I looked at it briefly before and havn't
> >> had a chance to look into into it deeper. From what I saw it looked ok.
> >
> > Thanks. I'll proceed and do the same for all the types.
>
> ... and here is the patch that does the same thing for all factory types.

Awesome :)

>
>
>
> >>> Kohsuke Kawaguchi wrote:
> >>> > Matt Wringe wrote:
> >>> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> Does this direction make sense?
> >>> >>>
> >>> >>> I guess what I'm trying to avoid here is to spend time implementing this
> >>> >>> feature, only to learn later that you don't agree with the direction.
> >>> >>>
> >>> >> it makes sense to me :)
> >>> >
> >>> > Attached is the patch for this change that covers Deployable and
> >>> > DeployableFactory. If this looks OK, I'll proceed with all the other
> >>> > factory types.
> >>> >
> >>> > See the test code for how the container implementation would look like.
> >>> >
> >>> >
> >>> >>> Kohsuke Kawaguchi wrote:
> >>> >>> > Matt Wringe wrote:
> >>> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> >>> Matt Wringe wrote:
> >>> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
> >>> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
> >>> >>> >>> >
> >>> >>> >>> > Replied to the jira about how cargo already supports custom containers
> >>> >>> >>> > and configuration.
> >>> >>> >>>
> >>> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
> >>> >>> >>> me is a good feature, but IMHO it's not suffice.
> >>> >>> >>>
> >>> >>> >>>
> >>> >>> >>> >> The implementation approach that I'm thinking of is to have
> >>> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
> >>> >>> >>> >> META-INF/services and use them as delegates on methods like
> >>> >>> >>> >> "getDeployerClass". So the container implementations that would like to
> >>> >>> >>> >> hook into this would implement their own factories and write
> >>> >>> >>> >> META-INF/services entry.
> >>> >>> >>> >
> >>> >>> >>> > I do not have a lot of experience with jar service providers so I don't
> >>> >>> >>> > really know how feasible this is.
> >>> >>> >>>
> >>> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
> >>> >>> >> ok :) could you explain a bit more how exactly it would work then?
> >>> >>> >
> >>> >>> > This is a standard convention used widely everywhere, such as in many
> >>> >>> > JCP APIs.
> >>> >>> >
> >>> >>> > The technique is for the use cases where typically one wants to discover
> >>> >>> > implementations of some contract defined in the API. For example, in
> >>> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
> >>> >>> > implementations (like Xerces) without having any prior knowledge about
> >>> >>> > such an implementation.
> >>> >>> >
> >>> >>> > Here is the technique. The base contract is defined in terms of a Java
> >>> >>> > class/interface. Suppose this contract has the fully-qualified name of
> >>> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
> >>> >>> > that like to be discovered will put a file with the name
> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >>> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
> >>> >>> > qualified names of classes that implement this base interface/class.
> >>> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
> >>> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
> >>> >>> >
> >>> >>> > The discovery process takes one ClassLoader as a parameter. This
> >>> >>> > parameter is normally defaulted to
> >>> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
> >>> >>> > scenarios the caller do need to be able to pass it in.
> >>> >>> >
> >>> >>> > The API then lists up all
> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >>> >>> > from resources, parse them all, and list up all the classes. It'll
> >>> >>> > instantiate them and use them, with suitable error handling in-between.
> >>> >>> >
> >>> >>> > commons-discovery defines code to do this. Writing it from scratch is
> >>> >>> > also rather easy.
> >>> >>> >
> >>> >>> > The end result is that all that needed to be done by the user is put
> >>> >>> > "cargo-glassfish-impl.jar" in the classpath.
> >>> >>> >
> >>> >>> >
> >>> >>> >>> > I really don't like the current factory implementation and would ideally
> >>> >>> >>> > like to see cargo automatically determine the proper classes to use
> >>> >>> >>> > based on a class naming scheme (and cargo is already using a naming
> >>> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
> >>> >>> >>> > implementation for Tomcat6 for a local installed container).
> >>> >>> >>> >
> >>> >>> >>> > With the current factory implementation its too easy to make
> >>> >>> >>> mistakes.
> >>> >>> >>>
> >>> >>> >>> I guess the only problem with the naming convention is that all the
> >>> >>> >>> implementations would have to live in the "org.codehaus" package.
> >>> >>> >>
> >>> >>> >> well, it could just look at the class name and not the package name.
> >>> >>> >
> >>> >>> > OK, so your idea of pluggability must be bit different than mine. I
> >>> >>> > suspect your approach is that somehow container implementations would
> >>> >>> > tell the factory about its class names, and the factory would figure out
> >>> >>> > the ID and the type from the class name. But this is missing how the
> >>> >>> > containers tell the factory their class names in the first place,
> >>> >>> > without asking the users' help --- that is the part that I'm trying to
> >>> >>> > eliminate.
> >>> >>> >
> >>> >>> > What I was thinking was that the factory implementation now only know
> >>> >>> > about a class name convention, and whenever someone requests an
> >>> >>> > instantiation from some parameters, it just blindly tries to create a
> >>> >>> > class of certain names. If the class is there, it works, otherwise, it
> >>> >>> > won't.
> >>> >>> >
> >>> >>> > So in the latter scheme, while you can try more than one class names,
> >>> >>> > you cannot really allow arbitrary package names or class names, simply
> >>> >>> > because the factory can't know about them all.
> >>> >>> >
> >>> >>> >
> >>> >>> >>> But if the Cargo project is OK for random people to be putting classes
> >>> >>> >>> in this package,
> >>> >>> >>
> >>> >>> >> I don't think it would be ok for people to be using
> >>> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
> >>> >>> >> the code origins. Please see my comment above.
> >>> >>> >
> >>> >>> > OK. Then that eliminates convention-only approach.
> >>> >>> >
> >>> >>> >
> >>> >>> >>>  this is probably better than META-INF/services look up,
> >>> >>> >>> because the container implementor needs to write less.
> >>> >>> >>>
> >>> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
> >>> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
> >>> >>> >>> naming convention would be somewhat contrived to make it fit with the
> >>> >>> >>> current class names.
> >>> >>> >>
> >>> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
> >>> >>> >
> >>> >>> > OK.
> >>> >>> >
> >>> >>> >
> >>> >>> >>> >> I feel container developers would have to write more than they should,
> >>> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
> >>> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
> >>> >>> >>> >> I hesitate.
> >>> >>> >>> >>
> >>> >>> >>> >>
> >>> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
> >>> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
> >>> >>> >>> >> other factories.
> >>> >>> >>> >>
> >>> >>> >>> >>
> >>> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
> >>> >>> >>> >
> >>> >>> >>> > great, we need someone with glassfish knowledge
> >>> >>> >>> >
> >>> >>> >>> >>  but I think
> >>> >>> >>> >> this should be the first step, so that various people in the GlassFish
> >>> >>> >>> >> project can eat the dogfood before it can be considered for the
> >>> >>> >>> >> inclusion into Cargo core.
> >>> >>> >>> >
> >>> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
> >>> >>> >>> > before considering contributing to cargo. You can always start with some
> >>> >>> >>> > simple commands and work from there. If needed we can always add
> >>> >>> >>> > documentation about it being a feature preview.
> >>> >>> >>>
> >>> >>> >>> I think the feature still makes sense. Code that works is one reason,
> >>> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
> >>> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
> >>> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
> >>> >>> >>
> >>> >>> >> hehe, yes, I agree here.
> >>> >>> >>
> >>> >>> >>> It
> >>> >>> >>> was really painful for me to make sure every check in satisfies all the
> >>> >>> >>> check style rules.
> >>> >>> >>
> >>> >>> >> Its really not that bad when you use an ide to manage it for you. Right
> >>> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
> >>> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
> >>> >>> >> and tells you what is wrong with them (although I can't seem to figure
> >>> >>> >> out how to tell eclipse where the suppression file is).
> >>> >>> >
> >>> >>> > Yes, I was hoping that at least there are some IDEs that understand
> >>> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
> >>> >>> > available on my IDE, hence the preference to postpone till the end.
> >>> >>> >
> >>> >>> >
> >>> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
> >>> >>> >>> that work until we integrate that back into the core (and perhaps at
> >>> >>> >>> that point I can ask for other people's help.)
> >>> >>> >>
> >>> >>> >> You can also easily disabled it by passing a specific -D option during a
> >>> >>> >> build.
> >>> >>> >
> >>> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
> >>> >>> > make a commit, it better passes the checkstyle.
> >>> >>> >
> >>> >>> >> If you are really worried about the checkstyle, we could even
> >>> >>> >> temporarily disable it for the Glassfish part.
> >>> >>> >
> >>> >>> > That's good to know :-)
> >>> >>> >
> >>> >>> > But I really think it's better for the Cargo project as a whole it it
> >>> >>> > lets people write containers outside the Cargo project.
> >>> >>> >
> >>> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
> >>> >>> > people to write custom tasks outside the Apache Ant project itself, I
> >>> >>> > don't think it would have been this successful.
> >>> >>> >
> >>> >>>
> >>> >>>
> >>> >>
> >>> >>
> >>> >> ---------------------------------------------------------------------
> >>> >> To unsubscribe from this list, please visit:
> >>> >>
> >>> >>     http://xircles.codehaus.org/manage_email
> >>> >>
> >>> >>
> >>> >>
> >>> >
> >>> >
> >>>
> >>>
> >>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe from this list, please visit:
> >>
> >>     http://xircles.codehaus.org/manage_email
> >>
> >>
> >>
> >
> >
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[kohsuke]

Matt Wringe wrote:

> On Tue, 2008-08-05 at 09:36 -0700, Kohsuke Kawaguchi wrote:
>> Kohsuke Kawaguchi wrote:
>> > Matt Wringe wrote:
>> >> On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
>> >>> Any news on this? Should I assume that no news is a good news?
>> >>
>> >> Sorry for no response from me, I looked at it briefly before and havn't
>> >> had a chance to look into into it deeper. From what I saw it looked ok.
>> >
>> > Thanks. I'll proceed and do the same for all the types.
>>
>> ... and here is the patch that does the same thing for all factory types.
>
> Awesome :)

If I can commit this change, let me know.

>> >>> Kohsuke Kawaguchi wrote:
>> >>> > Matt Wringe wrote:
>> >>> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
>> >>> >>> Does this direction make sense?
>> >>> >>>
>> >>> >>> I guess what I'm trying to avoid here is to spend time implementing this
>> >>> >>> feature, only to learn later that you don't agree with the direction.
>> >>> >>>
>> >>> >> it makes sense to me :)
>> >>> >
>> >>> > Attached is the patch for this change that covers Deployable and
>> >>> > DeployableFactory. If this looks OK, I'll proceed with all the other
>> >>> > factory types.
>> >>> >
>> >>> > See the test code for how the container implementation would look like.
>> >>> >
>> >>> >
>> >>> >>> Kohsuke Kawaguchi wrote:
>> >>> >>> > Matt Wringe wrote:
>> >>> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
>> >>> >>> >>> Matt Wringe wrote:
>> >>> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>> >>> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>> >>> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
>> >>> >>> >>> >
>> >>> >>> >>> > Replied to the jira about how cargo already supports custom containers
>> >>> >>> >>> > and configuration.
>> >>> >>> >>>
>> >>> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
>> >>> >>> >>> me is a good feature, but IMHO it's not suffice.
>> >>> >>> >>>
>> >>> >>> >>>
>> >>> >>> >>> >> The implementation approach that I'm thinking of is to have
>> >>> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
>> >>> >>> >>> >> META-INF/services and use them as delegates on methods like
>> >>> >>> >>> >> "getDeployerClass". So the container implementations that would like to
>> >>> >>> >>> >> hook into this would implement their own factories and write
>> >>> >>> >>> >> META-INF/services entry.
>> >>> >>> >>> >
>> >>> >>> >>> > I do not have a lot of experience with jar service providers so I don't
>> >>> >>> >>> > really know how feasible this is.
>> >>> >>> >>>
>> >>> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
>> >>> >>> >> ok :) could you explain a bit more how exactly it would work then?
>> >>> >>> >
>> >>> >>> > This is a standard convention used widely everywhere, such as in many
>> >>> >>> > JCP APIs.
>> >>> >>> >
>> >>> >>> > The technique is for the use cases where typically one wants to discover
>> >>> >>> > implementations of some contract defined in the API. For example, in
>> >>> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
>> >>> >>> > implementations (like Xerces) without having any prior knowledge about
>> >>> >>> > such an implementation.
>> >>> >>> >
>> >>> >>> > Here is the technique. The base contract is defined in terms of a Java
>> >>> >>> > class/interface. Suppose this contract has the fully-qualified name of
>> >>> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
>> >>> >>> > that like to be discovered will put a file with the name
>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>> >>> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
>> >>> >>> > qualified names of classes that implement this base interface/class.
>> >>> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
>> >>> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
>> >>> >>> >
>> >>> >>> > The discovery process takes one ClassLoader as a parameter. This
>> >>> >>> > parameter is normally defaulted to
>> >>> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
>> >>> >>> > scenarios the caller do need to be able to pass it in.
>> >>> >>> >
>> >>> >>> > The API then lists up all
>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>> >>> >>> > from resources, parse them all, and list up all the classes. It'll
>> >>> >>> > instantiate them and use them, with suitable error handling in-between.
>> >>> >>> >
>> >>> >>> > commons-discovery defines code to do this. Writing it from scratch is
>> >>> >>> > also rather easy.
>> >>> >>> >
>> >>> >>> > The end result is that all that needed to be done by the user is put
>> >>> >>> > "cargo-glassfish-impl.jar" in the classpath.
>> >>> >>> >
>> >>> >>> >
>> >>> >>> >>> > I really don't like the current factory implementation and would ideally
>> >>> >>> >>> > like to see cargo automatically determine the proper classes to use
>> >>> >>> >>> > based on a class naming scheme (and cargo is already using a naming
>> >>> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
>> >>> >>> >>> > implementation for Tomcat6 for a local installed container).
>> >>> >>> >>> >
>> >>> >>> >>> > With the current factory implementation its too easy to make
>> >>> >>> >>> mistakes.
>> >>> >>> >>>
>> >>> >>> >>> I guess the only problem with the naming convention is that all the
>> >>> >>> >>> implementations would have to live in the "org.codehaus" package.
>> >>> >>> >>
>> >>> >>> >> well, it could just look at the class name and not the package name.
>> >>> >>> >
>> >>> >>> > OK, so your idea of pluggability must be bit different than mine. I
>> >>> >>> > suspect your approach is that somehow container implementations would
>> >>> >>> > tell the factory about its class names, and the factory would figure out
>> >>> >>> > the ID and the type from the class name. But this is missing how the
>> >>> >>> > containers tell the factory their class names in the first place,
>> >>> >>> > without asking the users' help --- that is the part that I'm trying to
>> >>> >>> > eliminate.
>> >>> >>> >
>> >>> >>> > What I was thinking was that the factory implementation now only know
>> >>> >>> > about a class name convention, and whenever someone requests an
>> >>> >>> > instantiation from some parameters, it just blindly tries to create a
>> >>> >>> > class of certain names. If the class is there, it works, otherwise, it
>> >>> >>> > won't.
>> >>> >>> >
>> >>> >>> > So in the latter scheme, while you can try more than one class names,
>> >>> >>> > you cannot really allow arbitrary package names or class names, simply
>> >>> >>> > because the factory can't know about them all.
>> >>> >>> >
>> >>> >>> >
>> >>> >>> >>> But if the Cargo project is OK for random people to be putting classes
>> >>> >>> >>> in this package,
>> >>> >>> >>
>> >>> >>> >> I don't think it would be ok for people to be using
>> >>> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
>> >>> >>> >> the code origins. Please see my comment above.
>> >>> >>> >
>> >>> >>> > OK. Then that eliminates convention-only approach.
>> >>> >>> >
>> >>> >>> >
>> >>> >>> >>>  this is probably better than META-INF/services look up,
>> >>> >>> >>> because the container implementor needs to write less.
>> >>> >>> >>>
>> >>> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
>> >>> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
>> >>> >>> >>> naming convention would be somewhat contrived to make it fit with the
>> >>> >>> >>> current class names.
>> >>> >>> >>
>> >>> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
>> >>> >>> >
>> >>> >>> > OK.
>> >>> >>> >
>> >>> >>> >
>> >>> >>> >>> >> I feel container developers would have to write more than they should,
>> >>> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
>> >>> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
>> >>> >>> >>> >> I hesitate.
>> >>> >>> >>> >>
>> >>> >>> >>> >>
>> >>> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
>> >>> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
>> >>> >>> >>> >> other factories.
>> >>> >>> >>> >>
>> >>> >>> >>> >>
>> >>> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
>> >>> >>> >>> >
>> >>> >>> >>> > great, we need someone with glassfish knowledge
>> >>> >>> >>> >
>> >>> >>> >>> >>  but I think
>> >>> >>> >>> >> this should be the first step, so that various people in the GlassFish
>> >>> >>> >>> >> project can eat the dogfood before it can be considered for the
>> >>> >>> >>> >> inclusion into Cargo core.
>> >>> >>> >>> >
>> >>> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
>> >>> >>> >>> > before considering contributing to cargo. You can always start with some
>> >>> >>> >>> > simple commands and work from there. If needed we can always add
>> >>> >>> >>> > documentation about it being a feature preview.
>> >>> >>> >>>
>> >>> >>> >>> I think the feature still makes sense. Code that works is one reason,
>> >>> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
>> >>> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
>> >>> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
>> >>> >>> >>
>> >>> >>> >> hehe, yes, I agree here.
>> >>> >>> >>
>> >>> >>> >>> It
>> >>> >>> >>> was really painful for me to make sure every check in satisfies all the
>> >>> >>> >>> check style rules.
>> >>> >>> >>
>> >>> >>> >> Its really not that bad when you use an ide to manage it for you. Right
>> >>> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
>> >>> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
>> >>> >>> >> and tells you what is wrong with them (although I can't seem to figure
>> >>> >>> >> out how to tell eclipse where the suppression file is).
>> >>> >>> >
>> >>> >>> > Yes, I was hoping that at least there are some IDEs that understand
>> >>> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
>> >>> >>> > available on my IDE, hence the preference to postpone till the end.
>> >>> >>> >
>> >>> >>> >
>> >>> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
>> >>> >>> >>> that work until we integrate that back into the core (and perhaps at
>> >>> >>> >>> that point I can ask for other people's help.)
>> >>> >>> >>
>> >>> >>> >> You can also easily disabled it by passing a specific -D option during a
>> >>> >>> >> build.
>> >>> >>> >
>> >>> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
>> >>> >>> > make a commit, it better passes the checkstyle.
>> >>> >>> >
>> >>> >>> >> If you are really worried about the checkstyle, we could even
>> >>> >>> >> temporarily disable it for the Glassfish part.
>> >>> >>> >
>> >>> >>> > That's good to know :-)
>> >>> >>> >
>> >>> >>> > But I really think it's better for the Cargo project as a whole it it
>> >>> >>> > lets people write containers outside the Cargo project.
>> >>> >>> >
>> >>> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
>> >>> >>> > people to write custom tasks outside the Apache Ant project itself, I
>> >>> >>> > don't think it would have been this successful.
>> >>> >>> >
>> >>> >>>
>> >>> >>>
>> >>> >>
>> >>> >>
>> >>> >> ---------------------------------------------------------------------
>> >>> >> To unsubscribe from this list, please visit:
>> >>> >>
>> >>> >>     http://xircles.codehaus.org/manage_email
>> >>> >>
>> >>> >>
>> >>> >>
>> >>> >
>> >>> >
>> >>>
>> >>>
>> >>
>> >>
>> >> ---------------------------------------------------------------------
>> >> To unsubscribe from this list, please visit:
>> >>
>> >>     http://xircles.codehaus.org/manage_email
>> >>
>> >>
>> >>
>> >
>> >
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>
>

--
Kohsuke Kawaguchi
Sun Microsystems                   http://weblogs.java.net/blog/kohsuke/

[kohsuke]

Kohsuke Kawaguchi wrote:

> Matt Wringe wrote:
>> On Tue, 2008-08-05 at 09:36 -0700, Kohsuke Kawaguchi wrote:
>>> Kohsuke Kawaguchi wrote:
>>> > Matt Wringe wrote:
>>> >> On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
>>> >>> Any news on this? Should I assume that no news is a good news?
>>> >>
>>> >> Sorry for no response from me, I looked at it briefly before and havn't
>>> >> had a chance to look into into it deeper. From what I saw it looked ok.
>>> >
>>> > Thanks. I'll proceed and do the same for all the types.
>>>
>>> ... and here is the patch that does the same thing for all factory types.
>>
>> Awesome :)
>
> If I can commit this change, let me know.

Sorry for my impatience, but can I proceed?

>
>
>>> >>> Kohsuke Kawaguchi wrote:
>>> >>> > Matt Wringe wrote:
>>> >>> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
>>> >>> >>> Does this direction make sense?
>>> >>> >>>
>>> >>> >>> I guess what I'm trying to avoid here is to spend time implementing this
>>> >>> >>> feature, only to learn later that you don't agree with the direction.
>>> >>> >>>
>>> >>> >> it makes sense to me :)
>>> >>> >
>>> >>> > Attached is the patch for this change that covers Deployable and
>>> >>> > DeployableFactory. If this looks OK, I'll proceed with all the other
>>> >>> > factory types.
>>> >>> >
>>> >>> > See the test code for how the container implementation would look like.
>>> >>> >
>>> >>> >
>>> >>> >>> Kohsuke Kawaguchi wrote:
>>> >>> >>> > Matt Wringe wrote:
>>> >>> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
>>> >>> >>> >>> Matt Wringe wrote:
>>> >>> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>>> >>> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>>> >>> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
>>> >>> >>> >>> >
>>> >>> >>> >>> > Replied to the jira about how cargo already supports custom containers
>>> >>> >>> >>> > and configuration.
>>> >>> >>> >>>
>>> >>> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
>>> >>> >>> >>> me is a good feature, but IMHO it's not suffice.
>>> >>> >>> >>>
>>> >>> >>> >>>
>>> >>> >>> >>> >> The implementation approach that I'm thinking of is to have
>>> >>> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
>>> >>> >>> >>> >> META-INF/services and use them as delegates on methods like
>>> >>> >>> >>> >> "getDeployerClass". So the container implementations that would like to
>>> >>> >>> >>> >> hook into this would implement their own factories and write
>>> >>> >>> >>> >> META-INF/services entry.
>>> >>> >>> >>> >
>>> >>> >>> >>> > I do not have a lot of experience with jar service providers so I don't
>>> >>> >>> >>> > really know how feasible this is.
>>> >>> >>> >>>
>>> >>> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
>>> >>> >>> >> ok :) could you explain a bit more how exactly it would work then?
>>> >>> >>> >
>>> >>> >>> > This is a standard convention used widely everywhere, such as in many
>>> >>> >>> > JCP APIs.
>>> >>> >>> >
>>> >>> >>> > The technique is for the use cases where typically one wants to discover
>>> >>> >>> > implementations of some contract defined in the API. For example, in
>>> >>> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
>>> >>> >>> > implementations (like Xerces) without having any prior knowledge about
>>> >>> >>> > such an implementation.
>>> >>> >>> >
>>> >>> >>> > Here is the technique. The base contract is defined in terms of a Java
>>> >>> >>> > class/interface. Suppose this contract has the fully-qualified name of
>>> >>> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
>>> >>> >>> > that like to be discovered will put a file with the name
>>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>>> >>> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
>>> >>> >>> > qualified names of classes that implement this base interface/class.
>>> >>> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
>>> >>> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
>>> >>> >>> >
>>> >>> >>> > The discovery process takes one ClassLoader as a parameter. This
>>> >>> >>> > parameter is normally defaulted to
>>> >>> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
>>> >>> >>> > scenarios the caller do need to be able to pass it in.
>>> >>> >>> >
>>> >>> >>> > The API then lists up all
>>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>>> >>> >>> > from resources, parse them all, and list up all the classes. It'll
>>> >>> >>> > instantiate them and use them, with suitable error handling in-between.
>>> >>> >>> >
>>> >>> >>> > commons-discovery defines code to do this. Writing it from scratch is
>>> >>> >>> > also rather easy.
>>> >>> >>> >
>>> >>> >>> > The end result is that all that needed to be done by the user is put
>>> >>> >>> > "cargo-glassfish-impl.jar" in the classpath.
>>> >>> >>> >
>>> >>> >>> >
>>> >>> >>> >>> > I really don't like the current factory implementation and would ideally
>>> >>> >>> >>> > like to see cargo automatically determine the proper classes to use
>>> >>> >>> >>> > based on a class naming scheme (and cargo is already using a naming
>>> >>> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
>>> >>> >>> >>> > implementation for Tomcat6 for a local installed container).
>>> >>> >>> >>> >
>>> >>> >>> >>> > With the current factory implementation its too easy to make
>>> >>> >>> >>> mistakes.
>>> >>> >>> >>>
>>> >>> >>> >>> I guess the only problem with the naming convention is that all the
>>> >>> >>> >>> implementations would have to live in the "org.codehaus" package.
>>> >>> >>> >>
>>> >>> >>> >> well, it could just look at the class name and not the package name.
>>> >>> >>> >
>>> >>> >>> > OK, so your idea of pluggability must be bit different than mine. I
>>> >>> >>> > suspect your approach is that somehow container implementations would
>>> >>> >>> > tell the factory about its class names, and the factory would figure out
>>> >>> >>> > the ID and the type from the class name. But this is missing how the
>>> >>> >>> > containers tell the factory their class names in the first place,
>>> >>> >>> > without asking the users' help --- that is the part that I'm trying to
>>> >>> >>> > eliminate.
>>> >>> >>> >
>>> >>> >>> > What I was thinking was that the factory implementation now only know
>>> >>> >>> > about a class name convention, and whenever someone requests an
>>> >>> >>> > instantiation from some parameters, it just blindly tries to create a
>>> >>> >>> > class of certain names. If the class is there, it works, otherwise, it
>>> >>> >>> > won't.
>>> >>> >>> >
>>> >>> >>> > So in the latter scheme, while you can try more than one class names,
>>> >>> >>> > you cannot really allow arbitrary package names or class names, simply
>>> >>> >>> > because the factory can't know about them all.
>>> >>> >>> >
>>> >>> >>> >
>>> >>> >>> >>> But if the Cargo project is OK for random people to be putting classes
>>> >>> >>> >>> in this package,
>>> >>> >>> >>
>>> >>> >>> >> I don't think it would be ok for people to be using
>>> >>> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
>>> >>> >>> >> the code origins. Please see my comment above.
>>> >>> >>> >
>>> >>> >>> > OK. Then that eliminates convention-only approach.
>>> >>> >>> >
>>> >>> >>> >
>>> >>> >>> >>>  this is probably better than META-INF/services look up,
>>> >>> >>> >>> because the container implementor needs to write less.
>>> >>> >>> >>>
>>> >>> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
>>> >>> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
>>> >>> >>> >>> naming convention would be somewhat contrived to make it fit with the
>>> >>> >>> >>> current class names.
>>> >>> >>> >>
>>> >>> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
>>> >>> >>> >
>>> >>> >>> > OK.
>>> >>> >>> >
>>> >>> >>> >
>>> >>> >>> >>> >> I feel container developers would have to write more than they should,
>>> >>> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
>>> >>> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
>>> >>> >>> >>> >> I hesitate.
>>> >>> >>> >>> >>
>>> >>> >>> >>> >>
>>> >>> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
>>> >>> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
>>> >>> >>> >>> >> other factories.
>>> >>> >>> >>> >>
>>> >>> >>> >>> >>
>>> >>> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
>>> >>> >>> >>> >
>>> >>> >>> >>> > great, we need someone with glassfish knowledge
>>> >>> >>> >>> >
>>> >>> >>> >>> >>  but I think
>>> >>> >>> >>> >> this should be the first step, so that various people in the GlassFish
>>> >>> >>> >>> >> project can eat the dogfood before it can be considered for the
>>> >>> >>> >>> >> inclusion into Cargo core.
>>> >>> >>> >>> >
>>> >>> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
>>> >>> >>> >>> > before considering contributing to cargo. You can always start with some
>>> >>> >>> >>> > simple commands and work from there. If needed we can always add
>>> >>> >>> >>> > documentation about it being a feature preview.
>>> >>> >>> >>>
>>> >>> >>> >>> I think the feature still makes sense. Code that works is one reason,
>>> >>> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
>>> >>> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
>>> >>> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
>>> >>> >>> >>
>>> >>> >>> >> hehe, yes, I agree here.
>>> >>> >>> >>
>>> >>> >>> >>> It
>>> >>> >>> >>> was really painful for me to make sure every check in satisfies all the
>>> >>> >>> >>> check style rules.
>>> >>> >>> >>
>>> >>> >>> >> Its really not that bad when you use an ide to manage it for you. Right
>>> >>> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
>>> >>> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
>>> >>> >>> >> and tells you what is wrong with them (although I can't seem to figure
>>> >>> >>> >> out how to tell eclipse where the suppression file is).
>>> >>> >>> >
>>> >>> >>> > Yes, I was hoping that at least there are some IDEs that understand
>>> >>> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
>>> >>> >>> > available on my IDE, hence the preference to postpone till the end.
>>> >>> >>> >
>>> >>> >>> >
>>> >>> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
>>> >>> >>> >>> that work until we integrate that back into the core (and perhaps at
>>> >>> >>> >>> that point I can ask for other people's help.)
>>> >>> >>> >>
>>> >>> >>> >> You can also easily disabled it by passing a specific -D option during a
>>> >>> >>> >> build.
>>> >>> >>> >
>>> >>> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
>>> >>> >>> > make a commit, it better passes the checkstyle.
>>> >>> >>> >
>>> >>> >>> >> If you are really worried about the checkstyle, we could even
>>> >>> >>> >> temporarily disable it for the Glassfish part.
>>> >>> >>> >
>>> >>> >>> > That's good to know :-)
>>> >>> >>> >
>>> >>> >>> > But I really think it's better for the Cargo project as a whole it it
>>> >>> >>> > lets people write containers outside the Cargo project.
>>> >>> >>> >
>>> >>> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
>>> >>> >>> > people to write custom tasks outside the Apache Ant project itself, I
>>> >>> >>> > don't think it would have been this successful.
>>> >>> >>> >
>>> >>> >>>
>>> >>> >>>
>>> >>> >>
>>> >>> >>
>>> >>> >> ---------------------------------------------------------------------
>>> >>> >> To unsubscribe from this list, please visit:
>>> >>> >>
>>> >>> >>     http://xircles.codehaus.org/manage_email
>>> >>> >>
>>> >>> >>
>>> >>> >>
>>> >>> >
>>> >>> >
>>> >>>
>>> >>>
>>> >>
>>> >>
>>> >> ---------------------------------------------------------------------
>>> >> To unsubscribe from this list, please visit:
>>> >>
>>> >>     http://xircles.codehaus.org/manage_email
>>> >>
>>> >>
>>> >>
>>> >
>>> >
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe from this list, please visit:
>>
>>     http://xircles.codehaus.org/manage_email
>>
>>
>>
>
>

--
Kohsuke Kawaguchi
Sun Microsystems                   http://weblogs.java.net/blog/kohsuke/

[Matt Wringe]

On Thu, 2008-08-07 at 10:52 -0700, Kohsuke Kawaguchi wrote:

> Kohsuke Kawaguchi wrote:
> > Matt Wringe wrote:
> >> On Tue, 2008-08-05 at 09:36 -0700, Kohsuke Kawaguchi wrote:
> >>> Kohsuke Kawaguchi wrote:
> >>> > Matt Wringe wrote:
> >>> >> On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> Any news on this? Should I assume that no news is a good news?
> >>> >>
> >>> >> Sorry for no response from me, I looked at it briefly before and havn't
> >>> >> had a chance to look into into it deeper. From what I saw it looked ok.
> >>> >
> >>> > Thanks. I'll proceed and do the same for all the types.
> >>>
> >>> ... and here is the patch that does the same thing for all factory types.
> >>
> >> Awesome :)
> >
> > If I can commit this change, let me know.
>
> Sorry for my impatience, but can I proceed?

I see no problems with it, although I have only quickly glanced over the
code and not actually tested it or anything.

Since no one has objected, I would think it would be ok to proceed.

>
> >
> >
> >>> >>> Kohsuke Kawaguchi wrote:
> >>> >>> > Matt Wringe wrote:
> >>> >>> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> >>> Does this direction make sense?
> >>> >>> >>>
> >>> >>> >>> I guess what I'm trying to avoid here is to spend time implementing this
> >>> >>> >>> feature, only to learn later that you don't agree with the direction.
> >>> >>> >>>
> >>> >>> >> it makes sense to me :)
> >>> >>> >
> >>> >>> > Attached is the patch for this change that covers Deployable and
> >>> >>> > DeployableFactory. If this looks OK, I'll proceed with all the other
> >>> >>> > factory types.
> >>> >>> >
> >>> >>> > See the test code for how the container implementation would look like.
> >>> >>> >
> >>> >>> >
> >>> >>> >>> Kohsuke Kawaguchi wrote:
> >>> >>> >>> > Matt Wringe wrote:
> >>> >>> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> >>> >>> Matt Wringe wrote:
> >>> >>> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
> >>> >>> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
> >>> >>> >>> >>> >
> >>> >>> >>> >>> > Replied to the jira about how cargo already supports custom containers
> >>> >>> >>> >>> > and configuration.
> >>> >>> >>> >>>
> >>> >>> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
> >>> >>> >>> >>> me is a good feature, but IMHO it's not suffice.
> >>> >>> >>> >>>
> >>> >>> >>> >>>
> >>> >>> >>> >>> >> The implementation approach that I'm thinking of is to have
> >>> >>> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
> >>> >>> >>> >>> >> META-INF/services and use them as delegates on methods like
> >>> >>> >>> >>> >> "getDeployerClass". So the container implementations that would like to
> >>> >>> >>> >>> >> hook into this would implement their own factories and write
> >>> >>> >>> >>> >> META-INF/services entry.
> >>> >>> >>> >>> >
> >>> >>> >>> >>> > I do not have a lot of experience with jar service providers so I don't
> >>> >>> >>> >>> > really know how feasible this is.
> >>> >>> >>> >>>
> >>> >>> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
> >>> >>> >>> >> ok :) could you explain a bit more how exactly it would work then?
> >>> >>> >>> >
> >>> >>> >>> > This is a standard convention used widely everywhere, such as in many
> >>> >>> >>> > JCP APIs.
> >>> >>> >>> >
> >>> >>> >>> > The technique is for the use cases where typically one wants to discover
> >>> >>> >>> > implementations of some contract defined in the API. For example, in
> >>> >>> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
> >>> >>> >>> > implementations (like Xerces) without having any prior knowledge about
> >>> >>> >>> > such an implementation.
> >>> >>> >>> >
> >>> >>> >>> > Here is the technique. The base contract is defined in terms of a Java
> >>> >>> >>> > class/interface. Suppose this contract has the fully-qualified name of
> >>> >>> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
> >>> >>> >>> > that like to be discovered will put a file with the name
> >>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >>> >>> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
> >>> >>> >>> > qualified names of classes that implement this base interface/class.
> >>> >>> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
> >>> >>> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
> >>> >>> >>> >
> >>> >>> >>> > The discovery process takes one ClassLoader as a parameter. This
> >>> >>> >>> > parameter is normally defaulted to
> >>> >>> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
> >>> >>> >>> > scenarios the caller do need to be able to pass it in.
> >>> >>> >>> >
> >>> >>> >>> > The API then lists up all
> >>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >>> >>> >>> > from resources, parse them all, and list up all the classes. It'll
> >>> >>> >>> > instantiate them and use them, with suitable error handling in-between.
> >>> >>> >>> >
> >>> >>> >>> > commons-discovery defines code to do this. Writing it from scratch is
> >>> >>> >>> > also rather easy.
> >>> >>> >>> >
> >>> >>> >>> > The end result is that all that needed to be done by the user is put
> >>> >>> >>> > "cargo-glassfish-impl.jar" in the classpath.
> >>> >>> >>> >
> >>> >>> >>> >
> >>> >>> >>> >>> > I really don't like the current factory implementation and would ideally
> >>> >>> >>> >>> > like to see cargo automatically determine the proper classes to use
> >>> >>> >>> >>> > based on a class naming scheme (and cargo is already using a naming
> >>> >>> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
> >>> >>> >>> >>> > implementation for Tomcat6 for a local installed container).
> >>> >>> >>> >>> >
> >>> >>> >>> >>> > With the current factory implementation its too easy to make
> >>> >>> >>> >>> mistakes.
> >>> >>> >>> >>>
> >>> >>> >>> >>> I guess the only problem with the naming convention is that all the
> >>> >>> >>> >>> implementations would have to live in the "org.codehaus" package.
> >>> >>> >>> >>
> >>> >>> >>> >> well, it could just look at the class name and not the package name.
> >>> >>> >>> >
> >>> >>> >>> > OK, so your idea of pluggability must be bit different than mine. I
> >>> >>> >>> > suspect your approach is that somehow container implementations would
> >>> >>> >>> > tell the factory about its class names, and the factory would figure out
> >>> >>> >>> > the ID and the type from the class name. But this is missing how the
> >>> >>> >>> > containers tell the factory their class names in the first place,
> >>> >>> >>> > without asking the users' help --- that is the part that I'm trying to
> >>> >>> >>> > eliminate.
> >>> >>> >>> >
> >>> >>> >>> > What I was thinking was that the factory implementation now only know
> >>> >>> >>> > about a class name convention, and whenever someone requests an
> >>> >>> >>> > instantiation from some parameters, it just blindly tries to create a
> >>> >>> >>> > class of certain names. If the class is there, it works, otherwise, it
> >>> >>> >>> > won't.
> >>> >>> >>> >
> >>> >>> >>> > So in the latter scheme, while you can try more than one class names,
> >>> >>> >>> > you cannot really allow arbitrary package names or class names, simply
> >>> >>> >>> > because the factory can't know about them all.
> >>> >>> >>> >
> >>> >>> >>> >
> >>> >>> >>> >>> But if the Cargo project is OK for random people to be putting classes
> >>> >>> >>> >>> in this package,
> >>> >>> >>> >>
> >>> >>> >>> >> I don't think it would be ok for people to be using
> >>> >>> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
> >>> >>> >>> >> the code origins. Please see my comment above.
> >>> >>> >>> >
> >>> >>> >>> > OK. Then that eliminates convention-only approach.
> >>> >>> >>> >
> >>> >>> >>> >
> >>> >>> >>> >>>  this is probably better than META-INF/services look up,
> >>> >>> >>> >>> because the container implementor needs to write less.
> >>> >>> >>> >>>
> >>> >>> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
> >>> >>> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
> >>> >>> >>> >>> naming convention would be somewhat contrived to make it fit with the
> >>> >>> >>> >>> current class names.
> >>> >>> >>> >>
> >>> >>> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
> >>> >>> >>> >
> >>> >>> >>> > OK.
> >>> >>> >>> >
> >>> >>> >>> >
> >>> >>> >>> >>> >> I feel container developers would have to write more than they should,
> >>> >>> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
> >>> >>> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
> >>> >>> >>> >>> >> I hesitate.
> >>> >>> >>> >>> >>
> >>> >>> >>> >>> >>
> >>> >>> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
> >>> >>> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
> >>> >>> >>> >>> >> other factories.
> >>> >>> >>> >>> >>
> >>> >>> >>> >>> >>
> >>> >>> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
> >>> >>> >>> >>> >
> >>> >>> >>> >>> > great, we need someone with glassfish knowledge
> >>> >>> >>> >>> >
> >>> >>> >>> >>> >>  but I think
> >>> >>> >>> >>> >> this should be the first step, so that various people in the GlassFish
> >>> >>> >>> >>> >> project can eat the dogfood before it can be considered for the
> >>> >>> >>> >>> >> inclusion into Cargo core.
> >>> >>> >>> >>> >
> >>> >>> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
> >>> >>> >>> >>> > before considering contributing to cargo. You can always start with some
> >>> >>> >>> >>> > simple commands and work from there. If needed we can always add
> >>> >>> >>> >>> > documentation about it being a feature preview.
> >>> >>> >>> >>>
> >>> >>> >>> >>> I think the feature still makes sense. Code that works is one reason,
> >>> >>> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
> >>> >>> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
> >>> >>> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
> >>> >>> >>> >>
> >>> >>> >>> >> hehe, yes, I agree here.
> >>> >>> >>> >>
> >>> >>> >>> >>> It
> >>> >>> >>> >>> was really painful for me to make sure every check in satisfies all the
> >>> >>> >>> >>> check style rules.
> >>> >>> >>> >>
> >>> >>> >>> >> Its really not that bad when you use an ide to manage it for you. Right
> >>> >>> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
> >>> >>> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
> >>> >>> >>> >> and tells you what is wrong with them (although I can't seem to figure
> >>> >>> >>> >> out how to tell eclipse where the suppression file is).
> >>> >>> >>> >
> >>> >>> >>> > Yes, I was hoping that at least there are some IDEs that understand
> >>> >>> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
> >>> >>> >>> > available on my IDE, hence the preference to postpone till the end.
> >>> >>> >>> >
> >>> >>> >>> >
> >>> >>> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
> >>> >>> >>> >>> that work until we integrate that back into the core (and perhaps at
> >>> >>> >>> >>> that point I can ask for other people's help.)
> >>> >>> >>> >>
> >>> >>> >>> >> You can also easily disabled it by passing a specific -D option during a
> >>> >>> >>> >> build.
> >>> >>> >>> >
> >>> >>> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
> >>> >>> >>> > make a commit, it better passes the checkstyle.
> >>> >>> >>> >
> >>> >>> >>> >> If you are really worried about the checkstyle, we could even
> >>> >>> >>> >> temporarily disable it for the Glassfish part.
> >>> >>> >>> >
> >>> >>> >>> > That's good to know :-)
> >>> >>> >>> >
> >>> >>> >>> > But I really think it's better for the Cargo project as a whole it it
> >>> >>> >>> > lets people write containers outside the Cargo project.
> >>> >>> >>> >
> >>> >>> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
> >>> >>> >>> > people to write custom tasks outside the Apache Ant project itself, I
> >>> >>> >>> > don't think it would have been this successful.
> >>> >>> >>> >
> >>> >>> >>>
> >>> >>> >>>
> >>> >>> >>
> >>> >>> >>
> >>> >>> >> ---------------------------------------------------------------------
> >>> >>> >> To unsubscribe from this list, please visit:
> >>> >>> >>
> >>> >>> >>     http://xircles.codehaus.org/manage_email
> >>> >>> >>
> >>> >>> >>
> >>> >>> >>
> >>> >>> >
> >>> >>> >
> >>> >>>
> >>> >>>
> >>> >>
> >>> >>
> >>> >> ---------------------------------------------------------------------
> >>> >> To unsubscribe from this list, please visit:
> >>> >>
> >>> >>     http://xircles.codehaus.org/manage_email
> >>> >>
> >>> >>
> >>> >>
> >>> >
> >>> >
> >>>
> >>>
> >>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe from this list, please visit:
> >>
> >>     http://xircles.codehaus.org/manage_email
> >>
> >>
> >>
> >
> >
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[kohsuke]

Thank you!

Matt Wringe wrote:

> On Thu, 2008-08-07 at 10:52 -0700, Kohsuke Kawaguchi wrote:
>> Kohsuke Kawaguchi wrote:
>> > Matt Wringe wrote:
>> >> On Tue, 2008-08-05 at 09:36 -0700, Kohsuke Kawaguchi wrote:
>> >>> Kohsuke Kawaguchi wrote:
>> >>> > Matt Wringe wrote:
>> >>> >> On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
>> >>> >>> Any news on this? Should I assume that no news is a good news?
>> >>> >>
>> >>> >> Sorry for no response from me, I looked at it briefly before and havn't
>> >>> >> had a chance to look into into it deeper. From what I saw it looked ok.
>> >>> >
>> >>> > Thanks. I'll proceed and do the same for all the types.
>> >>>
>> >>> ... and here is the patch that does the same thing for all factory types.
>> >>
>> >> Awesome :)
>> >
>> > If I can commit this change, let me know.
>>
>> Sorry for my impatience, but can I proceed?
>
> I see no problems with it, although I have only quickly glanced over the
> code and not actually tested it or anything.
>
> Since no one has objected, I would think it would be ok to proceed.
>
>>
>> >
>> >
>> >>> >>> Kohsuke Kawaguchi wrote:
>> >>> >>> > Matt Wringe wrote:
>> >>> >>> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
>> >>> >>> >>> Does this direction make sense?
>> >>> >>> >>>
>> >>> >>> >>> I guess what I'm trying to avoid here is to spend time implementing this
>> >>> >>> >>> feature, only to learn later that you don't agree with the direction.
>> >>> >>> >>>
>> >>> >>> >> it makes sense to me :)
>> >>> >>> >
>> >>> >>> > Attached is the patch for this change that covers Deployable and
>> >>> >>> > DeployableFactory. If this looks OK, I'll proceed with all the other
>> >>> >>> > factory types.
>> >>> >>> >
>> >>> >>> > See the test code for how the container implementation would look like.
>> >>> >>> >
>> >>> >>> >
>> >>> >>> >>> Kohsuke Kawaguchi wrote:
>> >>> >>> >>> > Matt Wringe wrote:
>> >>> >>> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
>> >>> >>> >>> >>> Matt Wringe wrote:
>> >>> >>> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>> >>> >>> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>> >>> >>> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
>> >>> >>> >>> >>> >
>> >>> >>> >>> >>> > Replied to the jira about how cargo already supports custom containers
>> >>> >>> >>> >>> > and configuration.
>> >>> >>> >>> >>>
>> >>> >>> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
>> >>> >>> >>> >>> me is a good feature, but IMHO it's not suffice.
>> >>> >>> >>> >>>
>> >>> >>> >>> >>>
>> >>> >>> >>> >>> >> The implementation approach that I'm thinking of is to have
>> >>> >>> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
>> >>> >>> >>> >>> >> META-INF/services and use them as delegates on methods like
>> >>> >>> >>> >>> >> "getDeployerClass". So the container implementations that would like to
>> >>> >>> >>> >>> >> hook into this would implement their own factories and write
>> >>> >>> >>> >>> >> META-INF/services entry.
>> >>> >>> >>> >>> >
>> >>> >>> >>> >>> > I do not have a lot of experience with jar service providers so I don't
>> >>> >>> >>> >>> > really know how feasible this is.
>> >>> >>> >>> >>>
>> >>> >>> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
>> >>> >>> >>> >> ok :) could you explain a bit more how exactly it would work then?
>> >>> >>> >>> >
>> >>> >>> >>> > This is a standard convention used widely everywhere, such as in many
>> >>> >>> >>> > JCP APIs.
>> >>> >>> >>> >
>> >>> >>> >>> > The technique is for the use cases where typically one wants to discover
>> >>> >>> >>> > implementations of some contract defined in the API. For example, in
>> >>> >>> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
>> >>> >>> >>> > implementations (like Xerces) without having any prior knowledge about
>> >>> >>> >>> > such an implementation.
>> >>> >>> >>> >
>> >>> >>> >>> > Here is the technique. The base contract is defined in terms of a Java
>> >>> >>> >>> > class/interface. Suppose this contract has the fully-qualified name of
>> >>> >>> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
>> >>> >>> >>> > that like to be discovered will put a file with the name
>> >>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>> >>> >>> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
>> >>> >>> >>> > qualified names of classes that implement this base interface/class.
>> >>> >>> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
>> >>> >>> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
>> >>> >>> >>> >
>> >>> >>> >>> > The discovery process takes one ClassLoader as a parameter. This
>> >>> >>> >>> > parameter is normally defaulted to
>> >>> >>> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
>> >>> >>> >>> > scenarios the caller do need to be able to pass it in.
>> >>> >>> >>> >
>> >>> >>> >>> > The API then lists up all
>> >>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>> >>> >>> >>> > from resources, parse them all, and list up all the classes. It'll
>> >>> >>> >>> > instantiate them and use them, with suitable error handling in-between.
>> >>> >>> >>> >
>> >>> >>> >>> > commons-discovery defines code to do this. Writing it from scratch is
>> >>> >>> >>> > also rather easy.
>> >>> >>> >>> >
>> >>> >>> >>> > The end result is that all that needed to be done by the user is put
>> >>> >>> >>> > "cargo-glassfish-impl.jar" in the classpath.
>> >>> >>> >>> >
>> >>> >>> >>> >
>> >>> >>> >>> >>> > I really don't like the current factory implementation and would ideally
>> >>> >>> >>> >>> > like to see cargo automatically determine the proper classes to use
>> >>> >>> >>> >>> > based on a class naming scheme (and cargo is already using a naming
>> >>> >>> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
>> >>> >>> >>> >>> > implementation for Tomcat6 for a local installed container).
>> >>> >>> >>> >>> >
>> >>> >>> >>> >>> > With the current factory implementation its too easy to make
>> >>> >>> >>> >>> mistakes.
>> >>> >>> >>> >>>
>> >>> >>> >>> >>> I guess the only problem with the naming convention is that all the
>> >>> >>> >>> >>> implementations would have to live in the "org.codehaus" package.
>> >>> >>> >>> >>
>> >>> >>> >>> >> well, it could just look at the class name and not the package name.
>> >>> >>> >>> >
>> >>> >>> >>> > OK, so your idea of pluggability must be bit different than mine. I
>> >>> >>> >>> > suspect your approach is that somehow container implementations would
>> >>> >>> >>> > tell the factory about its class names, and the factory would figure out
>> >>> >>> >>> > the ID and the type from the class name. But this is missing how the
>> >>> >>> >>> > containers tell the factory their class names in the first place,
>> >>> >>> >>> > without asking the users' help --- that is the part that I'm trying to
>> >>> >>> >>> > eliminate.
>> >>> >>> >>> >
>> >>> >>> >>> > What I was thinking was that the factory implementation now only know
>> >>> >>> >>> > about a class name convention, and whenever someone requests an
>> >>> >>> >>> > instantiation from some parameters, it just blindly tries to create a
>> >>> >>> >>> > class of certain names. If the class is there, it works, otherwise, it
>> >>> >>> >>> > won't.
>> >>> >>> >>> >
>> >>> >>> >>> > So in the latter scheme, while you can try more than one class names,
>> >>> >>> >>> > you cannot really allow arbitrary package names or class names, simply
>> >>> >>> >>> > because the factory can't know about them all.
>> >>> >>> >>> >
>> >>> >>> >>> >
>> >>> >>> >>> >>> But if the Cargo project is OK for random people to be putting classes
>> >>> >>> >>> >>> in this package,
>> >>> >>> >>> >>
>> >>> >>> >>> >> I don't think it would be ok for people to be using
>> >>> >>> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
>> >>> >>> >>> >> the code origins. Please see my comment above.
>> >>> >>> >>> >
>> >>> >>> >>> > OK. Then that eliminates convention-only approach.
>> >>> >>> >>> >
>> >>> >>> >>> >
>> >>> >>> >>> >>>  this is probably better than META-INF/services look up,
>> >>> >>> >>> >>> because the container implementor needs to write less.
>> >>> >>> >>> >>>
>> >>> >>> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
>> >>> >>> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
>> >>> >>> >>> >>> naming convention would be somewhat contrived to make it fit with the
>> >>> >>> >>> >>> current class names.
>> >>> >>> >>> >>
>> >>> >>> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
>> >>> >>> >>> >
>> >>> >>> >>> > OK.
>> >>> >>> >>> >
>> >>> >>> >>> >
>> >>> >>> >>> >>> >> I feel container developers would have to write more than they should,
>> >>> >>> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
>> >>> >>> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
>> >>> >>> >>> >>> >> I hesitate.
>> >>> >>> >>> >>> >>
>> >>> >>> >>> >>> >>
>> >>> >>> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
>> >>> >>> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
>> >>> >>> >>> >>> >> other factories.
>> >>> >>> >>> >>> >>
>> >>> >>> >>> >>> >>
>> >>> >>> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
>> >>> >>> >>> >>> >
>> >>> >>> >>> >>> > great, we need someone with glassfish knowledge
>> >>> >>> >>> >>> >
>> >>> >>> >>> >>> >>  but I think
>> >>> >>> >>> >>> >> this should be the first step, so that various people in the GlassFish
>> >>> >>> >>> >>> >> project can eat the dogfood before it can be considered for the
>> >>> >>> >>> >>> >> inclusion into Cargo core.
>> >>> >>> >>> >>> >
>> >>> >>> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
>> >>> >>> >>> >>> > before considering contributing to cargo. You can always start with some
>> >>> >>> >>> >>> > simple commands and work from there. If needed we can always add
>> >>> >>> >>> >>> > documentation about it being a feature preview.
>> >>> >>> >>> >>>
>> >>> >>> >>> >>> I think the feature still makes sense. Code that works is one reason,
>> >>> >>> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
>> >>> >>> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
>> >>> >>> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
>> >>> >>> >>> >>
>> >>> >>> >>> >> hehe, yes, I agree here.
>> >>> >>> >>> >>
>> >>> >>> >>> >>> It
>> >>> >>> >>> >>> was really painful for me to make sure every check in satisfies all the
>> >>> >>> >>> >>> check style rules.
>> >>> >>> >>> >>
>> >>> >>> >>> >> Its really not that bad when you use an ide to manage it for you. Right
>> >>> >>> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
>> >>> >>> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
>> >>> >>> >>> >> and tells you what is wrong with them (although I can't seem to figure
>> >>> >>> >>> >> out how to tell eclipse where the suppression file is).
>> >>> >>> >>> >
>> >>> >>> >>> > Yes, I was hoping that at least there are some IDEs that understand
>> >>> >>> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
>> >>> >>> >>> > available on my IDE, hence the preference to postpone till the end.
>> >>> >>> >>> >
>> >>> >>> >>> >
>> >>> >>> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
>> >>> >>> >>> >>> that work until we integrate that back into the core (and perhaps at
>> >>> >>> >>> >>> that point I can ask for other people's help.)
>> >>> >>> >>> >>
>> >>> >>> >>> >> You can also easily disabled it by passing a specific -D option during a
>> >>> >>> >>> >> build.
>> >>> >>> >>> >
>> >>> >>> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
>> >>> >>> >>> > make a commit, it better passes the checkstyle.
>> >>> >>> >>> >
>> >>> >>> >>> >> If you are really worried about the checkstyle, we could even
>> >>> >>> >>> >> temporarily disable it for the Glassfish part.
>> >>> >>> >>> >
>> >>> >>> >>> > That's good to know :-)
>> >>> >>> >>> >
>> >>> >>> >>> > But I really think it's better for the Cargo project as a whole it it
>> >>> >>> >>> > lets people write containers outside the Cargo project.
>> >>> >>> >>> >
>> >>> >>> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
>> >>> >>> >>> > people to write custom tasks outside the Apache Ant project itself, I
>> >>> >>> >>> > don't think it would have been this successful.
>> >>> >>> >>> >
>> >>> >>> >>>
>> >>> >>> >>>
>> >>> >>> >>
>> >>> >>> >>
>> >>> >>> >> ---------------------------------------------------------------------
>> >>> >>> >> To unsubscribe from this list, please visit:
>> >>> >>> >>
>> >>> >>> >>     http://xircles.codehaus.org/manage_email
>> >>> >>> >>
>> >>> >>> >>
>> >>> >>> >>
>> >>> >>> >
>> >>> >>> >
>> >>> >>>
>> >>> >>>
>> >>> >>
>> >>> >>
>> >>> >> ---------------------------------------------------------------------
>> >>> >> To unsubscribe from this list, please visit:
>> >>> >>
>> >>> >>     http://xircles.codehaus.org/manage_email
>> >>> >>
>> >>> >>
>> >>> >>
>> >>> >
>> >>> >
>> >>>
>> >>>
>> >>
>> >>
>> >> ---------------------------------------------------------------------
>> >> To unsubscribe from this list, please visit:
>> >>
>> >>     http://xircles.codehaus.org/manage_email
>> >>
>> >>
>> >>
>> >
>> >
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>
>

--
Kohsuke Kawaguchi
Sun Microsystems                   http://weblogs.java.net/blog/kohsuke/

[Matt Wringe]

On Thu, 2008-08-07 at 14:33 -0700, Kohsuke Kawaguchi wrote:
> Thank you!

I just did some quick testing with it, it mostly works fine for me
except you are missing a dependency on commons-discovery.

> Matt Wringe wrote:
> > On Thu, 2008-08-07 at 10:52 -0700, Kohsuke Kawaguchi wrote:
> >> Kohsuke Kawaguchi wrote:
> >> > Matt Wringe wrote:
> >> >> On Tue, 2008-08-05 at 09:36 -0700, Kohsuke Kawaguchi wrote:
> >> >>> Kohsuke Kawaguchi wrote:
> >> >>> > Matt Wringe wrote:
> >> >>> >> On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
> >> >>> >>> Any news on this? Should I assume that no news is a good news?
> >> >>> >>
> >> >>> >> Sorry for no response from me, I looked at it briefly before and havn't
> >> >>> >> had a chance to look into into it deeper. From what I saw it looked ok.
> >> >>> >
> >> >>> > Thanks. I'll proceed and do the same for all the types.
> >> >>>
> >> >>> ... and here is the patch that does the same thing for all factory types.
> >> >>
> >> >> Awesome :)
> >> >
> >> > If I can commit this change, let me know.
> >>
> >> Sorry for my impatience, but can I proceed?
> >
> > I see no problems with it, although I have only quickly glanced over the
> > code and not actually tested it or anything.
> >
> > Since no one has objected, I would think it would be ok to proceed.
> >
> >>
> >> >
> >> >
> >> >>> >>> Kohsuke Kawaguchi wrote:
> >> >>> >>> > Matt Wringe wrote:
> >> >>> >>> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
> >> >>> >>> >>> Does this direction make sense?
> >> >>> >>> >>>
> >> >>> >>> >>> I guess what I'm trying to avoid here is to spend time implementing this
> >> >>> >>> >>> feature, only to learn later that you don't agree with the direction.
> >> >>> >>> >>>
> >> >>> >>> >> it makes sense to me :)
> >> >>> >>> >
> >> >>> >>> > Attached is the patch for this change that covers Deployable and
> >> >>> >>> > DeployableFactory. If this looks OK, I'll proceed with all the other
> >> >>> >>> > factory types.
> >> >>> >>> >
> >> >>> >>> > See the test code for how the container implementation would look like.
> >> >>> >>> >
> >> >>> >>> >
> >> >>> >>> >>> Kohsuke Kawaguchi wrote:
> >> >>> >>> >>> > Matt Wringe wrote:
> >> >>> >>> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
> >> >>> >>> >>> >>> Matt Wringe wrote:
> >> >>> >>> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
> >> >>> >>> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
> >> >>> >>> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
> >> >>> >>> >>> >>> >
> >> >>> >>> >>> >>> > Replied to the jira about how cargo already supports custom containers
> >> >>> >>> >>> >>> > and configuration.
> >> >>> >>> >>> >>>
> >> >>> >>> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
> >> >>> >>> >>> >>> me is a good feature, but IMHO it's not suffice.
> >> >>> >>> >>> >>>
> >> >>> >>> >>> >>>
> >> >>> >>> >>> >>> >> The implementation approach that I'm thinking of is to have
> >> >>> >>> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
> >> >>> >>> >>> >>> >> META-INF/services and use them as delegates on methods like
> >> >>> >>> >>> >>> >> "getDeployerClass". So the container implementations that would like to
> >> >>> >>> >>> >>> >> hook into this would implement their own factories and write
> >> >>> >>> >>> >>> >> META-INF/services entry.
> >> >>> >>> >>> >>> >
> >> >>> >>> >>> >>> > I do not have a lot of experience with jar service providers so I don't
> >> >>> >>> >>> >>> > really know how feasible this is.
> >> >>> >>> >>> >>>
> >> >>> >>> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
> >> >>> >>> >>> >> ok :) could you explain a bit more how exactly it would work then?
> >> >>> >>> >>> >
> >> >>> >>> >>> > This is a standard convention used widely everywhere, such as in many
> >> >>> >>> >>> > JCP APIs.
> >> >>> >>> >>> >
> >> >>> >>> >>> > The technique is for the use cases where typically one wants to discover
> >> >>> >>> >>> > implementations of some contract defined in the API. For example, in
> >> >>> >>> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
> >> >>> >>> >>> > implementations (like Xerces) without having any prior knowledge about
> >> >>> >>> >>> > such an implementation.
> >> >>> >>> >>> >
> >> >>> >>> >>> > Here is the technique. The base contract is defined in terms of a Java
> >> >>> >>> >>> > class/interface. Suppose this contract has the fully-qualified name of
> >> >>> >>> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
> >> >>> >>> >>> > that like to be discovered will put a file with the name
> >> >>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >> >>> >>> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
> >> >>> >>> >>> > qualified names of classes that implement this base interface/class.
> >> >>> >>> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
> >> >>> >>> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
> >> >>> >>> >>> >
> >> >>> >>> >>> > The discovery process takes one ClassLoader as a parameter. This
> >> >>> >>> >>> > parameter is normally defaulted to
> >> >>> >>> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
> >> >>> >>> >>> > scenarios the caller do need to be able to pass it in.
> >> >>> >>> >>> >
> >> >>> >>> >>> > The API then lists up all
> >> >>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >> >>> >>> >>> > from resources, parse them all, and list up all the classes. It'll
> >> >>> >>> >>> > instantiate them and use them, with suitable error handling in-between.
> >> >>> >>> >>> >
> >> >>> >>> >>> > commons-discovery defines code to do this. Writing it from scratch is
> >> >>> >>> >>> > also rather easy.
> >> >>> >>> >>> >
> >> >>> >>> >>> > The end result is that all that needed to be done by the user is put
> >> >>> >>> >>> > "cargo-glassfish-impl.jar" in the classpath.
> >> >>> >>> >>> >
> >> >>> >>> >>> >
> >> >>> >>> >>> >>> > I really don't like the current factory implementation and would ideally
> >> >>> >>> >>> >>> > like to see cargo automatically determine the proper classes to use
> >> >>> >>> >>> >>> > based on a class naming scheme (and cargo is already using a naming
> >> >>> >>> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
> >> >>> >>> >>> >>> > implementation for Tomcat6 for a local installed container).
> >> >>> >>> >>> >>> >
> >> >>> >>> >>> >>> > With the current factory implementation its too easy to make
> >> >>> >>> >>> >>> mistakes.
> >> >>> >>> >>> >>>
> >> >>> >>> >>> >>> I guess the only problem with the naming convention is that all the
> >> >>> >>> >>> >>> implementations would have to live in the "org.codehaus" package.
> >> >>> >>> >>> >>
> >> >>> >>> >>> >> well, it could just look at the class name and not the package name.
> >> >>> >>> >>> >
> >> >>> >>> >>> > OK, so your idea of pluggability must be bit different than mine. I
> >> >>> >>> >>> > suspect your approach is that somehow container implementations would
> >> >>> >>> >>> > tell the factory about its class names, and the factory would figure out
> >> >>> >>> >>> > the ID and the type from the class name. But this is missing how the
> >> >>> >>> >>> > containers tell the factory their class names in the first place,
> >> >>> >>> >>> > without asking the users' help --- that is the part that I'm trying to
> >> >>> >>> >>> > eliminate.
> >> >>> >>> >>> >
> >> >>> >>> >>> > What I was thinking was that the factory implementation now only know
> >> >>> >>> >>> > about a class name convention, and whenever someone requests an
> >> >>> >>> >>> > instantiation from some parameters, it just blindly tries to create a
> >> >>> >>> >>> > class of certain names. If the class is there, it works, otherwise, it
> >> >>> >>> >>> > won't.
> >> >>> >>> >>> >
> >> >>> >>> >>> > So in the latter scheme, while you can try more than one class names,
> >> >>> >>> >>> > you cannot really allow arbitrary package names or class names, simply
> >> >>> >>> >>> > because the factory can't know about them all.
> >> >>> >>> >>> >
> >> >>> >>> >>> >
> >> >>> >>> >>> >>> But if the Cargo project is OK for random people to be putting classes
> >> >>> >>> >>> >>> in this package,
> >> >>> >>> >>> >>
> >> >>> >>> >>> >> I don't think it would be ok for people to be using
> >> >>> >>> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
> >> >>> >>> >>> >> the code origins. Please see my comment above.
> >> >>> >>> >>> >
> >> >>> >>> >>> > OK. Then that eliminates convention-only approach.
> >> >>> >>> >>> >
> >> >>> >>> >>> >
> >> >>> >>> >>> >>>  this is probably better than META-INF/services look up,
> >> >>> >>> >>> >>> because the container implementor needs to write less.
> >> >>> >>> >>> >>>
> >> >>> >>> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
> >> >>> >>> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
> >> >>> >>> >>> >>> naming convention would be somewhat contrived to make it fit with the
> >> >>> >>> >>> >>> current class names.
> >> >>> >>> >>> >>
> >> >>> >>> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
> >> >>> >>> >>> >
> >> >>> >>> >>> > OK.
> >> >>> >>> >>> >
> >> >>> >>> >>> >
> >> >>> >>> >>> >>> >> I feel container developers would have to write more than they should,
> >> >>> >>> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
> >> >>> >>> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
> >> >>> >>> >>> >>> >> I hesitate.
> >> >>> >>> >>> >>> >>
> >> >>> >>> >>> >>> >>
> >> >>> >>> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
> >> >>> >>> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
> >> >>> >>> >>> >>> >> other factories.
> >> >>> >>> >>> >>> >>
> >> >>> >>> >>> >>> >>
> >> >>> >>> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
> >> >>> >>> >>> >>> >
> >> >>> >>> >>> >>> > great, we need someone with glassfish knowledge
> >> >>> >>> >>> >>> >
> >> >>> >>> >>> >>> >>  but I think
> >> >>> >>> >>> >>> >> this should be the first step, so that various people in the GlassFish
> >> >>> >>> >>> >>> >> project can eat the dogfood before it can be considered for the
> >> >>> >>> >>> >>> >> inclusion into Cargo core.
> >> >>> >>> >>> >>> >
> >> >>> >>> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
> >> >>> >>> >>> >>> > before considering contributing to cargo. You can always start with some
> >> >>> >>> >>> >>> > simple commands and work from there. If needed we can always add
> >> >>> >>> >>> >>> > documentation about it being a feature preview.
> >> >>> >>> >>> >>>
> >> >>> >>> >>> >>> I think the feature still makes sense. Code that works is one reason,
> >> >>> >>> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
> >> >>> >>> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
> >> >>> >>> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
> >> >>> >>> >>> >>
> >> >>> >>> >>> >> hehe, yes, I agree here.
> >> >>> >>> >>> >>
> >> >>> >>> >>> >>> It
> >> >>> >>> >>> >>> was really painful for me to make sure every check in satisfies all the
> >> >>> >>> >>> >>> check style rules.
> >> >>> >>> >>> >>
> >> >>> >>> >>> >> Its really not that bad when you use an ide to manage it for you. Right
> >> >>> >>> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
> >> >>> >>> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
> >> >>> >>> >>> >> and tells you what is wrong with them (although I can't seem to figure
> >> >>> >>> >>> >> out how to tell eclipse where the suppression file is).
> >> >>> >>> >>> >
> >> >>> >>> >>> > Yes, I was hoping that at least there are some IDEs that understand
> >> >>> >>> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
> >> >>> >>> >>> > available on my IDE, hence the preference to postpone till the end.
> >> >>> >>> >>> >
> >> >>> >>> >>> >
> >> >>> >>> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
> >> >>> >>> >>> >>> that work until we integrate that back into the core (and perhaps at
> >> >>> >>> >>> >>> that point I can ask for other people's help.)
> >> >>> >>> >>> >>
> >> >>> >>> >>> >> You can also easily disabled it by passing a specific -D option during a
> >> >>> >>> >>> >> build.
> >> >>> >>> >>> >
> >> >>> >>> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
> >> >>> >>> >>> > make a commit, it better passes the checkstyle.
> >> >>> >>> >>> >
> >> >>> >>> >>> >> If you are really worried about the checkstyle, we could even
> >> >>> >>> >>> >> temporarily disable it for the Glassfish part.
> >> >>> >>> >>> >
> >> >>> >>> >>> > That's good to know :-)
> >> >>> >>> >>> >
> >> >>> >>> >>> > But I really think it's better for the Cargo project as a whole it it
> >> >>> >>> >>> > lets people write containers outside the Cargo project.
> >> >>> >>> >>> >
> >> >>> >>> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
> >> >>> >>> >>> > people to write custom tasks outside the Apache Ant project itself, I
> >> >>> >>> >>> > don't think it would have been this successful.
> >> >>> >>> >>> >
> >> >>> >>> >>>
> >> >>> >>> >>>
> >> >>> >>> >>
> >> >>> >>> >>
> >> >>> >>> >> ---------------------------------------------------------------------
> >> >>> >>> >> To unsubscribe from this list, please visit:
> >> >>> >>> >>
> >> >>> >>> >>     http://xircles.codehaus.org/manage_email
> >> >>> >>> >>
> >> >>> >>> >>
> >> >>> >>> >>
> >> >>> >>> >
> >> >>> >>> >
> >> >>> >>>
> >> >>> >>>
> >> >>> >>
> >> >>> >>
> >> >>> >> ---------------------------------------------------------------------
> >> >>> >> To unsubscribe from this list, please visit:
> >> >>> >>
> >> >>> >>     http://xircles.codehaus.org/manage_email
> >> >>> >>
> >> >>> >>
> >> >>> >>
> >> >>> >
> >> >>> >
> >> >>>
> >> >>>
> >> >>
> >> >>
> >> >> ---------------------------------------------------------------------
> >> >> To unsubscribe from this list, please visit:
> >> >>
> >> >>     http://xircles.codehaus.org/manage_email
> >> >>
> >> >>
> >> >>
> >> >
> >> >
> >>
> >>
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe from this list, please visit:
> >
> >     http://xircles.codehaus.org/manage_email
> >
> >
> >
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[kohsuke]

Argh. I'm sorry, I forgot to commit that file. Should I commit the
change or are you going to?

Matt Wringe wrote:

> On Thu, 2008-08-07 at 14:33 -0700, Kohsuke Kawaguchi wrote:
>> Thank you!
>
> I just did some quick testing with it, it mostly works fine for me
> except you are missing a dependency on commons-discovery.
>
>> Matt Wringe wrote:
>> > On Thu, 2008-08-07 at 10:52 -0700, Kohsuke Kawaguchi wrote:
>> >> Kohsuke Kawaguchi wrote:
>> >> > Matt Wringe wrote:
>> >> >> On Tue, 2008-08-05 at 09:36 -0700, Kohsuke Kawaguchi wrote:
>> >> >>> Kohsuke Kawaguchi wrote:
>> >> >>> > Matt Wringe wrote:
>> >> >>> >> On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
>> >> >>> >>> Any news on this? Should I assume that no news is a good news?
>> >> >>> >>
>> >> >>> >> Sorry for no response from me, I looked at it briefly before and havn't
>> >> >>> >> had a chance to look into into it deeper. From what I saw it looked ok.
>> >> >>> >
>> >> >>> > Thanks. I'll proceed and do the same for all the types.
>> >> >>>
>> >> >>> ... and here is the patch that does the same thing for all factory types.
>> >> >>
>> >> >> Awesome :)
>> >> >
>> >> > If I can commit this change, let me know.
>> >>
>> >> Sorry for my impatience, but can I proceed?
>> >
>> > I see no problems with it, although I have only quickly glanced over the
>> > code and not actually tested it or anything.
>> >
>> > Since no one has objected, I would think it would be ok to proceed.
>> >
>> >>
>> >> >
>> >> >
>> >> >>> >>> Kohsuke Kawaguchi wrote:
>> >> >>> >>> > Matt Wringe wrote:
>> >> >>> >>> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
>> >> >>> >>> >>> Does this direction make sense?
>> >> >>> >>> >>>
>> >> >>> >>> >>> I guess what I'm trying to avoid here is to spend time implementing this
>> >> >>> >>> >>> feature, only to learn later that you don't agree with the direction.
>> >> >>> >>> >>>
>> >> >>> >>> >> it makes sense to me :)
>> >> >>> >>> >
>> >> >>> >>> > Attached is the patch for this change that covers Deployable and
>> >> >>> >>> > DeployableFactory. If this looks OK, I'll proceed with all the other
>> >> >>> >>> > factory types.
>> >> >>> >>> >
>> >> >>> >>> > See the test code for how the container implementation would look like.
>> >> >>> >>> >
>> >> >>> >>> >
>> >> >>> >>> >>> Kohsuke Kawaguchi wrote:
>> >> >>> >>> >>> > Matt Wringe wrote:
>> >> >>> >>> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
>> >> >>> >>> >>> >>> Matt Wringe wrote:
>> >> >>> >>> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
>> >> >>> >>> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
>> >> >>> >>> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
>> >> >>> >>> >>> >>> >
>> >> >>> >>> >>> >>> > Replied to the jira about how cargo already supports custom containers
>> >> >>> >>> >>> >>> > and configuration.
>> >> >>> >>> >>> >>>
>> >> >>> >>> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
>> >> >>> >>> >>> >>> me is a good feature, but IMHO it's not suffice.
>> >> >>> >>> >>> >>>
>> >> >>> >>> >>> >>>
>> >> >>> >>> >>> >>> >> The implementation approach that I'm thinking of is to have
>> >> >>> >>> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
>> >> >>> >>> >>> >>> >> META-INF/services and use them as delegates on methods like
>> >> >>> >>> >>> >>> >> "getDeployerClass". So the container implementations that would like to
>> >> >>> >>> >>> >>> >> hook into this would implement their own factories and write
>> >> >>> >>> >>> >>> >> META-INF/services entry.
>> >> >>> >>> >>> >>> >
>> >> >>> >>> >>> >>> > I do not have a lot of experience with jar service providers so I don't
>> >> >>> >>> >>> >>> > really know how feasible this is.
>> >> >>> >>> >>> >>>
>> >> >>> >>> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
>> >> >>> >>> >>> >> ok :) could you explain a bit more how exactly it would work then?
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > This is a standard convention used widely everywhere, such as in many
>> >> >>> >>> >>> > JCP APIs.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > The technique is for the use cases where typically one wants to discover
>> >> >>> >>> >>> > implementations of some contract defined in the API. For example, in
>> >> >>> >>> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
>> >> >>> >>> >>> > implementations (like Xerces) without having any prior knowledge about
>> >> >>> >>> >>> > such an implementation.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > Here is the technique. The base contract is defined in terms of a Java
>> >> >>> >>> >>> > class/interface. Suppose this contract has the fully-qualified name of
>> >> >>> >>> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
>> >> >>> >>> >>> > that like to be discovered will put a file with the name
>> >> >>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>> >> >>> >>> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
>> >> >>> >>> >>> > qualified names of classes that implement this base interface/class.
>> >> >>> >>> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
>> >> >>> >>> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > The discovery process takes one ClassLoader as a parameter. This
>> >> >>> >>> >>> > parameter is normally defaulted to
>> >> >>> >>> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
>> >> >>> >>> >>> > scenarios the caller do need to be able to pass it in.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > The API then lists up all
>> >> >>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
>> >> >>> >>> >>> > from resources, parse them all, and list up all the classes. It'll
>> >> >>> >>> >>> > instantiate them and use them, with suitable error handling in-between.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > commons-discovery defines code to do this. Writing it from scratch is
>> >> >>> >>> >>> > also rather easy.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > The end result is that all that needed to be done by the user is put
>> >> >>> >>> >>> > "cargo-glassfish-impl.jar" in the classpath.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >>> > I really don't like the current factory implementation and would ideally
>> >> >>> >>> >>> >>> > like to see cargo automatically determine the proper classes to use
>> >> >>> >>> >>> >>> > based on a class naming scheme (and cargo is already using a naming
>> >> >>> >>> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
>> >> >>> >>> >>> >>> > implementation for Tomcat6 for a local installed container).
>> >> >>> >>> >>> >>> >
>> >> >>> >>> >>> >>> > With the current factory implementation its too easy to make
>> >> >>> >>> >>> >>> mistakes.
>> >> >>> >>> >>> >>>
>> >> >>> >>> >>> >>> I guess the only problem with the naming convention is that all the
>> >> >>> >>> >>> >>> implementations would have to live in the "org.codehaus" package.
>> >> >>> >>> >>> >>
>> >> >>> >>> >>> >> well, it could just look at the class name and not the package name.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > OK, so your idea of pluggability must be bit different than mine. I
>> >> >>> >>> >>> > suspect your approach is that somehow container implementations would
>> >> >>> >>> >>> > tell the factory about its class names, and the factory would figure out
>> >> >>> >>> >>> > the ID and the type from the class name. But this is missing how the
>> >> >>> >>> >>> > containers tell the factory their class names in the first place,
>> >> >>> >>> >>> > without asking the users' help --- that is the part that I'm trying to
>> >> >>> >>> >>> > eliminate.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > What I was thinking was that the factory implementation now only know
>> >> >>> >>> >>> > about a class name convention, and whenever someone requests an
>> >> >>> >>> >>> > instantiation from some parameters, it just blindly tries to create a
>> >> >>> >>> >>> > class of certain names. If the class is there, it works, otherwise, it
>> >> >>> >>> >>> > won't.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > So in the latter scheme, while you can try more than one class names,
>> >> >>> >>> >>> > you cannot really allow arbitrary package names or class names, simply
>> >> >>> >>> >>> > because the factory can't know about them all.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >>> But if the Cargo project is OK for random people to be putting classes
>> >> >>> >>> >>> >>> in this package,
>> >> >>> >>> >>> >>
>> >> >>> >>> >>> >> I don't think it would be ok for people to be using
>> >> >>> >>> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
>> >> >>> >>> >>> >> the code origins. Please see my comment above.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > OK. Then that eliminates convention-only approach.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >>>  this is probably better than META-INF/services look up,
>> >> >>> >>> >>> >>> because the container implementor needs to write less.
>> >> >>> >>> >>> >>>
>> >> >>> >>> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
>> >> >>> >>> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
>> >> >>> >>> >>> >>> naming convention would be somewhat contrived to make it fit with the
>> >> >>> >>> >>> >>> current class names.
>> >> >>> >>> >>> >>
>> >> >>> >>> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > OK.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >>> >> I feel container developers would have to write more than they should,
>> >> >>> >>> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
>> >> >>> >>> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
>> >> >>> >>> >>> >>> >> I hesitate.
>> >> >>> >>> >>> >>> >>
>> >> >>> >>> >>> >>> >>
>> >> >>> >>> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
>> >> >>> >>> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
>> >> >>> >>> >>> >>> >> other factories.
>> >> >>> >>> >>> >>> >>
>> >> >>> >>> >>> >>> >>
>> >> >>> >>> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
>> >> >>> >>> >>> >>> >
>> >> >>> >>> >>> >>> > great, we need someone with glassfish knowledge
>> >> >>> >>> >>> >>> >
>> >> >>> >>> >>> >>> >>  but I think
>> >> >>> >>> >>> >>> >> this should be the first step, so that various people in the GlassFish
>> >> >>> >>> >>> >>> >> project can eat the dogfood before it can be considered for the
>> >> >>> >>> >>> >>> >> inclusion into Cargo core.
>> >> >>> >>> >>> >>> >
>> >> >>> >>> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
>> >> >>> >>> >>> >>> > before considering contributing to cargo. You can always start with some
>> >> >>> >>> >>> >>> > simple commands and work from there. If needed we can always add
>> >> >>> >>> >>> >>> > documentation about it being a feature preview.
>> >> >>> >>> >>> >>>
>> >> >>> >>> >>> >>> I think the feature still makes sense. Code that works is one reason,
>> >> >>> >>> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
>> >> >>> >>> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
>> >> >>> >>> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
>> >> >>> >>> >>> >>
>> >> >>> >>> >>> >> hehe, yes, I agree here.
>> >> >>> >>> >>> >>
>> >> >>> >>> >>> >>> It
>> >> >>> >>> >>> >>> was really painful for me to make sure every check in satisfies all the
>> >> >>> >>> >>> >>> check style rules.
>> >> >>> >>> >>> >>
>> >> >>> >>> >>> >> Its really not that bad when you use an ide to manage it for you. Right
>> >> >>> >>> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
>> >> >>> >>> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
>> >> >>> >>> >>> >> and tells you what is wrong with them (although I can't seem to figure
>> >> >>> >>> >>> >> out how to tell eclipse where the suppression file is).
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > Yes, I was hoping that at least there are some IDEs that understand
>> >> >>> >>> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
>> >> >>> >>> >>> > available on my IDE, hence the preference to postpone till the end.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
>> >> >>> >>> >>> >>> that work until we integrate that back into the core (and perhaps at
>> >> >>> >>> >>> >>> that point I can ask for other people's help.)
>> >> >>> >>> >>> >>
>> >> >>> >>> >>> >> You can also easily disabled it by passing a specific -D option during a
>> >> >>> >>> >>> >> build.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
>> >> >>> >>> >>> > make a commit, it better passes the checkstyle.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> >> If you are really worried about the checkstyle, we could even
>> >> >>> >>> >>> >> temporarily disable it for the Glassfish part.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > That's good to know :-)
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > But I really think it's better for the Cargo project as a whole it it
>> >> >>> >>> >>> > lets people write containers outside the Cargo project.
>> >> >>> >>> >>> >
>> >> >>> >>> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
>> >> >>> >>> >>> > people to write custom tasks outside the Apache Ant project itself, I
>> >> >>> >>> >>> > don't think it would have been this successful.
>> >> >>> >>> >>> >
>> >> >>> >>> >>>
>> >> >>> >>> >>>
>> >> >>> >>> >>
>> >> >>> >>> >>
>> >> >>> >>> >> ---------------------------------------------------------------------
>> >> >>> >>> >> To unsubscribe from this list, please visit:
>> >> >>> >>> >>
>> >> >>> >>> >>     http://xircles.codehaus.org/manage_email
>> >> >>> >>> >>
>> >> >>> >>> >>
>> >> >>> >>> >>
>> >> >>> >>> >
>> >> >>> >>> >
>> >> >>> >>>
>> >> >>> >>>
>> >> >>> >>
>> >> >>> >>
>> >> >>> >> ---------------------------------------------------------------------
>> >> >>> >> To unsubscribe from this list, please visit:
>> >> >>> >>
>> >> >>> >>     http://xircles.codehaus.org/manage_email
>> >> >>> >>
>> >> >>> >>
>> >> >>> >>
>> >> >>> >
>> >> >>> >
>> >> >>>
>> >> >>>
>> >> >>
>> >> >>
>> >> >> ---------------------------------------------------------------------
>> >> >> To unsubscribe from this list, please visit:
>> >> >>
>> >> >>     http://xircles.codehaus.org/manage_email
>> >> >>
>> >> >>
>> >> >>
>> >> >
>> >> >
>> >>
>> >>
>> >
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe from this list, please visit:
>> >
>> >     http://xircles.codehaus.org/manage_email
>> >
>> >
>> >
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>
>

--
Kohsuke Kawaguchi
Sun Microsystems                   http://weblogs.java.net/blog/kohsuke/

[Matt Wringe]

On Fri, 2008-08-08 at 15:22 -0700, Kohsuke Kawaguchi wrote:
> Argh. I'm sorry, I forgot to commit that file. Should I commit the
> change or are you going to?

you can commit the forgotten file.

>
> Matt Wringe wrote:
> > On Thu, 2008-08-07 at 14:33 -0700, Kohsuke Kawaguchi wrote:
> >> Thank you!
> >
> > I just did some quick testing with it, it mostly works fine for me
> > except you are missing a dependency on commons-discovery.
> >
> >> Matt Wringe wrote:
> >> > On Thu, 2008-08-07 at 10:52 -0700, Kohsuke Kawaguchi wrote:
> >> >> Kohsuke Kawaguchi wrote:
> >> >> > Matt Wringe wrote:
> >> >> >> On Tue, 2008-08-05 at 09:36 -0700, Kohsuke Kawaguchi wrote:
> >> >> >>> Kohsuke Kawaguchi wrote:
> >> >> >>> > Matt Wringe wrote:
> >> >> >>> >> On Thu, 2008-07-24 at 15:00 -0700, Kohsuke Kawaguchi wrote:
> >> >> >>> >>> Any news on this? Should I assume that no news is a good news?
> >> >> >>> >>
> >> >> >>> >> Sorry for no response from me, I looked at it briefly before and havn't
> >> >> >>> >> had a chance to look into into it deeper. From what I saw it looked ok.
> >> >> >>> >
> >> >> >>> > Thanks. I'll proceed and do the same for all the types.
> >> >> >>>
> >> >> >>> ... and here is the patch that does the same thing for all factory types.
> >> >> >>
> >> >> >> Awesome :)
> >> >> >
> >> >> > If I can commit this change, let me know.
> >> >>
> >> >> Sorry for my impatience, but can I proceed?
> >> >
> >> > I see no problems with it, although I have only quickly glanced over the
> >> > code and not actually tested it or anything.
> >> >
> >> > Since no one has objected, I would think it would be ok to proceed.
> >> >
> >> >>
> >> >> >
> >> >> >
> >> >> >>> >>> Kohsuke Kawaguchi wrote:
> >> >> >>> >>> > Matt Wringe wrote:
> >> >> >>> >>> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
> >> >> >>> >>> >>> Does this direction make sense?
> >> >> >>> >>> >>>
> >> >> >>> >>> >>> I guess what I'm trying to avoid here is to spend time implementing this
> >> >> >>> >>> >>> feature, only to learn later that you don't agree with the direction.
> >> >> >>> >>> >>>
> >> >> >>> >>> >> it makes sense to me :)
> >> >> >>> >>> >
> >> >> >>> >>> > Attached is the patch for this change that covers Deployable and
> >> >> >>> >>> > DeployableFactory. If this looks OK, I'll proceed with all the other
> >> >> >>> >>> > factory types.
> >> >> >>> >>> >
> >> >> >>> >>> > See the test code for how the container implementation would look like.
> >> >> >>> >>> >
> >> >> >>> >>> >
> >> >> >>> >>> >>> Kohsuke Kawaguchi wrote:
> >> >> >>> >>> >>> > Matt Wringe wrote:
> >> >> >>> >>> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
> >> >> >>> >>> >>> >>> Matt Wringe wrote:
> >> >> >>> >>> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
> >> >> >>> >>> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
> >> >> >>> >>> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
> >> >> >>> >>> >>> >>> >
> >> >> >>> >>> >>> >>> > Replied to the jira about how cargo already supports custom containers
> >> >> >>> >>> >>> >>> > and configuration.
> >> >> >>> >>> >>> >>>
> >> >> >>> >>> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
> >> >> >>> >>> >>> >>> me is a good feature, but IMHO it's not suffice.
> >> >> >>> >>> >>> >>>
> >> >> >>> >>> >>> >>>
> >> >> >>> >>> >>> >>> >> The implementation approach that I'm thinking of is to have
> >> >> >>> >>> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
> >> >> >>> >>> >>> >>> >> META-INF/services and use them as delegates on methods like
> >> >> >>> >>> >>> >>> >> "getDeployerClass". So the container implementations that would like to
> >> >> >>> >>> >>> >>> >> hook into this would implement their own factories and write
> >> >> >>> >>> >>> >>> >> META-INF/services entry.
> >> >> >>> >>> >>> >>> >
> >> >> >>> >>> >>> >>> > I do not have a lot of experience with jar service providers so I don't
> >> >> >>> >>> >>> >>> > really know how feasible this is.
> >> >> >>> >>> >>> >>>
> >> >> >>> >>> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
> >> >> >>> >>> >>> >> ok :) could you explain a bit more how exactly it would work then?
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > This is a standard convention used widely everywhere, such as in many
> >> >> >>> >>> >>> > JCP APIs.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > The technique is for the use cases where typically one wants to discover
> >> >> >>> >>> >>> > implementations of some contract defined in the API. For example, in
> >> >> >>> >>> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
> >> >> >>> >>> >>> > implementations (like Xerces) without having any prior knowledge about
> >> >> >>> >>> >>> > such an implementation.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > Here is the technique. The base contract is defined in terms of a Java
> >> >> >>> >>> >>> > class/interface. Suppose this contract has the fully-qualified name of
> >> >> >>> >>> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
> >> >> >>> >>> >>> > that like to be discovered will put a file with the name
> >> >> >>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >> >> >>> >>> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
> >> >> >>> >>> >>> > qualified names of classes that implement this base interface/class.
> >> >> >>> >>> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
> >> >> >>> >>> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > The discovery process takes one ClassLoader as a parameter. This
> >> >> >>> >>> >>> > parameter is normally defaulted to
> >> >> >>> >>> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
> >> >> >>> >>> >>> > scenarios the caller do need to be able to pass it in.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > The API then lists up all
> >> >> >>> >>> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >> >> >>> >>> >>> > from resources, parse them all, and list up all the classes. It'll
> >> >> >>> >>> >>> > instantiate them and use them, with suitable error handling in-between.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > commons-discovery defines code to do this. Writing it from scratch is
> >> >> >>> >>> >>> > also rather easy.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > The end result is that all that needed to be done by the user is put
> >> >> >>> >>> >>> > "cargo-glassfish-impl.jar" in the classpath.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >>> > I really don't like the current factory implementation and would ideally
> >> >> >>> >>> >>> >>> > like to see cargo automatically determine the proper classes to use
> >> >> >>> >>> >>> >>> > based on a class naming scheme (and cargo is already using a naming
> >> >> >>> >>> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
> >> >> >>> >>> >>> >>> > implementation for Tomcat6 for a local installed container).
> >> >> >>> >>> >>> >>> >
> >> >> >>> >>> >>> >>> > With the current factory implementation its too easy to make
> >> >> >>> >>> >>> >>> mistakes.
> >> >> >>> >>> >>> >>>
> >> >> >>> >>> >>> >>> I guess the only problem with the naming convention is that all the
> >> >> >>> >>> >>> >>> implementations would have to live in the "org.codehaus" package.
> >> >> >>> >>> >>> >>
> >> >> >>> >>> >>> >> well, it could just look at the class name and not the package name.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > OK, so your idea of pluggability must be bit different than mine. I
> >> >> >>> >>> >>> > suspect your approach is that somehow container implementations would
> >> >> >>> >>> >>> > tell the factory about its class names, and the factory would figure out
> >> >> >>> >>> >>> > the ID and the type from the class name. But this is missing how the
> >> >> >>> >>> >>> > containers tell the factory their class names in the first place,
> >> >> >>> >>> >>> > without asking the users' help --- that is the part that I'm trying to
> >> >> >>> >>> >>> > eliminate.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > What I was thinking was that the factory implementation now only know
> >> >> >>> >>> >>> > about a class name convention, and whenever someone requests an
> >> >> >>> >>> >>> > instantiation from some parameters, it just blindly tries to create a
> >> >> >>> >>> >>> > class of certain names. If the class is there, it works, otherwise, it
> >> >> >>> >>> >>> > won't.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > So in the latter scheme, while you can try more than one class names,
> >> >> >>> >>> >>> > you cannot really allow arbitrary package names or class names, simply
> >> >> >>> >>> >>> > because the factory can't know about them all.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >>> But if the Cargo project is OK for random people to be putting classes
> >> >> >>> >>> >>> >>> in this package,
> >> >> >>> >>> >>> >>
> >> >> >>> >>> >>> >> I don't think it would be ok for people to be using
> >> >> >>> >>> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
> >> >> >>> >>> >>> >> the code origins. Please see my comment above.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > OK. Then that eliminates convention-only approach.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >>>  this is probably better than META-INF/services look up,
> >> >> >>> >>> >>> >>> because the container implementor needs to write less.
> >> >> >>> >>> >>> >>>
> >> >> >>> >>> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
> >> >> >>> >>> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
> >> >> >>> >>> >>> >>> naming convention would be somewhat contrived to make it fit with the
> >> >> >>> >>> >>> >>> current class names.
> >> >> >>> >>> >>> >>
> >> >> >>> >>> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > OK.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >>> >> I feel container developers would have to write more than they should,
> >> >> >>> >>> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
> >> >> >>> >>> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
> >> >> >>> >>> >>> >>> >> I hesitate.
> >> >> >>> >>> >>> >>> >>
> >> >> >>> >>> >>> >>> >>
> >> >> >>> >>> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
> >> >> >>> >>> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
> >> >> >>> >>> >>> >>> >> other factories.
> >> >> >>> >>> >>> >>> >>
> >> >> >>> >>> >>> >>> >>
> >> >> >>> >>> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
> >> >> >>> >>> >>> >>> >
> >> >> >>> >>> >>> >>> > great, we need someone with glassfish knowledge
> >> >> >>> >>> >>> >>> >
> >> >> >>> >>> >>> >>> >>  but I think
> >> >> >>> >>> >>> >>> >> this should be the first step, so that various people in the GlassFish
> >> >> >>> >>> >>> >>> >> project can eat the dogfood before it can be considered for the
> >> >> >>> >>> >>> >>> >> inclusion into Cargo core.
> >> >> >>> >>> >>> >>> >
> >> >> >>> >>> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
> >> >> >>> >>> >>> >>> > before considering contributing to cargo. You can always start with some
> >> >> >>> >>> >>> >>> > simple commands and work from there. If needed we can always add
> >> >> >>> >>> >>> >>> > documentation about it being a feature preview.
> >> >> >>> >>> >>> >>>
> >> >> >>> >>> >>> >>> I think the feature still makes sense. Code that works is one reason,
> >> >> >>> >>> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
> >> >> >>> >>> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
> >> >> >>> >>> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
> >> >> >>> >>> >>> >>
> >> >> >>> >>> >>> >> hehe, yes, I agree here.
> >> >> >>> >>> >>> >>
> >> >> >>> >>> >>> >>> It
> >> >> >>> >>> >>> >>> was really painful for me to make sure every check in satisfies all the
> >> >> >>> >>> >>> >>> check style rules.
> >> >> >>> >>> >>> >>
> >> >> >>> >>> >>> >> Its really not that bad when you use an ide to manage it for you. Right
> >> >> >>> >>> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
> >> >> >>> >>> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
> >> >> >>> >>> >>> >> and tells you what is wrong with them (although I can't seem to figure
> >> >> >>> >>> >>> >> out how to tell eclipse where the suppression file is).
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > Yes, I was hoping that at least there are some IDEs that understand
> >> >> >>> >>> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
> >> >> >>> >>> >>> > available on my IDE, hence the preference to postpone till the end.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
> >> >> >>> >>> >>> >>> that work until we integrate that back into the core (and perhaps at
> >> >> >>> >>> >>> >>> that point I can ask for other people's help.)
> >> >> >>> >>> >>> >>
> >> >> >>> >>> >>> >> You can also easily disabled it by passing a specific -D option during a
> >> >> >>> >>> >>> >> build.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
> >> >> >>> >>> >>> > make a commit, it better passes the checkstyle.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> >> If you are really worried about the checkstyle, we could even
> >> >> >>> >>> >>> >> temporarily disable it for the Glassfish part.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > That's good to know :-)
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > But I really think it's better for the Cargo project as a whole it it
> >> >> >>> >>> >>> > lets people write containers outside the Cargo project.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
> >> >> >>> >>> >>> > people to write custom tasks outside the Apache Ant project itself, I
> >> >> >>> >>> >>> > don't think it would have been this successful.
> >> >> >>> >>> >>> >
> >> >> >>> >>> >>>
> >> >> >>> >>> >>>
> >> >> >>> >>> >>
> >> >> >>> >>> >>
> >> >> >>> >>> >> ---------------------------------------------------------------------
> >> >> >>> >>> >> To unsubscribe from this list, please visit:
> >> >> >>> >>> >>
> >> >> >>> >>> >>     http://xircles.codehaus.org/manage_email
> >> >> >>> >>> >>
> >> >> >>> >>> >>
> >> >> >>> >>> >>
> >> >> >>> >>> >
> >> >> >>> >>> >
> >> >> >>> >>>
> >> >> >>> >>>
> >> >> >>> >>
> >> >> >>> >>
> >> >> >>> >> ---------------------------------------------------------------------
> >> >> >>> >> To unsubscribe from this list, please visit:
> >> >> >>> >>
> >> >> >>> >>     http://xircles.codehaus.org/manage_email
> >> >> >>> >>
> >> >> >>> >>
> >> >> >>> >>
> >> >> >>> >
> >> >> >>> >
> >> >> >>>
> >> >> >>>
> >> >> >>
> >> >> >>
> >> >> >> ---------------------------------------------------------------------
> >> >> >> To unsubscribe from this list, please visit:
> >> >> >>
> >> >> >>     http://xircles.codehaus.org/manage_email
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >
> >> >> >
> >> >>
> >> >>
> >> >
> >> >
> >> > ---------------------------------------------------------------------
> >> > To unsubscribe from this list, please visit:
> >> >
> >> >     http://xircles.codehaus.org/manage_email
> >> >
> >> >
> >> >
> >>
> >>
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe from this list, please visit:
> >
> >     http://xircles.codehaus.org/manage_email
> >
> >
> >
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email