geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leonardo Uribe <lu4...@gmail.com>
Subject Re: ordering tck failures in geronimo
Date Tue, 14 Dec 2010 03:24:29 GMT
Hi

Good to know that, thanks. In these moments I'm running the necessary steps
for send the vote, so I hope to release these artifacts this week.

regards,

Leonardo Uribe

2010/12/13 Ivan <xhhsld@gmail.com>

> Thanks, Jakob and Leonardo, the new changes work perfect ;-)
> By the way, I have run the TCK on my local environment, although there are
> some failure cases, they should be caused by webbeans integration.
>
> 2010/12/11 Jakob Korherr <jakob.korherr@gmail.com>
>
> Hi guys,
>>
>> As discussed, I just committed a modified version of the proposed patch.
>>
>> Furthermore I added the custom SPI impl that I used for testing to the
>> issue [1] which may help for the real Geronimo impl.
>>
>> It would be great if you guys could check if this solution works for
>> you, and if so, we can close this issue and start the 2.0.3 release!
>>
>> Regards,
>> Jakob
>>
>> [1]
>> https://issues.apache.org/jira/secure/attachment/12466010/GeronimoFacesConfigurationMerger.java
>>
>> 2010/12/10 Jakob Korherr <jakob.korherr@gmail.com>:
>> > Hi Leo,
>> >
>> > OK great. That's fine with me.
>> >
>> > I will apply the appropriate changes and commit the patch!
>> >
>> > Regards,
>> > Jakob
>> >
>> > 2010/12/10 Leonardo Uribe <lu4242@gmail.com>:
>> >> Hi Jakob
>> >>
>> >> I think it is better do not include it as parameter and instead add
>> some
>> >> comment on the javadoc,
>> >> saying the information to take into account is retrieved from
>> >> FacesConfigurationProvider. The fact
>> >> of use FacesConfigurationProvider is more an implementation detail than
>> a
>> >> requeriment, so if
>> >> somebody wants to do not use it is valid usage.
>> >>
>> >> regards,
>> >>
>> >> Leonardo Uribe
>> >>
>> >> 2010/12/10 Jakob Korherr <jakob.korherr@gmail.com>
>> >>>
>> >>> Hi,
>> >>>
>> >>> OK, great! I added another comment to MYFACES-2945 explaining why I
>> >>> did it this way.
>> >>>
>> >>> Please tell me your final opinion after reading this comment and I'll
>> >>> commit an appropriate version of the proposed patch!
>> >>>
>> >>> Regards,
>> >>> Jakob
>> >>>
>> >>> 2010/12/9 Leonardo Uribe <lu4242@gmail.com>:
>> >>> > Hi
>> >>> >
>> >>> > I think it is not necessary to pass FacesConfigurationProvider
as
>> param
>> >>> > for
>> >>> > getFacesConfigData, because in theory we don't do anything with
it
>> >>> > before
>> >>> > pass it and wrappers will not do anything with it later. I think
it
>> >>> > looks
>> >>> > good to load it using
>> >>> >
>> >>> >
>> FacesConfigurationProviderFactory.getFacesConfigurationProvider(ExternalContext).
>> >>> >
>> >>> > The other parts of the patch looks good.
>> >>> >
>> >>> > regards,
>> >>> >
>> >>> > Leonardo Uribe
>> >>> >
>> >>> > 2010/12/9 Jakob Korherr <jakob.korherr@gmail.com>
>> >>> >>
>> >>> >> Hi guys,
>> >>> >>
>> >>> >> I just uploaded a patch for a FacesConfigurationMerger SPI:
>> >>> >> MYFACES-2945-FacesConfigurationMerger.patch
>> >>> >>
>> >>> >> Furthermore I added a quick code sample as comment on the
>> MYFACES-2945
>> >>> >> issue about how Geronimo can use this new SPI.
>> >>> >>
>> >>> >> Please take a look at the patch and if there are no objections,
I
>> will
>> >>> >> commit it soon!
>> >>> >>
>> >>> >> Regards,
>> >>> >> Jakob
>> >>> >>
>> >>> >> 2010/12/9 Jakob Korherr <jakob.korherr@gmail.com>:
>> >>> >> > Hi,
>> >>> >> >
>> >>> >> > I called it ugly, because of its implementation code in
>> >>> >> > DefaultFacesConfigurationProvider: The method is already
inside
>> of a
>> >>> >> > FacesConfigurationProvider, but it does this:
>> >>> >> >
>> >>> >> > FacesConfigurationProvider provider =
>> >>> >> > FacesConfigurationProviderFactory.
>> >>> >> >
>>  getFacesConfigurationProviderFactory(_externalContext).
>> >>> >> >                getFacesConfigurationProvider(_externalContext);
>> >>> >> >
>> >>> >> > ...and then calls all the other methods of
>> FacesConfigurationProvider
>> >>> >> > on this provider instance.
>> >>> >> >
>> >>> >> > As I said, I know this is this way, because
>> >>> >> > FacesConfigurationProvider
>> >>> >> > can be wrapped, but IMHO it is really ugly.
>> >>> >> >
>> >>> >> >
>> >>> >> > The method used on MYFACES-2998 was a first approach to
solve
>> this
>> >>> >> > problem in a better way. However, I really like those
two of your
>> >>> >> > suggestions:
>> >>> >> >
>> >>> >> > 1) Leo #2: Create another SPI interface for getFacesConfigData()
>> >>> >> > (please suggest a name for it, maybe
>> >>> >> > FacesConfigurationMergerProvider?) and remove this method
form
>> >>> >> > FacesConfigurationProvider.
>> >>> >> > 2) Ivan: In a few words: let getFacesConfigData() on
>> >>> >> > FacesConfigurationProvider, but provide an
>> >>> >> > AbstractFacesConfigurationProvider which implements the
merging
>> and
>> >>> >> > sorting to let custom SPI impls take advantage of it.
>> >>> >> >
>> >>> >> > At first, I really liked Ivan's proposal, but after thinking
more
>> >>> >> > about it, it is not consistent with what we have right
now (we do
>> not
>> >>> >> > provide any other Abstract-SPI impl) and we would have
the huge
>> >>> >> > merging and sorting code all in the SPI(-api) package,
but IMO it
>> >>> >> > should really go into o.a.m.config.
>> >>> >> >
>> >>> >> > Thus I think that a new FacesConfigurationMergerProvider-SPI
as
>> Leo
>> >>> >> > proposed is the best choice here. Note that for this SPI
it is
>> good
>> >>> >> > practice to use other SPI impls.
>> >>> >> >
>> >>> >> > I will provide a patch for it soon and then we can discuss
it
>> >>> >> > further!
>> >>> >> >
>> >>> >> > Regards,
>> >>> >> > Jakob
>> >>> >> >
>> >>> >> > 2010/12/9 Ivan <xhhsld@gmail.com>:
>> >>> >> >> my 2 cents, it seems for me less urgly ...
>> >>> >> >> a. For the FacesConfigurationProvider , it is better
to have
>> only
>> >>> >> >> one
>> >>> >> >> method
>> >>> >> >> getFacesConfigData()
>> >>> >> >> b. Create another abstract class
>> AbstractFacesConfigurationProvider
>> >>> >> >> which
>> >>> >> >> extends FacesConfigurationProvider, and define those
proctected
>> >>> >> >> methods
>> >>> >> >> of
>> >>> >> >> get***, also place those sorting/merging codes there.
>> >>> >> >> c. In the DefaultFacesConfigurationProvider, it only
implements
>> >>> >> >> those
>> >>> >> >> get***
>> >>> >> >> methods.
>> >>> >> >> thanks.
>> >>> >> >>
>> >>> >> >> 2010/12/9 Leonardo Uribe <lu4242@gmail.com>
>> >>> >> >>>
>> >>> >> >>> Hi
>> >>> >> >>>
>> >>> >> >>> I agree with Jakob about faces-config merging
and ordering
>> >>> >> >>> algorithm
>> >>> >> >>> should not be exposed by MyFaces. Why is it not
enough?. At
>> this
>> >>> >> >>> point
>> >>> >> >>> it is
>> >>> >> >>> not clear the reasons. Note in this moment ordering
and sorting
>> >>> >> >>> algoritm it
>> >>> >> >>> is not being exposed by FacesConfigurationProvider
interface.
>> >>> >> >>>
>> >>> >> >>> Other different thing is
>> >>> >> >>> FacesConfigurationProvider.getFacesConfigData().
>> >>> >> >>> The intention of this method is provide a way
to get a
>> Serializable
>> >>> >> >>> object
>> >>> >> >>> that represents all config information required
to initialize
>> >>> >> >>> MyFaces
>> >>> >> >>> and
>> >>> >> >>> allow container to store it temporally somewere.
In this way it
>> is
>> >>> >> >>> possible
>> >>> >> >>> to deploy and undeploy an application more quickly,
because if
>> >>> >> >>> "nothing
>> >>> >> >>> changes"(no added dependencies, no update from
faces-config.xml
>> >>> >> >>> files
>> >>> >> >>> or
>> >>> >> >>> classes) this information is always the same.
>> >>> >> >>>
>> >>> >> >>> On MYFACES-2945 and previous discussions it was
proposed two
>> >>> >> >>> different
>> >>> >> >>> options:
>> >>> >> >>>
>> >>> >> >>> 1. Add getFacesConfigData()
>> >>> >> >>> 2. Add a set of methods to retrieve FacesConfig
objects
>> instead.
>> >>> >> >>>
>> >>> >> >>>     public abstract FacesConfig
>> >>> >> >>> getStandardFacesConfig(ExternalContext
>> >>> >> >>> ectx);
>> >>> >> >>>     public abstract FacesConfig
>> >>> >> >>> getMetaInfServicesFacesConfig(ExternalContext
ectx);
>> >>> >> >>>     public abstract FacesConfig
>> >>> >> >>> getAnnotationsFacesConfig(ExternalContext
>> >>> >> >>> ectx, boolean metadataComplete);
>> >>> >> >>>     public abstract List<FacesConfig>
>> >>> >> >>> getClassloaderFacesConfig(ExternalContext ectx);
>> >>> >> >>>     public abstract List<FacesConfig>
>> >>> >> >>> getContextSpecifiedFacesConfig(ExternalContext
ectx);
>> >>> >> >>>     public abstract FacesConfig
>> >>> >> >>> getWebAppFacesConfig(ExternalContext
>> >>> >> >>> ectx);
>> >>> >> >>>
>> >>> >> >>> The first option has the advantage that it fill
the initial
>> >>> >> >>> requeriment
>> >>> >> >>> without add more complexity to the problem. The
second one
>> seems to
>> >>> >> >>> be
>> >>> >> >>> more
>> >>> >> >>> structured and opens the possibility to do other
things like
>> how
>> >>> >> >>> override
>> >>> >> >>> MyFaces parsing for faces-config.xml files like
it is being
>> >>> >> >>> discussed.
>> >>> >> >>> If
>> >>> >> >>> the container parse faces-config.xml files, with
the previous
>> >>> >> >>> methods
>> >>> >> >>> it is
>> >>> >> >>> possible to prevent parse the same files twice.
>> >>> >> >>>
>> >>> >> >>> My first intention, as is shown on MYFACES-2945
was that
>> >>> >> >>> FacesConfigurationProvider does not included
>> getFacesConfigData(),
>> >>> >> >>> because
>> >>> >> >>> it is possible to fill the initial objective with
these
>> methods,
>> >>> >> >>> but
>> >>> >> >>> finally
>> >>> >> >>> it was agreed the first option looks better, right?
>> >>> >> >>>
>> >>> >> >>> Now I see that on MYFACES-2998 that fact is questioned:
>> >>> >> >>>
>> >>> >> >>> JK>> Unfortunately it also provides a method
that should
>> combine
>> >>> >> >>> all
>> >>> >> >>> these
>> >>> >> >>> data: getFacesConfigData().
>> >>> >> >>> JK>> This method is here due to refactorings,
but IMHO this is
>> the
>> >>> >> >>> last
>> >>> >> >>> place where it should be put,
>> >>> >> >>> JK>> because it gets "its own implementation"
via its Factory
>> and
>> >>> >> >>> then
>> >>> >> >>> calls all of the above methods on
>> >>> >> >>> JK>> it. I know this was introduced to support
wrapping the
>> default
>> >>> >> >>> impl,
>> >>> >> >>> but it really is very, very ugly.
>> >>> >> >>>
>> >>> >> >>> In few words, why does it looks ugly? or with
other words, what
>> can
>> >>> >> >>> we
>> >>> >> >>> do
>> >>> >> >>> to make it cleaner? remove it? or just provide
another SPI
>> >>> >> >>> interface
>> >>> >> >>> and put
>> >>> >> >>> that method there? In practice, getFacesConfigData()
merges all
>> >>> >> >>> FacesConfig
>> >>> >> >>> information, and "on the way" it does order
>> applicationFacesConfig
>> >>> >> >>> files
>> >>> >> >>> (the ones obtained from getClassloaderFacesConfig()
and
>> >>> >> >>> getContextSpecifiedFacesConfig() ) . To do that
it requires to
>> call
>> >>> >> >>> all six
>> >>> >> >>> methods from FacesConfigurationProvider, there
is no other way,
>> so
>> >>> >> >>> I
>> >>> >> >>> don't
>> >>> >> >>> see why do that is considered ugly.
>> >>> >> >>>
>> >>> >> >>> At this moment we have the following courses of
action:
>> >>> >> >>>
>> >>> >> >>> 1. Remove FacesConfigurationResource interface
partially,
>> because
>> >>> >> >>> it
>> >>> >> >>> is
>> >>> >> >>> still too inmature and let it for myfaces core
2.0.4.
>> >>> >> >>> 2. Create another SPI interface for getFacesConfigData()
>> (please
>> >>> >> >>> suggest a
>> >>> >> >>> name for it, maybe FacesConfigurationMergerProvider?)
and
>> remove
>> >>> >> >>> this
>> >>> >> >>> method
>> >>> >> >>> form FacesConfigurationResource. Apply the patch
on
>> MYFACES-2998
>> >>> >> >>> seems
>> >>> >> >>> to be
>> >>> >> >>> in this direction, but forget the reason why it
is wanted to
>> expose
>> >>> >> >>> getFacesConfigData() to the container.
>> >>> >> >>> 3. Apply something like MYFACES-2998 patch, and
refactor this
>> one
>> >>> >> >>> later in
>> >>> >> >>> myfaces core 2.0.4
>> >>> >> >>>
>> >>> >> >>> Suggestions are welcome.
>> >>> >> >>>
>> >>> >> >>> regards,
>> >>> >> >>>
>> >>> >> >>> Leonardo Uribe
>> >>> >> >>>
>> >>> >> >>>
>> >>> >> >>>
>> >>> >> >>>
>> >>> >> >>
>> >>> >> >>
>> >>> >> >>
>> >>> >> >> --
>> >>> >> >> Ivan
>> >>> >> >>
>> >>> >> >
>> >>> >> >
>> >>> >> >
>> >>> >> > --
>> >>> >> > Jakob Korherr
>> >>> >> >
>> >>> >> > blog: http://www.jakobk.com
>> >>> >> > twitter: http://twitter.com/jakobkorherr
>> >>> >> > work: http://www.irian.at
>> >>> >> >
>> >>> >>
>> >>> >>
>> >>> >>
>> >>> >> --
>> >>> >> Jakob Korherr
>> >>> >>
>> >>> >> blog: http://www.jakobk.com
>> >>> >> twitter: http://twitter.com/jakobkorherr
>> >>> >> work: http://www.irian.at
>> >>> >
>> >>> >
>> >>>
>> >>>
>> >>>
>> >>> --
>> >>> Jakob Korherr
>> >>>
>> >>> blog: http://www.jakobk.com
>> >>> twitter: http://twitter.com/jakobkorherr
>> >>> work: http://www.irian.at
>> >>
>> >>
>> >
>> >
>> >
>> > --
>> > Jakob Korherr
>> >
>> > blog: http://www.jakobk.com
>> > twitter: http://twitter.com/jakobkorherr
>> > work: http://www.irian.at
>> >
>>
>>
>>
>> --
>> Jakob Korherr
>>
>> blog: http://www.jakobk.com
>> twitter: http://twitter.com/jakobkorherr
>> work: http://www.irian.at
>>
>
>
>
> --
> Ivan
>

Mime
View raw message