Hi,
See my comment inline.
Thanks
Jeff
Glen Mazza wrote:
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.
The reason that why you think the "WHICH_JARS" in the lib lists much smaller
jars, is it uses the all-in-one jar as I said before. Well, I can update it to use the
the all-in-one
jar instead of module-based jars for the simplicity, but I still think list 40 more jars is not a good idea. If we want to make users clear which jars will be needed for their usage,
I think that is why the "WHICH_JARS" existed.
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.
Sounds good, will update the patch as you suggested.
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
