Return-Path: Delivered-To: apmail-geronimo-dev-archive@www.apache.org Received: (qmail 49041 invoked from network); 8 Dec 2010 13:30:53 -0000 Received: from unknown (HELO mail.apache.org) (140.211.11.3) by 140.211.11.9 with SMTP; 8 Dec 2010 13:30:53 -0000 Received: (qmail 2052 invoked by uid 500); 8 Dec 2010 13:30:53 -0000 Delivered-To: apmail-geronimo-dev-archive@geronimo.apache.org Received: (qmail 1928 invoked by uid 500); 8 Dec 2010 13:30:53 -0000 Mailing-List: contact dev-help@geronimo.apache.org; run by ezmlm Precedence: bulk list-help: list-unsubscribe: List-Post: Reply-To: dev@geronimo.apache.org List-Id: Delivered-To: mailing list dev@geronimo.apache.org Received: (qmail 1916 invoked by uid 99); 8 Dec 2010 13:30:52 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 08 Dec 2010 13:30:52 +0000 X-ASF-Spam-Status: No, hits=-0.7 required=10.0 tests=FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of sethfromaustria@gmail.com designates 74.125.82.50 as permitted sender) Received: from [74.125.82.50] (HELO mail-ww0-f50.google.com) (74.125.82.50) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 08 Dec 2010 13:30:48 +0000 Received: by wwf26 with SMTP id 26so1132222wwf.31 for ; Wed, 08 Dec 2010 05:30:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received :in-reply-to:references:date:x-google-sender-auth:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=eOflXLyWPgNozDvN73qHF4KPV/IXmK9VQ6aauWCtf8Y=; b=oTKbGHS4nE6m+jN/Oczz92KK+yeLGej9/qm5wzThzYIXsn0M6g/bVBaYOQXmtzdDAV x5UDDKGNGjgQZhXRYZCcfCENLRhqKwa8uJS98mGo14vAPgPERwzjHrH6X/5iPWMkUuWL 2R/xKcuvq9odyxlkFX5oy5mTLuY/giCTLws6U= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=dKfgska8k4/yYPLfICycFEJk7sFQxQsHvM5yi8dhkWpgm1aPhzRCMVeUdQ6xEF6vgJ aapanNthjCmE3K/9gmtSXFnSxdroog88gZHpTjDH8qpju74RXNEsSsj66qb/BOGCb/H8 lfH8LHxfR9CTtRh4tIJkzcQmQ1997ijWc6aC4= MIME-Version: 1.0 Received: by 10.216.24.132 with SMTP id x4mr1806784wex.81.1291815026680; Wed, 08 Dec 2010 05:30:26 -0800 (PST) Sender: sethfromaustria@gmail.com Received: by 10.216.53.78 with HTTP; Wed, 8 Dec 2010 05:30:26 -0800 (PST) In-Reply-To: References: <0E901BAB-10FB-4263-ACBE-6247F61F41AF@yahoo.com> <2B5C75DA-830D-4343-8DC9-F9DDE57A1AAA@yahoo.com> <9E004281-2991-406F-82A1-D3497681E366@yahoo.com> Date: Wed, 8 Dec 2010 14:30:26 +0100 X-Google-Sender-Auth: g4XXs9HRqEO-gGGMslxho4m42wo Message-ID: Subject: Re: ordering tck failures in geronimo From: Jakob Korherr To: MyFaces Development Cc: dev@geronimo.apache.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi guys, I really see no reason why Geronimo should implement its own faces-config merging and ordering algorithm or even have to think about relying on MyFaces internals. IMHO getFacesConfigData() should be removed from FacesConfigurationProvider, because this algorithm does not have to be customizable and thus should be removed from the SPI. I created MYFACES-2998 with a more detailed description and a patch for this problem. Please review it and if there are no objections, I will commit the patch soon. I hope this will make your lifes a lot easier! Regards, Jakob 2010/12/8 Ivan : > Thanks for tacking this, Leonardo. > I am not sure I understand the changes. Moving methods to the > FacesConfigurationProvider is required, but it seems to me that it is not > enough. With this spi, do I still need to provide those sorting/merging/.= .. > methods ? > The only way I could see now is that, a. extend the wrapper class b. Use > FacesConfigurationProviderFactory to get an instance of > FacesConfigurationProvider. But that seems a little strange to me ... > > I also moved this thread from geronimo-tck to dev-geronimo, as current > discussion is not related to tck, and no related message in current email= . > thanks. > > 2010/12/8 Leonardo Uribe >> >> Hi >> >> 2010/12/7 David Jencks >>> >>> On Dec 7, 2010, at 10:12 AM, Leonardo Uribe wrote: >>> >>> Hi >>> >>> 2010/12/6 Ivan >>> >>>> =C2=A0 =C2=A0 So, at least could you please help to add the export the >>>> org.apache.myfaces.config package in 2.0.3, that will make my life eas= ier >>>> now. And remove it once those methods are moved to >>>> FacesConfigurationProvider in the future. >>> >>> >>> Instead do that, it is better to add them now. (I committed it on >>> MYFACES-2945) It is a change already studied, so I don't see any proble= m >>> doing it. I did some small changes too, so it could be good if you try = it to >>> see if everything is ok (I'll wait for your response) . >>> >>> 2010/12/7 David Jencks >>>> >>>> >>>> I have not compared the myfaces classes with the openejb jaxb tree for >>>> faces-config.xml, so I have no idea how plausible this idea might be. = =C2=A0Would >>>> it be possible to annotate the myfaces object tree representing >>>> faces-config.xml so that it could be read in by jaxb as well as by dig= ester? >>>> For use in geronimo, I wonder if putting some or all of this code in a >>>> fragment bundle hosted by the myfaces bundle would reduce the number o= f >>>> exports needed. >>> >>> I don't think so, because do that requires to add a dependency to jaxb = on >>> myfaces impl, and we have one xml library dependency already >>> (commons-digester). >>> >>> Classes containing annotations that aren't available at runtime should >>> still load fine, so this would be a compile time dependency but optiona= l at >>> runtime. =C2=A0It would possibly make the commons-digester dependency o= ptional at >>> runtime as well (at least with non-myfaces additional code that uses ja= xb) >> >> I have never tried, but what I understand is only annotations with >> @Retention(RetentionPolicy.SOURCE) are discarded by the compiler. But if= it >> is possible to add jaxb as an optional dependency, yes, myfaces could >> include those annotations. Anyway in this moment it is just a possibilit= y, >> so I will not believe it until I see it. Note JSF 2.0 is JDK 1.5 compati= ble. >> That means we should take care on keep MyFaces working in 1.5. >> >> regards, >> >> Leonardo Uribe >> >>> >>> thanks >>> david jencks >>> >>> regards, >>> >>> Leonardo URibe >>> >>>> >>>> thanks >>>> david jencks >>>> >>>> For item b, I created a sub classes of >>>> DefaultFacesConfigurationProvider, and just overrides those get*** met= hods, >>>> the class is look like : >>>> ---> >>>> public class GeronimoFacesConfigurationProvider extends >>>> DefaultFacesConfigurationProvider { >>>> >>>> =C2=A0=C2=A0=C2=A0 private FacesConfig annotationsFacesConfig; >>>> >>>> =C2=A0=C2=A0=C2=A0 private List classloaderFacesConfigs; >>>> >>>> =C2=A0=C2=A0=C2=A0 private List contextSpecifiedFacesConf= igs; >>>> >>>> =C2=A0=C2=A0=C2=A0 private FacesConfig webAppFacesConfig; >>>> >>>> =C2=A0=C2=A0=C2=A0 private FacesConfig standardFacesConfig; >>>> >>>> =C2=A0=C2=A0=C2=A0 public GeronimoFacesConfigurationProvider(FacesConf= ig >>>> standardFacesConfig, FacesConfig webAppFacesConfig, FacesConfig >>>> annotationsFacesConfig, List classloaderFacesConfigs, >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Lis= t contextSpecifiedFacesConfigs) { >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 this.annotationsFacesConfig= =3D annotationsFacesConfig; >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 this.classloaderFacesConfig= s =3D classloaderFacesConfigs; >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 this.contextSpecifiedFacesC= onfigs =3D >>>> contextSpecifiedFacesConfigs; >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 this.webAppFacesConfig =3D = webAppFacesConfig; >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 this.standardFacesConfig = =3D standardFacesConfig; >>>> =C2=A0=C2=A0=C2=A0 } >>>> >>>> =C2=A0=C2=A0=C2=A0 @Override >>>> =C2=A0=C2=A0=C2=A0 protected FacesConfig getAnnotationsFacesConfig(Ext= ernalContext >>>> ectx, boolean metadataComplete) { >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return annotationsFacesConf= ig; >>>> =C2=A0=C2=A0=C2=A0 } >>>> >>>> =C2=A0=C2=A0=C2=A0 @Override >>>> =C2=A0=C2=A0=C2=A0 protected List >>>> getClassloaderFacesConfig(ExternalContext externalContext) { >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return classloaderFacesConf= igs; >>>> =C2=A0=C2=A0=C2=A0 } >>>> >>>> =C2=A0=C2=A0=C2=A0 @Override >>>> =C2=A0=C2=A0=C2=A0 protected List >>>> getContextSpecifiedFacesConfig(ExternalContext externalContext) { >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return contextSpecifiedFace= sConfigs; >>>> =C2=A0=C2=A0=C2=A0 } >>>> >>>> =C2=A0=C2=A0=C2=A0 @Override >>>> =C2=A0=C2=A0=C2=A0 protected FacesConfig getStandardFacesConfig(Extern= alContext >>>> externalContext) { >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return standardFacesConfig; >>>> =C2=A0=C2=A0=C2=A0 } >>>> >>>> =C2=A0=C2=A0=C2=A0 @Override >>>> =C2=A0=C2=A0=C2=A0 protected FacesConfig getWebAppFacesConfig(External= Context >>>> externalContext) { >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return webAppFacesConfig; >>>> =C2=A0=C2=A0=C2=A0 } >>>> >>>> } >>>> <--- >>>> Thoughts ? >>>> thanks. >>>> >>>> 2010/12/7 Leonardo Uribe >>>>> >>>>> Hi >>>>> >>>>> First of all, I want to note that if the default algorithm for >>>>> FacesConfigResourceProvider is not able to find a.faces-config.xml an= d >>>>> c.faces-config.xml, that means the Thread Context Class Loader needs = to be >>>>> fixed, because is not taking into account jar files under WEB-INF/lib= . >>>>> >>>>> 2010/12/6 Ivan >>>>>> >>>>>> >>>>>> 2010/12/6 David Jencks >>>>>>> >>>>>>> On Dec 5, 2010, at 7:44 PM, Ivan wrote: >>>>>>> >>>>>>> > I am wondering whether the myfaces bundle could also export the >>>>>>> > spi.impl package, I hope to take advantage of some codes there, e= .g. >>>>>>> > DefaultFacesConfigurationProvider, it contains some sort algorith= m. >>>>>>> > thanks. >>>>>>> > >>>>>>> >>>>> >>>>> Why export spi.impl package? the idea to have an spi api and impl is >>>>> allow impl package to change and let api stable to prevent break code= later, >>>>> right? If something should be exposed from DefaultFacesConfigurationP= rovider >>>>> (for example adding some methods on FacesConfigurationProvider), it s= hould >>>>> be discussed first. >>>>> >>>>> I agree to expose this two: >>>>> >>>>> + >>>>> org.apache.myfaces.config.impl.digester.elements;version=3D"${project= .version}", >>>>> + >>>>> org.apache.myfaces.config.impl.digester;version=3D"${project.version} >>>>> because theorically the code there will not change (only between >>>>> different versions of JSF), but the other ones: >>>>> >>>>> + >>>>> org.apache.myfaces.spi.impl;version=3D"${project.version}", >>>>> + >>>>> org.apache.myfaces.config;version=3D"${project.version}", >>>>> >>>>> needs some argumentation first. >>>>> >>>>>>> >>>>>>> I'd say if you need to expose the default implementation of the spi >>>>>>> then there is something wrong with the interface design. =C2=A0If t= he sorting is >>>>>>> universal then perhaps it should not be in a class implemented by a= service >>>>>>> provider? >>>>>> >>>>> >>>>> Ok, as long as I undersand there is no reason why expose sorting >>>>> algorithm to the container. Also, allow a different parser for FacesC= onfig >>>>> was discussed before.=C2=A0I remember on MYFACES-2945 that it was pro= posed an >>>>> interface with this methods: >>>>> >>>>> public abstract class FacesConfigurationProvider >>>>> { >>>>> =C2=A0=C2=A0=C2=A0=C2=A0public abstract FacesConfig getStandardFacesC= onfig(ExternalContext >>>>> ectx); >>>>> >>>>> =C2=A0=C2=A0=C2=A0=C2=A0public abstract FacesConfig >>>>> getMetaInfServicesFacesConfig(ExternalContext ectx); >>>>> >>>>> =C2=A0=C2=A0=C2=A0=C2=A0public abstract FacesConfig >>>>> getAnnotationsFacesConfig(ExternalContext ectx, boolean metadataCompl= ete); >>>>> >>>>> =C2=A0=C2=A0=C2=A0=C2=A0public abstract List >>>>> getClassloaderFacesConfig(ExternalContext ectx); >>>>> >>>>> =C2=A0=C2=A0=C2=A0=C2=A0public abstract List >>>>> getContextSpecifiedFacesConfig(ExternalContext ectx); >>>>> >>>>> =C2=A0=C2=A0=C2=A0=C2=A0public abstract FacesConfig getWebAppFacesCon= fig(ExternalContext >>>>> ectx); >>>>> >>>>> } >>>>> >>>>> But finally the final form was this: >>>>> >>>>> public abstract class FacesConfigurationProvider >>>>> { >>>>> >>>>> =C2=A0=C2=A0=C2=A0=C2=A0public abstract FacesConfigData getFacesConfi= gData(ExternalContext >>>>> ectx); >>>>> >>>>> } >>>>> >>>>> And this comment was added: >>>>> >>>>> "...After some attempts, the structure of the solution is not too >>>>> different from the previous patch, because after all in >>>>> DefaultFacesConfigurationProvider it is necessary to put all previous= code >>>>> plus ordering and feeding of FacesConfig files...." >>>>> >>>>> Note the first form allows to define an custom parser, because to >>>>> retrieve FacesConfig you should locate them first and then parse them= , but >>>>> the second one does not. >>>>> >>>>>> =C2=A0=C2=A0 Yes, in Geronimo, we have a sepearted algorthim for sor= ting. >>>>>> Currently, I still use the implementation from MyFaces to do it, may= be in >>>>>> the future I could move them to Geronimo side or it will be abstract= ed from >>>>>> current default spi provider. I also need to provide a mock external= context >>>>>> to make it work. or I might need to copy some codes from myfaces. >>>>>> =C2=A0 Another thing is that, I use the digester xml parser from MyF= aces to >>>>>> parse all the faces configuration files, while we using jaxb tree in= the >>>>>> past. This could be avoid, just did not want to add some codes to co= nvert >>>>>> two FacesConfig objects. >>>>> >>>>> The idea behind refactor org.apache.myfaces.config.element package is >>>>> give some base classes from myfaces, to allow use a different parser.= Those >>>>> changes were committed, but to allow that it is still necessary to co= mmit >>>>> some methods on FacesConfigurationProvider to do the "bridge". >>>>> >>>>> regards, >>>>> >>>>> Leonardo Uribe >>>>> >>>>>> =C2=A0 Thanks. >>>>>> >>>>>>> >>>>>>> thanks >>>>>>> david jencks >>>>>>> >>>>>>> > >>>>>>> > -- >>>>>>> > Ivan >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Ivan >>>>> >>>> >>>> >>>> >>>> -- >>>> Ivan >>>> >>> >>> >> > > > > -- > Ivan > --=20 Jakob Korherr blog: http://www.jakobk.com twitter: http://twitter.com/jakobkorherr work: http://www.irian.at