OK, Round #2 Jeff--I yielded on 50% of the issues:
Am Donnerstag, den 16.08.2007, 10:45 +0800 schrieb Jeff.Yu:
> Hi, Glen
>
> Thanks for your comment, see my comments inline.
>
> Thanks
> Jeff
>
>
> Glen Mazza wrote:
> > Hello Jeff,
> >
> > Here's my comments on each patch.
> >
> > Patch #901:
> > 1.)
> >
> > + <target name="copy-war-libs" unless="without.libs">
> > + <copy todir="${war-lib}">
> > + <fileset dir="${cxf.home}/modules">
> > + <include name="cxf-*.jar" />
> > + </fileset>
> > + <fileset dir="${cxf.home}/lib">
> > + <exclude name="servlet-api-*.jar" />
> > + <exclude name="geronimo-servlet_*.jar" />
> > + <exclude name="cxf-*.jar" />
> > + </fileset>
> > + </copy>
> > </target>
> >
> > I think the listing of jar files needed should be explicitly listed via
> > <includes/> (even if there are 40 of them--remember this is only done
> > once in the common_build.xml), not *all* included but just having a few
> > left out via <excludes/>. This also will provide us a crisp,
> > authoritative list of precisely what is needed to run CXF. Using
> > excludes over time might result in unneeded jars getting into the WAR
> > files (i.e., we add a JAR file to the /lib directory but forget to
> > exclude it here.)
> >
> Well, I don't think we need to list them, because in fact we include *all*
> the jars. Allowing me explain a little bit on this.
> Currently, we have two flavors CXF jar, one is all-in-one jar, called
> cxf-2.1-incubator.jar; the other is module-based jar, which is listed in the
> "${cxf.home}/modules",
> for the WARs, we used the module-based jars, so we need to exclude the
> all-in-one jar.
> The reason why I excluded those two servlet jars, because we deploy it into a
> servlet-container, there is no need to have these two servlet jars.
>
Hmmm, in the lib directory there is a "needed jars" file, which
specifies a much smaller subset of all the jars in lib.
The way I see it, is as follows, is you just need something like this:
<war>
...
<lib dir="${lib.dir}">
<include name="abc.jar"/>
<include name="bcd.jar"/>
...
</lib>
...
</war>
For the time being, best to just include the jars needed by *any* of the
samples. Later, perhaps, we can think of a cleaner design where we can
bring in JARs required of *all* samples, plus a few extra
sample-dependent jars, depending on the sample.
> > 2.)
> >
> > Deploy the application into APACHE TOMCAT with the commond:
> > + ant deploy-tomcat
> >
> > Undeploy the application from the APACHE TOMCAT with the command:
> > + ant undeploy-tomcat
> >
> >
> > Can the instructions on tomcat-deploy and tomcat-undeploy be moved to
> > the common samples README without too much loss in user understanding,
> > instead of being copied in every sample application's instructions?
> > That would greatly simplify maintenance. (BTW, it's nice that you got
> > rid of that redundant -Dtomcat = true option)
> >
> I think it is better for us to keep it in those specific samples, first of
> all, not all samples have this "running demo in a servlet container", only 7
> or 8 samples,
> and for these samples, since it has this specific section, it would be better
> for us to keep it in this section.
>
It is not necessary for information in the common parent README.txt to
be applicable for *every* sample--just as long as the text in the parent
README indicates that.
All you need to do is say in the "child" READMEs is something like:
"This sample supports Tomcat deployment. Please see the readme file in
the sample root directory for more information."
And in the parent README, something like:
"Several of the samples support Tomcat deployment--please see the README
file located with the sample to see if it is supported. For those
samples supporting Tomcat deployment, you can deploy the application
with the following command:
> > + ant deploy-tomcat
> >
and undeploy with this command:
> > + ant undeploy-tomcat
>
Later, as we add more information to this, it will just need to go in
this one common place. And if we have to fix misspellings (see your
"commond" above with ant deploy-tomcat, for example), we fix them in one
place.
> > Patch #902:
> > 3.) Is the idea of supporting Java-first web services *without* adding
> > annotations[1] supported by the JAX-WS standard? I believe this is what
> > this sample is about, correct? I'm not certain why we should be
> > encouraging this style of coding--it doesn't seem very rigorous to
> > program web services this way.
> >
> > This is mainly a philosophical concern, not a comment on the code as a
> > whole.
> >
> > [1] http://cwiki.apache.org/CXF20DOC/simple-frontend.html
> >
> Yes, you are right, it doesn't have the annotation (JSR-181), it is another
> front-end (POJO based) other than JAX-WS front-end.
> we are not encouraging it, but we need to have a sample for the completeness,
> we will leave the decision to users.
>
OK.
> > 4.) I would take a look at this patch (#902) again--are their any
> > classes or interfaces being used which are *identical* to ones in other
> > samples? I don't know how others feel, but I would just reference them
> > in the ant.build file when compiling and creating the WAR file, instead
> > of constantly recreating identical classes. This will also help us in
> > the future when we starting factoring out common classes that are used
> > by multiple samples.
> >
> >
> I don't like reusing too much, but I would like to hear others comments on
> this...
OK, that's acceptable.
Regards,
Glen
