|
[
Permlink
| « Hide
]
Michelle Caisse added a comment - 21/Dec/05 10:14 AM
revision 358164 - Added connection pooling jars to project.class.path and removed reference implementation jars. Must now copy iut jars to iut_jars.
I've attached a patch for the next portion of this issue -- creating a separate classpath for the iut enhancer and the jdori enhancer. Currently we have properties iut.enhancer.sourcepath, iut.enhancer.classpath, jdo.enhancer.sourcepath, and jdo.enhancer.classpath in project.properties, but none of these are actually used in maven.xml. I see the value in setting iut.enhancer.classpath in a properties file and adding it to the classpath in maven.xml and this patch does that. I think that it is more readable and convenient to set jdo.enhancer.*path in maven.xml. I don't know if users will need to add items to iut.enhancer.sourcepath that we cannot foresee. Feedback?
I like the direction you're on, and have some specific comments.
I'd rewrite part of the objective as follows: Currently we have properties iut.enhancer.sourcepath, iut.enhancer.classpath, jdo.enhancer.sourcepath, and jdo.enhancer.classpath in project.properties, but none of these are actually used in maven.xml. I see the value in setting iut.enhancer.classpath in a properties file and adding it to the classpath in maven.xml and this patch does that. I think that it is more readable and convenient to set jdori.enhancer.*path in maven.xml. I don't know if users will need to add items to iut.enhancer.sourcepath that we cannot foresee. I think that if we define the enhancer execution classpath (where the iut.enhancer.main and its dependencies can be found) and enhancer source classpath (where the un-enhanced classes and .jdo metadata files are found) plus enhancer properties (arbitrary command line arguments for the execution of the enhancer) we are covered. Then there should be minimum requirement to change the maven.xml but instead simply update the provided sample iut.properties file (with all the properties commented). 1. +iut.enhancer.classpath = ${jpox.enhancer.jarfile}${path.separator}$... It appears that with this patch you can still run iut.enhance right after downloading the tck20 project and you will get the jpox implementation. I'm not convinced that this is the right thing to do. I think I'd rather have it fail with a message "Enhancer Main class is not specified" or something that indicates that the user has to configure the installation to use an iut. We can provide instructions on how to run JPOX as the iut but I feel uncomfortable with having it run out-of-the-box. 2. +#jdo.enhancer.classpath = ${jpox.enhancer.jarfile}${path.separator}$... Do your changes to this still allow JPOX enhancer to run as the JDO RI? 3. The use of "jdo" in the same context as "iut" to mean the RI bothers me a bit. Can we use "jdori" instead of "jdo" for this purpose, e.g. instead of: jdo.enhancer.options = -v -d "${enhanced.dir}" jdo.enhancer.args = ${jdo.tck.jdometadata.files} jdo.enhancer.sourcepath = ${jdo.tck.testclasses.dir}${path.separator}${jdo.jdoapi.jarfile}${path.separator}${junit.jarfile}${path.separator}${log4j.jarfile} jdo.enhancer.classpath = ${jpox.enhancer.jarfile}${path.separator}${jpox.jdori.jarfile}${path.separator}${jdo.jdoapi.jarfile}${path.separator}$ we could use: jdori.enhancer.options = -v -d "${enhanced.dir}" jdori.enhancer.args = ${jdo.tck.jdometadata.files} jdori.enhancer.sourcepath = ${jdo.tck.testclasses.dir}${path.separator}${jdo.jdoapi.jarfile}${path.separator}${junit.jarfile}${path.separator}${log4j.jarfile} jdori.enhancer.classpath = ${jpox.enhancer.jarfile}${path.separator}${jpox.jdori.jarfile}${path.separator}${jdo.jdoapi.jarfile}${path.separator}$ 4. In maven.xml, the element naming seems inconsistent with the other files. We have: + <!-- Paths needed for enhancement by iut enhancer. --> + <path id="enhance.iut.classpath"> Shouldn't this be e.g. + <!-- Paths needed for enhancement by iut enhancer. --> + <path id="iut.enhancer.classpath"> CLR: "I think that if we define the enhancer execution classpath (where the iut.enhancer.main and its dependencies can be found) and enhancer source classpath (where the un-enhanced classes and .jdo metadata files are found) plus enhancer properties (arbitrary command line arguments for the execution of the enhancer) we are covered. Then there should be minimum requirement to change the maven.xml but instead simply update the provided sample iut.properties file (with all the properties commented)."
0.a. I don't think the enhancer source classpath is needed in the iut.properties file because it is determined by the build itself and therefore known to maven.xml. 0.b. Do you agree that there is no benefit to setting the jdori enhancer classpaths in project.properties over setting them in maven.xml? 1. You are correct that the jpox enhancer will run out of the box with this patch. I did this mainly for convenience in debugging, but I can change it now. 2. Yes, everything works. Those lines that I commented out in project.properties were never referenced in maven.xml Commenting them out only serves to prove that. If the answer to 0.b. is "yes", I will delete them. 3. I agree with you and it bothers me too. I will happily make this change. 4. Good idea. I will make the change. 0. I think I misunderstood how this worked (defining the properties in the iut.properties instead of maven.xml or another properties file), and I now agree with you.
1. Ok. I understand why you wanted to test the scripts. I think now we need to test the "how to run your jdo implementation" part of the "run rules". Oh, yeah, that's what I'm supposed to do... Good job. 0. I hope we find a way that the user does not need to change maven.xml at all when running the TCK with an iut. I hope adapting the project.properties is all he needs to do.
4. I agree with calling it iut.enhancer.classpath. So how about the other classpath names? Should they become iut.classpath, jdori.classpath and jdori.enhancer.classpath? I hope we can rename project.class.path to iut.classpath, since I'm not sure whether project.class.path in some of the standard goal we are using. But it's worth trying. Since are are chaning the mavel.xml anyway: the definition of project.class.path includes two lines <pathelement location="${jdo.tck.enhanced.dir}/${jdo.tck.identitytype}/${jdo.tck.identitytype}.jar"/> <pathelement location="${jdo.tck.enhanced.dir}/${jdo.tck.identitytype}"/> I think the first line should be: <pathelement location="${jdo.tck.enhanced.dir}/${jdo.tck.identitytype}.jar"/> and the second line should be removed. Michael,
I plan to add an additional properties file called iut.properties where people can make all the changes needed for the iut without even touching project.properties. I will try to come up with a uniform convention for all the classpath names. This patch adressed only enhancer classpath issues. I will take a look at project.class.path next. It definitely should be renamed. Thanks for the pointer on a possible problem with the path. Another question: We currently have two mechanisms for finding classes for the iut and its dependencies:
(1) The iut tester puts stuff in iut_jars, and maven.xml references that location. (2) The iut tester sets iut.enhancer.classpath and maven.xml references that. In other words, we use different mechanisms for the iut jdo implementation and for the iut enhancer. Is this what we want? The attached patch is responsive to the comments received so far, except that the ri enhancer still runs out of the box when the user specified the iut. I would like to defer this change to a later phase for ease of debugging now. Thank you, reviewers, for your comments.
The patch looks good.
About your question: I'm not sure whether we still need the property iut.enhancer.classpath. Especially because maven.xml defines a path variable having the same name. This might be confusing. So I would be fine using approach (1): take all the iut jars from the directory iut_jars and add them to the path variable iut.enhancer.classpath. Checked in patch2 with revision 358890.
Interim check in: revision 367456
Changed class.path to classpath. Added iut..classpath, parallel to jdori.classpath. Removed testrunner.set goal -- we don't support GUI testrunner. Removed project.classpath, which is not needed. revision 367815
I propose removing the following unused properties from project.properties:
maven.junit.fork = yes maven.junit.dir = ${jdo.tck.testdir} maven.junit.sysproperties = PMFProperties Instead, we hardcode fork="yes" and refer directly to jdo.tck.testdir, e.g. ...<java fork="yes" dir="${jdo.tck.testdir}"... The use of PMFProperties in maven.xml requires further study, but maven.junit.sysproperties is currently not used and that is the issue for this proposal. Any objections? revision 368404: Refactored classpath properties for clarity and to avoid duplication.
This patch creates a build.properties file which contains properties that may be modified by the iut. (See http://maven.apache.org/maven-1.x/reference/properties.html for information about how maven uses build.properties.) The patch removes most of these properties from project.properties. Some properties in build.properties override those in project.properties.
The patch renames test/conf/jdori.properties to test/conf/jdori-pmf.properties and /test/conf/iut.properties to test/conf/iut-pmf.properties. (As I read the patch, it shows the old file names deleted but doesn't show the new ones added. I don't know why; they do show up in my workspace as added.] The changes look good. I also don't know why the rename isn't shown in the diff.
Committed patch with revision 370285. Still open for comments.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Resolved
Major
Fix issues in maven.xml preventing iut goals from running properly
